Repository navigation
Conversation
…into prototype/remove-ambassador-proposed-fixes
…github.com/helxplatform/helx-chart into prototype/remove-ambassador-proposed-fixes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When Ambassador was removed there were a few things that weren't addressed. I looked at the PR myself and found some code smells, so I ran it through Sol and it identified a good many issues. I fixed the following:
Critical:
charts/resty/templates/nginx-default-configmap.yamlwas defaulting toappstore.<namespace>.svc.cluster.local, but the appstore chart renders<release-name>-appstore. This would cause there to be no connectivity to the appstore service.High
DEV_PHASE.dev=true,charts/resty/templates/nginx-default-configmap.yaml:123skips the Lua block assigning$upstreamand$upstream_uri, but line 200 still executesproxy_pass http://$upstream$upstream_uri. Every request withDEV_PHASE.dev=truetherefore fails. This is ultimately because Ambassador was taking care of this work, and now nothing is. I'm not fixing it right now because it's a larger undertaking, but we need to fix this soon so that we can run appstore locally. My recommendation is to use resty itself as the reverse proxy locally and maybe just bundle resty and appstore into a docker-compose file that we can easily run to stand up appstore locally, but that's just a thought.charts/resty/templates/nginx-default-configmap.yamlpropagates only the resolver’s status withngx.exit(res.status). Appstore’s resolver is protected by Djangologin_required, so an expired session returns a 302 with a loginLocation, but the captured response headers are discarded. The browser receives a bare 302 instead of being redirected to login. I forwarded theLocationheader through.charts/resty/values.yamlusesSSL.restyTLSSecretwhile the templates useSSL.nginxTLSSecret. Fixed that.charts/resty/templates/nginx-deploy.yamlwas mounting certs under/opt/bitnami/openresty/nginx/conf/certsbutcharts/resty/templates/nginx-default-configmap.yamlwas reading them from/etc/nginx/ssl/tls.crtand/etc/nginx/ssl/tls.key. Fixed that.charts/resty/templates/nginx-default-configmap.yamlremoves the authority from every HTTP(S) URL, despite the comment saying “same-origin.” For example,Location: https://idp.example/loginbecomes/private/.../login, breaking OAuth/SSO and intentional external redirects. Now it only strips the authority after confirming it matches the external request host.Medium
charts/resty/templates/nginx-default-configmap.yamlusedglobal.apps_namespacefor appstore, UI, sockets, and Airflow as well as launched apps. The value was documented as the namespace where Tycho launches applications. Setting it to a separate workspace namespace therefore points the entire site at nonexistent Services. I ended up just removingappsNamespaceentirely and assuming that HeLx is deployed in one single namespace for now. If we want to extend HeLx to being able to be deployed in multiple namespaces, I think that should be a concerted effort later.Low
NOTES.txtfile hadn't been updated now that we use resty instead of nginx by default, so I went ahead and updated that since it took 2 seconds.