Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update weaver 6.1.0 #489

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

update weaver 6.1.0 #489

wants to merge 8 commits into from

Conversation

fmigneault
Copy link
Collaborator

@fmigneault fmigneault commented Dec 20, 2024

Overview

Apply latest Weaver to apply all updates and fixes since 5.6.1.

Changes

Non-breaking changes

  • Weaver: update weaver component default version to 6.1.0.

    Relevant changes

    • Add support of OGC API - Processes - Part 3: Workflows and Chaining with Nested Process ad-hoc workflow.
    • Add support of OGC API - Processes - Part 3: Workflows and Chaining with Remote Collection (STAC and OGC).
    • Add support of OGC API - Processes - Part 4: Job Management endpoints for job "pending" creation and execution.
    • Add support of OGC API - Processes - Part 4: Job Management endpoints for job provenance as W3C PROV metadata.
    • Multiple alignment and fixes related to latest OGC API - Processes - Part 1: Core definitions regarding handling
      of input parameters and headers when submitting jobs to obtain alternate result representations and behavior.
    • Add HTML responses by default via web browsers or as requested by Accept headers or f query parameter.
    • Add improved CWL schema validation with Weaver-specific definitions where applicable
      (see https://github.com/crim-ca/weaver/tree/master/weaver/schemas/cwl).
  • Weaver: modifications to proxy configurations for weaver

    • Add WEAVER_ALT_PREFIX optional variable that auto-configures WEAVER_ALT_PREFIX_PROXY_LOCATION,
      which allows setting an alternate endpoint to redirect requests to weaver.
      It uses /ogcapi by default which is a very common expectation from servers supporting OGC standards.
    • Use the TWITCHER_VERIFY_PATH approach to accelerate access of weaver resources authorization.
    • Modify proxy pass definitions and URL prefixes to resolve correctly with HTML resources.

Breaking changes

  • n/a

Related Issue / Discussion

  • Related to OGC Testbed-20 initiatives.

Additional Information

Links to other issues or sources.

CI Operations

birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false

@fmigneault fmigneault self-assigned this Dec 20, 2024
@github-actions github-actions bot added component/magpie Related to https://github.com/Ouranosinc/Magpie component/weaver Related to https://github.com/crim-ca/weaver documentation Improvements or additions to documentation feature/WPS Feature or service related to Web Processing Service labels Dec 20, 2024
@fmigneault
Copy link
Collaborator Author

fmigneault commented Dec 20, 2024

@mishaschwartz
To deploy this PR, I took the time to migrate from 1.42.2 to "2.7.0+this-PR".

For some reason, I could never get this variable to be detected (although set), and although everything else works fine:

\$ALERTMANAGER_ADMIN_EMAIL_RECEIVER

Any idea?

The definition seems to be as per usual, so I don't see what is wrong specifically in that case.
I had to disable monitoring in the meantime to make this work.

Copy link
Collaborator

@mishaschwartz mishaschwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the nginx config needs to be looked at again. When I test this out I run into all sorts of issues:

  • <my-test-url>/weaver/ -> nginx error: no resolver defined to resolve weaver while sending to client
  • <my-test-url>/weaver/api -> same nginx error as above
  • <my-test-url>/weaver/weaver -> magpie permission rejected as the anonymous user (and same nginx error above when any user)
  • <my-test-url>/weaver/weaver/api -> magpie permission rejected as the anonymous user (and same nginx error above when any user)

@mishaschwartz
Copy link
Collaborator

For some reason, I could never get this variable to be detected (although set), and although everything else works fine:

Do you mean it doesn't show up in the templated file? Or that alertmanager doesn't send emails to that address but everything else works ok.

Can you copy here any error logs from the alertmanager or proxy service and also the part of your env.local file where you're overriding it?

@fmigneault
Copy link
Collaborator Author

Do you mean it doesn't show up in the templated file?

It doesn't reach that point. The birdhouse script complains that the required variable is "missing", although it is exported explicitly in the env.local file that it sources. The same file contains many variables that override various items, so I know it gets handled "the usual way", except for that specific case somehow.

@mishaschwartz
Copy link
Collaborator

It doesn't reach that point. The birdhouse script complains that the required variable is "missing"

When I tested out 2.7.0 + this PR (with the ALERTMANAGER_ADMIN_EMAIL_RECEIVER set in env.local and the monitoring component enabled) everything starts up OK for me.

Are you absolutely sure that there's no typo in the ALERTMANAGER_ADMIN_EMAIL_RECEIVER in env.local? Or are you unsetting it later in env.local somewhere?

@fmigneault
Copy link
Collaborator Author

I restarted a fresh terminal, removed/re-added the component, and then no more issue about ALERTMANAGER_ADMIN_EMAIL_RECEIVER 🤷‍♂️

Copy link
Collaborator

@mishaschwartz mishaschwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good except that the weaver/post-docker-compose-up script fails to register external WPS providers with the current configuration. See the changes below for a suggested fix.

Note that we only need to change 301 to 308 in one case since I don't believe that weaver supports POST requests for the root path

location = ${TWITCHER_PROTECTED_PATH}/${WEAVER_MANAGER_NAME} {
return 301 /${WEAVER_MANAGER_NAME}/$is_args$args;
}
location ${TWITCHER_PROTECTED_PATH}/${WEAVER_MANAGER_NAME}/ {
Copy link
Collaborator

@mishaschwartz mishaschwartz Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
location ${TWITCHER_PROTECTED_PATH}/${WEAVER_MANAGER_NAME}/ {
location ~ ^${TWITCHER_PROTECTED_PATH}/${WEAVER_MANAGER_NAME}/(.*)$ {

We need to match something in order to include the subpath in the redirect (see next comment)

return 301 /${WEAVER_MANAGER_NAME}/$is_args$args;
}
location ${TWITCHER_PROTECTED_PATH}/${WEAVER_MANAGER_NAME}/ {
return 301 /${WEAVER_MANAGER_NAME}/$is_args$args;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return 301 /${WEAVER_MANAGER_NAME}/$is_args$args;
return 308 /${WEAVER_MANAGER_NAME}/$1$is_args$args;

Actually include the matched subpath in the redirect.

Also, this should be 308 so that clients don't change a POST request to GET when performing the redirect.

(This was discovered when running the weaver/post-docker-compose-up script which still uses the twitcher proxy routes)

Copy link
Collaborator

@tlvu tlvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Misha is more knowledgeable than me with Weaver. I'll let him review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/magpie Related to https://github.com/Ouranosinc/Magpie component/weaver Related to https://github.com/crim-ca/weaver documentation Improvements or additions to documentation feature/WPS Feature or service related to Web Processing Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants