bump magpie and twitcher to 3.2.1 #102

matprov · 2020-11-10T21:37:45Z

bump magpie to 3.2.0
bump twitcher to magpie-3.2.0
let the anonymous user browse Thredds

…to anon

matprov · 2020-11-10T21:46:53Z

@tlvu @fmigneault This PR replaces bump_twitcher_to_magpie-3.1.0 -> master, with a cleaner diff.

tlvu · 2020-11-10T23:31:42Z

Thanks for the cleaner diff ! Much appreciated.

@fmigneault will move part of #99 (comment) here, then we can test everything together.

And let me do this merge myself once I also test an upgrade on my perso server then our staging server. The pipeline only test fresh new install, but in production it's actually an upgrade with existing Magpie DB. This is a fairly big change so I'd like to be more cautious.

secure config for thredds

tlvu · 2020-11-12T19:47:15Z

@matprov can you also enable @fmigneault new ./optional/components/secure-thredds (in this PR) so we can test this new revoke permission feature?

matprov · 2020-11-12T20:34:57Z

can you also enable new ./optional/components/secure-thredds (in this PR) so we can test this new revoke permission feature?

@tlvu It's now enabled and will be applied in future instance creation. Will need to be remove as soon as we don't need it, in test instances.

…tcher_to_3.2.0

tlvu · 2020-11-13T22:30:56Z

I deployed this PR on my dev server with all-public-access and secure-thredds components both activated and my secure folder https://lvupavicsdev.ouranos.ca/twitcher/ows/proxy/thredds/catalog/birdhouse/testdata/secure/catalog.html is still accessible publicly.

In the logs I see

[2020-11-13 22:17:00,930] INFO       [MainThread][magpie.register] Processing permissions from configuration (4/4).                                   
[2020-11-13 22:17:00,930] INFO       [MainThread][magpie.register] Found 3 permissions to evaluate from configuration.                                
[2020-11-13 22:17:00,930] WARNING    [MainThread][magpie.register] Unknown permission [browse-deny-recursive] [permission #0], skipping...            
[2020-11-13 22:17:00,930] WARNING    [MainThread][magpie.register] Unknown permission [read-deny-recursive] [permission #1], skipping...              
[2020-11-13 22:17:00,931] WARNING    [MainThread][magpie.register] Unknown permission [write-deny-recursive] [permission #2], skipping...

Looks like the config is not applied properly.

fmigneault · 2020-11-16T17:30:19Z

@tlvu FYI
I found the problem, working on a fix that should come up soon.

edit
PR for issue in #102 (comment):
Ouranosinc/Magpie#368

tlvu · 2020-11-16T22:04:08Z

@matprov is it possible for the DACCS-iac pipeline to save docker logs of all the containers to the job before killing the server? Then to save those files to the Jenkins job you can do something like https://github.com/Ouranosinc/PAVICS-e2e-workflow-tests/blob/5d5a9aa2251386378406efb5b414b3aa6db0b37e/Jenkinsfile#L80-L81

matprov · 2020-11-16T22:32:24Z

@matprov is it possible for the DACCS-iac pipeline to save docker logs of all the containers to the job before killing the server? Then to save those files to the Jenkins job you can do something like https://github.com/Ouranosinc/PAVICS-e2e-workflow-tests/blob/5d5a9aa2251386378406efb5b414b3aa6db0b37e/Jenkinsfile#L80-L81

@tlvu Yes absolutely, there might be a way to forward the instance's logs to jenkins artifacts. I will look at this soon, thanks.

matprov · 2020-11-17T02:48:51Z

@tlvu FYI docker-compose logs are now stored as jenkins artifacts.
You can also access it using date | netcat IP_MACHINE 9875 > docker-compose.log.

fmigneault · 2020-11-17T21:41:13Z

@matprov @tlvu
Magpie 3.2.1 is available with fix of permission parsing.
I would suggest also adding type: directory field to THREDDS entries to make sure there is not any ambiguity when the resource must be created by Magpie.

tlvu · 2020-11-18T00:28:02Z

I would suggest also adding type: directory field to THREDDS entries to make sure there is not any ambiguity when the resource must be created by Magpie.

@fmigneault Thanks for the fix. Since you know it best, can you add it?

Since CRIM owns Twitcher/Magpie, I prefer to let you guys deal with any config changes required for the update.

I can help final testing once you guys confirm the PR is good to go. You guys should make sure to test both fresh deployment and upgrade existing deployment.

…ce type to permission config

…tcher_to_3.2.0

tlvu · 2020-11-20T14:06:10Z

@fmigneault

Would this also have worked for the .gif and .txt file issue? If we actually want to have access control on them? Meaning some .gif and .txt under some folder is private?

providers:
  LocalThredds:
    file_patterns:
      # note: make sure to employ quotes and double escapes to avoid parsing YAML error
      - ".*\\.nc"
      - ".*\\.gif"
      - ".*\\.txt"
    metadata_type:
      prefixes:
        - null  # note: special YAML value evaluated as `no-prefix`, use quotes if literal value is needed
        - "catalog\\.\\w+"  # note: special case for `THREDDS` top-level directory (root) accessed for `BROWSE`
        - "\\*\\.gif"
        - "\\*\\.txt"
        - ncml
        - (...)
    data_type:
      prefixes:
        - "\\*\\.gif"
        - "\\*\\.txt"
        - fileServer
        - (...)

fmigneault · 2020-11-20T16:51:09Z

Would this also have worked for the .gif and .txt file issue? If we actually want to have access control on them? Meaning some .gif and .txt under some folder is private?

providers:
  LocalThredds:
    file_patterns:
      # note: make sure to employ quotes and double escapes to avoid parsing YAML error
      - ".*\\.nc"
      - ".*\\.gif"
      - ".*\\.txt"
    metadata_type:
      prefixes:
        - null  # note: special YAML value evaluated as `no-prefix`, use quotes if literal value is needed
        - "catalog\\.\\w+"  # note: special case for `THREDDS` top-level directory (root) accessed for `BROWSE`
        - "\\*\\.gif"
        - "\\*\\.txt"
        - ncml
        - (...)
    data_type:
      prefixes:
        - "\\*\\.gif"
        - "\\*\\.txt"
        - fileServer
        - (...)

We will have to try it out. It could work, but due to the prefix handling, I am not 100% sure. Maybe will require top-level resources named like the .gif and .txt files.

fmigneault · 2020-11-20T19:36:37Z

@tlvu
I did some testing and something we didn't consider occurred with above custom config.
When doing the request on Twitcher, it doesn't know about that config since it is loaded via providers.cfg on Magpie side.
I will need to add that parsing/loading in MagpieAdapter for Twitcher.
The providers config will also need to be mounted into the Twitcher container.

tlvu · 2020-12-08T15:24:20Z

@fmigneault By the way, does this Twitcher/Magpie upgrade will also include bird-house/twitcher#98 ?

fmigneault · 2020-12-09T01:35:02Z

@tlvu

By the way, does this Twitcher/Magpie upgrade will also include bird-house/twitcher#98 ?

We can include it while we are at it.

tlvu · 2020-12-09T01:49:20Z

By the way, does this Twitcher/Magpie upgrade will also include bird-house/twitcher#98 ?

We can include it while we are at it.

@fmigneault Oh it's not already in this PR? Then would it be possible to just have the Twitcher fix separately so it can be quickly merged?

fmigneault · 2020-12-09T05:30:17Z

Oh it's not already in this PR? Then would it be possible to just have the Twitcher fix separately so it can be quickly merged?

@tlvu merged Ouranosinc/Magpie#377

but I'm not planing to build another tag until Ouranosinc/Magpie#373 is complete
(which is extremely close to be done, but travis-ci is making it longer as I must run everything locally beforehand)

tlvu · 2020-12-09T15:26:53Z

Oh it's not already in this PR? Then would it be possible to just have the Twitcher fix separately so it can be quickly merged?

@tlvu merged Ouranosinc/Magpie#377

but I'm not planing to build another tag until Ouranosinc/Magpie#373 is complete
(which is extremely close to be done, but travis-ci is making it longer as I must run everything locally beforehand)

Thanks @fmigneault

It's not 100% clear to me the inter dependencies between the 2 Magpie PR above and the Twitcher performance fix.

I simply prefer to have only the Twitcher performance fix alone without all the new features in Magpie. Is that possible so the Twitcher performance fix can go-live quickly?

fmigneault · 2020-12-09T16:43:25Z

@tlvu
Sure. edit pushed 3.4.0, should be building docker soon enough.
Tests with THREDDS will still fail until next PR is merged, so it will need to be bumped again very soon.

tlvu · 2020-12-09T18:04:09Z

Sure. edit pushed 3.4.0, should be building docker soon enough.
Tests with THREDDS will still fail until next PR is merged, so it will need to be bumped again very soon.

@fmigneault Why? We are only bumping Twitcher, not Magpie so no Magpie behavior change so why would Thredds tests fail?

tlvu · 2020-12-09T19:08:48Z

Why? We are only bumping Twitcher, not Magpie so no Magpie behavior change so why would Thredds tests fail?

@fmigneault We currently have also a very old twitcher pavics/twitcher:magpie-1.7.3. You mentioned in the meeting to alter the config of twitcher to get the performance fix, would that work with our current old twitcher?

If that is simple, can you then just submit a separate PR so we get the Twitcher perf fix in prod without having to wait for this big Magpie upgrade?

fmigneault · 2020-12-09T22:56:02Z

@tlvu

What I mean is that running the old Magpie, THREDDS will remain fully open. The PAVICS-end2end test that is added to validate secure directory is protected will not work.

If what you desire is only to backport Twitcher 0.5.4, to match Magpie 1.7.x, there is no actual change in code, so no new version needed either.

What I propose is to run gunicorn directly. The only thing that changes in that new Twitcher is the "default" WSGI server runner.
So this can be achieved with modified twitcher.ini that we override anyway such that it says

[server:main]
use = egg:gunicorn#main
host=localhost
port=8001

instead of

[server:main]
use = egg:waitress#main
bind = 0.0.0.0:8000

tlvu · 2020-12-10T14:51:06Z

For the record here, tested @fmigneault proposed backport of Twitcher perf fix, no improvements at all, see comment bird-house/twitcher#97 (comment)

matprov · 2020-12-13T13:44:26Z

@matprov can you also enable @fmigneault new ./optional/components/secure-thredds (in this PR) so we can test this new revoke permission feature?

@tlvu
Adding ./optional/components/secure-thredds is what prevents the server from correctly booting, potentially due to missing rights for some THREDDS path, leading to OWS exception when triggering the pavics crawler ValueError: Does not appear to be a Thredds catalog.

However, running the test environment without the secure-thredds component makes the build pass.

Would it be possible that ValueError: Does not appear to be a Thredds catalog is due to an inaccessible path?

tlvu · 2020-12-13T18:12:56Z

Adding ./optional/components/secure-thredds is what prevents the server from correctly booting, potentially due to missing rights for some THREDDS path, leading to OWS exception when triggering the pavics crawler ValueError: Does not appear to be a Thredds catalog.

However, running the test environment without the secure-thredds component makes the build pass.

Would it be possible that ValueError: Does not appear to be a Thredds catalog is due to an inaccessible path?

@fmigneault Could you take a look since you wrote ./optional-components/secure-thredds?

tlvu · 2020-12-13T18:20:28Z

@fmigneault @matprov

Found out ./optional-components/secure-thredds was merged without the pipeline being triggered (PR #99), aka it was never tested.

We should be much more strict to not merge if pipeline do not pass in the future.

matprov · 2020-12-13T18:40:37Z

Found out ./optional-components/secure-thredds was merged without the pipeline being triggered (PR #99), aka it was never tested.

We should be much more strict to not merge if pipeline do not pass in the future.

@tlvu This is due to the fact that PR commits done by Francis weren't picked up by the pipeline automatically, therefore not updating build status and not underlining potential issues.

New PR commits are now supposed to trigger the pipeline (known issue).

fmigneault · 2020-12-14T15:33:56Z

@tlvu it wasn't merged in master, but in this branch because it needs the new configs of #99 to do its job.
What is the problem with that?

tlvu · 2020-12-14T15:42:51Z

it wasn't merged in master, but in this branch becaus e it needs the new configs of #99 to do its job.
What is the problem with that?

@fmigneault I never said anything about #99 breaking master.

@matprov found out #99 breaks this branch so you should take a look. And maybe get used to use the pipeline for testing because #99 was not tested when it was merged.

fmigneault · 2020-12-15T17:44:51Z

@tlvu The "breaking merge" is intended, and it was known that it would break because Magpie priority permission is not completed. There is no plan to merge this new version of Magpie (before added "breaking merge"), as it doesn't bring anything "new" by itself.

tlvu · 2021-03-24T14:51:05Z

@fmigneault I can close this PR (since replaced by PR #107) and delete this branch as well?

tlvu · 2021-03-24T20:26:42Z

wait what??? merge into master??? Aren't you supposed to just close the PR?

tlvu · 2021-03-24T20:27:27Z

@fmigneault Can you make a new PR to undo this accidental merge?

tlvu · 2021-03-24T20:30:09Z

@fmigneault you are merging everything, well the other PR #107 already include this one so there was no need to merge this one anyways.

updated magpie and twitcher to 3.2.0 + add thredds browse permission …

807a13d

…to anon

matprov requested review from tlvu and fmigneault November 10, 2020 21:46

fmigneault approved these changes Nov 10, 2020

View reviewed changes

tlvu mentioned this pull request Nov 10, 2020

secure config for thredds #99

Merged

fmigneault added 2 commits November 10, 2020 18:49

secure config for thredds

d2c6405

readme secure-thredds updates + remove public handled by other config

5bcc1c3

matprov assigned tlvu Nov 11, 2020

fmigneault added 5 commits November 11, 2020 11:04

remove top-level secure directory permission + add write deny permission

89cbd89

add readme details

d9f4739

fix typos and over-length doc lines

2c460c9

fix markdown links

2ddf64e

add how-to enable secure component

c6982c3

fmigneault mentioned this pull request Nov 12, 2020

bump twitcher to magpie-3.2.0 #89

Closed

Merge pull request #99 from bird-house/test-magpie-auth

b08f972

secure config for thredds

tlvu added 2 commits November 13, 2020 16:59

Merge remote-tracking branch 'origin/master' into bump_magpie_and_twi…

9759e1d

…tcher_to_3.2.0

README: remove trailing white

90b27b3

create-magpie-users: bump magpie image to match newer version

32cd6f9

bump magpie 3.2.1 (fix permission modifiers from config) + add resour…

8f8eb2a

…ce type to permission config

Merge remote-tracking branch 'origin/master' into bump_magpie_and_twi…

c9ce4ca

…tcher_to_3.2.0

This was referenced Nov 21, 2020

Store custom config JSON in db Ouranosinc/Magpie#372

Merged

secured THREDDS tests Ouranosinc/PAVICS-e2e-workflow-tests#55

Merged

fmigneault mentioned this pull request Feb 18, 2021

Adapter cache service Ouranosinc/Magpie#388

Merged

fmigneault merged commit c9ce4ca into master Mar 24, 2021

fmigneault deleted the bump_magpie_and_twitcher_to_3.2.0 branch March 24, 2021 20:25

bump magpie and twitcher to 3.2.1 #102

bump magpie and twitcher to 3.2.1 #102

Conversation

matprov commented Nov 10, 2020

matprov commented Nov 10, 2020

tlvu commented Nov 10, 2020

tlvu commented Nov 12, 2020

matprov commented Nov 12, 2020

tlvu commented Nov 13, 2020

fmigneault commented Nov 16, 2020 • edited

tlvu commented Nov 16, 2020

matprov commented Nov 16, 2020

matprov commented Nov 17, 2020

fmigneault commented Nov 17, 2020

tlvu commented Nov 18, 2020

tlvu commented Nov 20, 2020

fmigneault commented Nov 20, 2020

fmigneault commented Nov 20, 2020

tlvu commented Dec 8, 2020

fmigneault commented Dec 9, 2020

tlvu commented Dec 9, 2020

fmigneault commented Dec 9, 2020

tlvu commented Dec 9, 2020

fmigneault commented Dec 9, 2020 • edited

tlvu commented Dec 9, 2020

tlvu commented Dec 9, 2020

fmigneault commented Dec 9, 2020

tlvu commented Dec 10, 2020

matprov commented Dec 13, 2020 • edited

tlvu commented Dec 13, 2020

tlvu commented Dec 13, 2020

matprov commented Dec 13, 2020

fmigneault commented Dec 14, 2020

tlvu commented Dec 14, 2020

fmigneault commented Dec 15, 2020

tlvu commented Mar 24, 2021

tlvu commented Mar 24, 2021

tlvu commented Mar 24, 2021

tlvu commented Mar 24, 2021

fmigneault commented Nov 16, 2020 •

edited

fmigneault commented Dec 9, 2020 •

edited

matprov commented Dec 13, 2020 •

edited