Skip to content

Prototype/remove ambassador proposed fixes - #118

Open
Hoid wants to merge 11 commits into
developfrom
prototype/remove-ambassador-proposed-fixes
Open

Hoid wants to merge 11 commits into
developfrom
prototype/remove-ambassador-proposed-fixes

Conversation

@Hoid

@Hoid Hoid commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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:

  • The appstore service name in charts/resty/templates/nginx-default-configmap.yaml was defaulting to appstore.<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

  • UNFIXED: With DEV_PHASE.dev=true, charts/resty/templates/nginx-default-configmap.yaml:123 skips the Lua block assigning $upstream and $upstream_uri, but line 200 still executes proxy_pass http://$upstream$upstream_uri. Every request with DEV_PHASE.dev=true therefore 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.yaml propagates only the resolver’s status with ngx.exit(res.status). Appstore’s resolver is protected by Django login_required, so an expired session returns a 302 with a login Location, but the captured response headers are discarded. The browser receives a bare 302 instead of being redirected to login. I forwarded the Location header through.
  • charts/resty/values.yaml uses SSL.restyTLSSecret while the templates use SSL.nginxTLSSecret. Fixed that.
  • charts/resty/templates/nginx-deploy.yaml was mounting certs under /opt/bitnami/openresty/nginx/conf/certs but charts/resty/templates/nginx-default-configmap.yaml was reading them from /etc/nginx/ssl/tls.crt and /etc/nginx/ssl/tls.key. Fixed that.
  • charts/resty/templates/nginx-default-configmap.yaml removes the authority from every HTTP(S) URL, despite the comment saying “same-origin.” For example, Location: https://idp.example/login becomes /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.yaml used global.apps_namespace for 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 removing appsNamespace entirely 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

  • The NOTES.txt file 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.

@Hoid
Hoid requested a review from joshua-seals August 10, 2026 15:49
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.

1 participant