Skip to content

Use upstream access control headers - #386

Merged
natefoo merged 1 commit into
galaxyproject:mainfrom
mvdbeek:upstream_acao_headers
Mar 26, 2024
Merged

Use upstream access control headers#386
natefoo merged 1 commit into
galaxyproject:mainfrom
mvdbeek:upstream_acao_headers

Conversation

@mvdbeek

@mvdbeek mvdbeek commented Mar 26, 2024

Copy link
Copy Markdown
Member

No description provided.

Comment on lines 150 to 151
#add_header Access-Control-Allow-Origin "http://registry.intermine.org";
if ($http_origin ~ "^https?://(covid19\.galaxyproject\.org|registry\.intermine\.org)$") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this still necessary then?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Probably not, but there are additional directives I'm not setting ... we can try this on test first.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(i mean Expose-Headers and Allow-Headers)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can try this on test first.

sounds good. (i've linked this thread from the related EU playbook change, so, they'll surely check here for the conclusion.)

mira-miracoli added a commit to mira-miracoli/infrastructure-playbook that referenced this pull request Mar 26, 2024
@mira-miracoli

Copy link
Copy Markdown

This should also work with a second proxy (traefik) load-balancing multiple servers running nginx right?

@mvdbeek

mvdbeek commented Mar 26, 2024

Copy link
Copy Markdown
Member Author

As long as the upstream doesn't drop any response headers it should work, yes.

@natefoo natefoo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM and much nicer than what we were doing.

@natefoo
natefoo merged commit 4f7c4d5 into galaxyproject:main Mar 26, 2024
@sanjaysrikakulam

sanjaysrikakulam commented Mar 28, 2024

Copy link
Copy Markdown

Can anyone please confirm that we can remove the CORS?

@sanjaysrikakulam

Copy link
Copy Markdown

Can anyone please confirm that this works and we can remove the CORS from our config?

@mvdbeek

mvdbeek commented May 28, 2024

Copy link
Copy Markdown
Member Author

How can we tell ? I know avivator works but I don't know why those other headers were set. You can try this yourself and report if anything doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants