Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

feat(appliance): implement re-direct on all unknown endpoints#64059

Merged
craigfurman merged 2 commits into
mainfrom
al/REL-78/nginx-redirect
Jul 25, 2024
Merged

feat(appliance): implement re-direct on all unknown endpoints#64059
craigfurman merged 2 commits into
mainfrom
al/REL-78/nginx-redirect

Conversation

@Chickensoupwithrice

@Chickensoupwithrice Chickensoupwithrice commented Jul 25, 2024

Copy link
Copy Markdown
Contributor

Resolves request 4 of REL-78

Test plan

Hopes and Prayers 🙏

Changelog

  • feat(appliance): implement re-direct on all unknown endpoints

@craigfurman craigfurman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't appear to work, but I wasn't able to get it working either 🤔

My progress is on top of this branch, in https://github.com/sourcegraph/sourcegraph/commit/469da79a5d89d39989c5ab55d7924d9224c7acdd. Please see commit message too.

I was testing this with:

docker rmi appliance-frontend:candidate
sg bazel run //docker-images/appliance-frontend:image_tarball
docker run -e API_ENDPOINT=http://localhost:8888 --platform linux/amd64 --rm -p 80:80 appliance-frontend:candidate

And navigating to localhost in my browser.

@craigfurman

Copy link
Copy Markdown
Contributor

new hash, who dis: https://github.com/sourcegraph/sourcegraph/commit/2ce7687062bf6d8e113548c95a5d6317d68c7b68

@jdpleiness and I were pairing and managed to get this working 😱 🎉 🙌 🍻

I reckon we can start merging this 3-stack (2 PRs and my branch, which maybe we merge into here), and roll forward with any bug fixes. It's hard to test this e2e because the base of the basest branch is so old, that the frontend and backend have changed a lot.

Base automatically changed from nelsona/appliance/maint-wolfi to main July 25, 2024 14:33
Chickensoupwithrice and others added 2 commits July 25, 2024 15:35
- Disable access logs. AFAICT these must be files, rather than
  references to stdout/stderr (I couldn't get that working). Since we'll
  be running in Kubernetes, we'd have to add a mechanism to pipe that
  file to stdout so that it shows up in logs, which doesn't seem worth
  it. Or bind-mount the file to a shared volume and use fluentd or
  similar to pipe it out, which also doesn't seem worth it.
- Change error log to stderr. Kubernetes containers won't have
  permission to open /dev/console, and neither did my local docker
  setup. Using stderr means that these logs will be visible in
  kubectl-logs too.
- Fix typo in "maintenance" for redirect
- Use 302 instead of 301. The browser 301 cache needs to be specifically
  cleared, which makes iteration tricky. We are not bottlenecked on the
  latency of the user's 301 cache anyway :)
- Add $server_port to redirect. Indeed, if we bind port 80 this is a
  no-op, but it might avoid confusing bugs in the future.
- I think we had a redirect loop, which I've tried to fix with some ugly
  nginx logic (turns out it doesn't have and/or, and nested ifs cause
  warnings in linters I found). I managed to stop the loop, and I think
  the redirect logic works now...
@craigfurman craigfurman force-pushed the al/REL-78/nginx-redirect branch from 718c054 to 6ba2a4d Compare July 25, 2024 14:35

@craigfurman craigfurman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Chickensoupwithrice apologies for appropriating this PR, @jdpleiness and I are hacking in an earlier time zone. Holler if our changes are bogus and we can roll forward with fixes together

@craigfurman craigfurman enabled auto-merge (squash) July 25, 2024 14:36
@craigfurman craigfurman merged commit 9f4c160 into main Jul 25, 2024
@craigfurman craigfurman deleted the al/REL-78/nginx-redirect branch July 25, 2024 14:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants