secured THREDDS tests

[fmigneault]

• add pipeline option to run notebooks in notebooks-auth
• basic setup of secured THREDDS tests against Magpie/Twitcher config

requires:

• bump magpie and twitcher to 3.2.1 bird-house/birdhouse-deploy#102
• Magpie thredds bird-house/birdhouse-deploy#107

[tlvu]

Thanks for writing extra test for Magpie. Some good round-trip test cases.

However I think you misunderstood the purpose of this test suite. It's being run on live production instance! It's not just run inside the DACCS-iac fresh and disposable instances. @matprov never told you about this?! This same test suite probably also hit your hirondelle nightly. On our side it's nightly agaisnt our prod.

I see Thredds access permissions being changed without being restored!

I suggest add a clean-up at the end to revert all the permission changes and delete all the created test user/group and make the whole test case not enabled by default. Then @matprov can enable it in the DACCS-iac while our nightly hitting prod will not enable it. Only run against prod manually but not nightly.

I have not finished reading the notebook, will continue later.

[fmigneault]

@tlvu

I see Thredds access permissions being changed without being restored!

The permissions are changed for a new group created on the fly during that test suite, so it will not impact access to other users/groups unless they are randomly added to that group.

I suggest add a clean-up at the end [...]

I agree. I can add a cleanup of said test group afterwards execution, but I cannot 100% guarantee everything will be cleaned properly if the reason the test fails is caused by Magpie errors. So a "clean as best as possible" with some try/except.

If that test feels too critical for a given server, it can be disabled using Jenkins' TEST_MAGPIE_AUTH setting.

[tlvu]

If that test feels too critical for a given server, it can be disabled using Jenkins' TEST_MAGPIE_AUTH setting.

Exact, that's why I suggested default the TEST_MAGPIE_AUTH setting to false.

[fmigneault]

If that test feels too critical for a given server, it can be disabled using Jenkins' TEST_MAGPIE_AUTH setting.

Exact, that's why I suggested default the TEST_MAGPIE_AUTH setting to false.

My personal take on this is that it is better to have all tests enabled by default, and if you don't want them for specific reasons, you disable them with the knowledge under which deactivating them is reasonable.
Nobody goes through all the configs to see which feature was tested or not (especially if "all green"), it is just assumed to be tested.
As an external user, I would much rather have an error indicating security related test fails, and disable them because they are not needed for my case, than not have anything tested and potentially leaking data / access because I didn't know I add to activate that test.

For now I applied false, but I feel this is something that is going to be always overridden, or that is going to bite us back at a later time.

[tlvu]

My personal take on this is that it is better to have all tests enabled by default, and if you don't want them for specific reasons, you disable them with the knowledge under which deactivating them is reasonable.
Nobody goes through all the configs to see which feature was tested or not (especially if "all green"), it is just assumed to be tested.
As an external user, I would much rather have an error indicating security related test fails, and disable them because they are not needed for my case, than not have anything tested and potentially leaking data / access because I didn't know I add to activate that test.

It's not un-common to have a make test and make test-all where make test only test a subset. So not running all tests by default is fairly common.

That said, I hear you point.

So I would suggest bringing your 3 new options all the way to the top to make them really visible.

Also add in the TEST_MAGPIE_AUTH description that "This test suite might require manual clean-up on failure, please follow instructions in the test." So user understand the impact of enabling this test.

I also assume there are instructions with sufficient details in the test for manual cleanup if something go south?

[fmigneault]

@tlvu

So I would suggest bringing your 3 new options all the way to the top to make them really visible.

I'm not sure of what you mean by the top here? Do you mean setting true in default_build_params, testall or some other place?
Script testall already has a check [ x"$TEST_MAGPIE_AUTH" = xtrue ], but I can add the default in other places as well.
It seems like there are many possible entrypoints to run those scripts, so the defaults should at least be aligned.

I also assume there are instructions with sufficient details in the test for manual cleanup if something go south?

Yes, I added clenup in new commits. It only really needs to remove the test user/group.
No existing user/group are used, new ones are created specifically for the test.
So worst case is that some extra users/groups are left in Magpie if the test fails drastically, but even then, they don't modify access to existing ones.
The directories used are also under testdata, so shouldn't really impact "real" data and permissions.

[tlvu]

So I would suggest bringing your 3 new options all the way to the top to make them really visible.

I'm not sure of what you mean by the top here? Do you mean setting true in default_build_params, testall or some other place?
Script testall already has a check [ x"$TEST_MAGPIE_AUTH" = xtrue ], but I can add the default in other places as well.
It seems like there are many possible entrypoints to run those scripts, so the defaults should at least be aligned.

I meant the list of params when requesting a jenkins build as in here https://daccs-jenkins.crim.ca/job/PAVICS-e2e-workflow-tests/job/magpie-func-tests/build.

To increase awareness that there are additional tests available, I was proposing to move those 3 new jenkins param high up in that page, just below PAVICS_HOST, which is the most useful and important param.

The order these params are in the Jenkinsfile is the same order they are displayed on the build request page.

[fmigneault]

@tlvu Oh ok, I see what you mean. Will do.

[tlvu]

Also there has been a massive documentation restructure on birdhouse-deploy repo (PR bird-house/birdhouse-deploy#118) so expect merge conflict with your merge with master.

[tlvu]

Branches not up-to-date with master for both repos. Some other Jenkins failure not related to lax mode so real failure and need to be addressed. Might need to update catalog_search.ipynb as well.

[tlvu]

By the way, is the host tested by Jenkins https://host-140-88.rdext.crim.ca from bird-house/birdhouse-deploy#102? The Magpie/Twitcher combo is still 3.2.1 https://github.com/bird-house/birdhouse-deploy/blob/c9ce4ca276b3d3560a8a95b79241f926d88cb759/birdhouse/docker-compose.yml#L284

Are you trying to test this PR bird-house/birdhouse-deploy#107 or bird-house/birdhouse-deploy#114?

[fmigneault]

Using bird-house/birdhouse-deploy#107 with Magpie/Twitcher 3.5.1
The permission not set error is caused by cleanup of the test-group following previous cell failing.

[fmigneault]

@huard
Following notebook references raised errors :

16:41:15  _____ pavics-sdi-master/docs/source/notebooks/pavics_thredds.ipynb::Cell 1 _____
16:41:15  Notebook cell execution failed
16:41:15  Cell 1: Cell execution caused an exception
16:41:15  
16:41:15  Input:
16:41:15  secured_url = "https://host-140-88.rdext.crim.ca/twitcher/ows/proxy/thredds/dodsC/birdhouse/testdata/secure/tasmax_Amon_MPI-ESM-MR_rcp45_r2i1p1_200601-200612.nc"
16:41:15  

16:41:15  OSError: [Errno -78] NetCDF: Authorization failure: b'https://host-140-88.rdext.crim.ca/twitcher/ows/proxy/thredds/dodsC/birdhouse/testdata/secure/tasmax_Amon_MPI-ESM-MR_rcp45_r2i1p1_200601-200612.nc'
16:41:15  
16:41:15  _____ pavics-sdi-master/docs/source/notebooks/pavics_thredds.ipynb::Cell 2 _____
16:41:15  Notebook cell execution failed
16:41:15  Cell 2: Cell execution caused an exception

[huard]

Ok. Could you send me a code snippet to authenticate ? I'll update the notebook once the new magpie auth system is deployed.
Is that how you saw the sequence ?

[fmigneault]

@huard
Sorry for the delay, didn't notice your message.
Simple as this:

def login_user(username, password):
    _data = {"user_name": username, "password": password}
    _path = MAGPIE_URL + "/signin"
    _head = {"Accept": "application/json", "Content-Type": "application/json"}
    _resp = requests.post(_path, json=_data, verify=False, headers=_head)
    assert _resp.status_code == 200
    return _resp.cookies


MAGPIE_URL = "https://<PAVICS_FQDN_PUBLIC>/magpie"
cookies = login_user("admin", "qwerty")
resp = requests.get("<protected-resource-url>", cookies=cookires)

[fmigneault]

Update:
Functional test_thredds notebook using https://host-140-36.rdext.crim.ca
http://daccs-jenkins.crim.ca/job/PAVICS-e2e-workflow-tests/job/magpie-func-tests/31/execution/node/28/log/

@huard @tlvu
With those updates, one of the only thing still failing is https://github.com/Ouranosinc/pavics-sdi/blob/master/docs/source/notebooks/catalog_search.ipynb
How do you want this service to be accessed? Full open (meaning magpie permission must be added here), or limited to CATALOG_USERNAME (notebook must be updated with auth), or a combination of both for different GetCapabilites/DescribeProcess/Execute WPS operations?

[huard]

No need to invest time in catalog_search. I'm ok with deprecating it and removing it entirely from our NBs.
We're gradually transitioning to a new catalog system designed by Mathieu P.

[tlvu]

Update:
Functional test_thredds notebook using https://host-140-36.rdext.crim.ca
http://daccs-jenkins.crim.ca/job/PAVICS-e2e-workflow-tests/job/magpie-func-tests/31/execution/node/28/log/

Superb ! So if we disable catalog_search.ipynb (either delete or move it somewhere else), there is pavics_thredds.ipynb left. I am guessing it will be fixed by Ouranosinc/pavics-sdi#209 and then we will have a completely successful test run?

[fmigneault]

@tlvu Exactly.

[tlvu]

@tlvu Exactly.

+1 waiting for that passing build !

[tlvu]

Trying out a new tool "ReviewNB". Use "Reply via ReviewNB" for a much better display of the notebook cells the comment was meant for.

[fmigneault]

All changes applied in #67

[fmigneault]

Why in this commit d1fe816 that is only supposed to move the new options up, changed the default value of TEST_MAGPIE_AUTH from false to true?

I thought we agreed to keep it to false here #55 (comment).

Another reason to keep this option to false by default, is this test suite also runs on nightly on production instances so the admin password will never be the default value, for security reason, so this new notebook will never pass.

Furthermore, you merged the PR #55 of this commit when it is still in the "changes requested" status. It would have been less rude if you ping me asking me to review it again and merge only after receiving an "approve".

@tlvu
I'm not sure what happened because I did change it in 46f1c61 .
Maybe some merge/rebase cause a reset I didn't notice.
Same thing for for the duplicated cells.

Sorry I misinterpreted +1 waiting for that passing build ! as "good to go when passing"
I will re-ping you again next time to make double check.