-
Notifications
You must be signed in to change notification settings - Fork 92
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
enhance nginx auth sidecar config #502
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Needs tests.
assert authSidecarServicePorts in docs[1]["spec"]["ports"] | ||
|
||
nginx_conf = pathlib.Path( | ||
"tests/chart/test_data/dag-server-authsidecar-nginx.conf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to start doing this kind of thing, we'll need to come up with a way to organize these files better by naming them after the function that uses them, or by their content hash. We don't need to solve this now if we don't want to, but if we don't solve it now we'll have to solve it as soon as we start adding more files.
location = /auth { | ||
{{ include "default_nginx_settings" . | indent 8 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this isn't nindent of a data structure, should we just indent it so it aligns with the code, rather than calling | indent 8
?
{{ include "default_nginx_settings" . | indent 8 }} | |
{{ include "default_nginx_server_settings" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. I left some optional code suggestions, and also a note about thinking more about the test data naming convention.
Co-authored-by: Daniel Hoherd <danielh@astronomer.io>
Co-authored-by: Daniel Hoherd <danielh@astronomer.io>
Co-authored-by: Daniel Hoherd <danielh@astronomer.io>
tests failing |
* enhance nginx auth sidecar config * only connect ipv4 * update static test cases for nginx * Update templates/webserver/webserver-auth-sidecar-configmap.yaml Co-authored-by: Daniel Hoherd <danielh@astronomer.io> * Update templates/flower/flower-auth-sidecar-configmap.yaml Co-authored-by: Daniel Hoherd <danielh@astronomer.io> * Update templates/dag-deploy/dag-server-auth-sidecar-configmap.yaml Co-authored-by: Daniel Hoherd <danielh@astronomer.io> * update nginx template --------- Co-authored-by: Daniel Hoherd <danielh@astronomer.io>
Description
This PR enhances existing authsidecar config and fixes code duplication.
change 1: client_max_body_size initially nested inside auth block its now moved to server block to get applied for all location
change 2: server_tokens is added every where in auth-sidecar nginx block now its moved to common location and called every to reduce code duplication
change 3: localhost in upstream block connects ipv6 by default and falls back to ipv4 this was causing error in first try and fallbacks to ipv4 and call passed sucessfully - we have changed to 127.0.0.1 to connect ipv4 by default
Related Issues
https://github.com/astronomer/issues/issues/6141
Testing
QA should validate webserver, flower and dag-server to see services comes up without any issues.
This also now enhances our upload limit for all auth proxy components
Merging
cherry-pick to release-1.10, master and 1.11