Skip to content
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

Add a local dynamic proxy server config #775

Merged
merged 4 commits into from
Jan 3, 2025
Merged

Add a local dynamic proxy server config #775

merged 4 commits into from
Jan 3, 2025

Conversation

njegosrailic
Copy link
Contributor

@njegosrailic njegosrailic commented Dec 30, 2024

What

  • Added a local dynamic proxy server configuration.
  • Renamed some input parameters to avoid confusion.
  • Added extra input parameters.
  • Added validation for all required environment variables (ENVs) in the Envoy template, as envsub does not fail. Clear and concise error messages will be displayed if something is missing.
  • Modified the "Verify Envoy Proxy" step to support multiple listeners.
  • Modified the LUA script to allow specifying the JWT header name instead of using a hardcoded name. This was a bug.

Why

The primary goal is to set up a local HTTPS proxy to proxy all requests and dynamically append the GitHub OIDC JWT token to a specific header. This will simplify the process, as most languages and software respect the HTTPS_PROXY ENV variable, and no need to modify the app code to fetch and append the header.

However, we may encounter other issues, such as SSL certificate validation. The local CA used to issue a self-signed TLS certificate should already be added to the main CA chain, but there may be other challenges. This is just the initial version, so please bear with me.

Once we configure this ENV, all requests will be sent through the dynamic forward proxy, so we need to be cautious, as we will proxy all app requests. We must ensure that the JWT token is not added to every request (for example other public domains). This is why I've added the main-dns-zone input. We can discuss this internally - perhaps we should just add a zone, and developers can use prefixes for service names. (We already have the DNS zone name in the K8s API hostname, so what I'm proposing is having just one secret for the DNS zone and using prefixes.)

During debugging, I noticed that missing ENVs can cause Envoy to crash, making it difficult to identify issues. By explicitly validating all required ENVs, it could save time in the future.

One more thing: I know that the LUA script is almost identical for both listeners, resulting in duplication, and it could be moved to a separate file and loaded. However, there are some loops involved in request processing, and I'd prefer to avoid altering that for now. We can improve this later, especially since I don't expect us to add more listeners anytime soon.

@njegosrailic njegosrailic marked this pull request as ready for review December 30, 2024 20:14
@njegosrailic njegosrailic requested a review from a team as a code owner December 30, 2024 20:14
Copy link
Contributor

@chainchad chainchad left a comment

Choose a reason for hiding this comment

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

Looks great, just a spelling thing and a question.

actions/setup-gap/action.yml Outdated Show resolved Hide resolved
actions/setup-gap/envoy.yaml.template Outdated Show resolved Hide resolved
njegosrailic and others added 2 commits December 31, 2024 12:41
Co-authored-by: chainchad <96362174+chainchad@users.noreply.github.com>
@njegosrailic njegosrailic enabled auto-merge (squash) January 3, 2025 14:35
@njegosrailic njegosrailic merged commit 0cb7aaa into main Jan 3, 2025
11 checks passed
@njegosrailic njegosrailic deleted the local-proxy branch January 3, 2025 14:37
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.

2 participants