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 AlwaysUseProxyPublicAddr to app spec protos #52693

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

avatus
Copy link
Contributor

@avatus avatus commented Mar 3, 2025

This adds a new field to the app, AlwaysUseProxyPublicAddr, that will be used to tell the web api/ui to rebuild the requested app's FQDN based on the proxy public addr that the request came from (upcoming PR to consume this)

Will backport the full feature (this plus next PR) all together

@avatus avatus added the no-changelog Indicates that a PR does not require a changelog entry label Mar 3, 2025
Comment on lines 1906 to 1909
if application.AlwaysUseProxyPublicAddr && application.PublicAddr != "" {
return trace.BadParameter("public_addr cannot be explicitly set if always_use_proxy_public_addr is set to true")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this isn't necessarily required but I decided to do it this way since the public_addr is an optional field. If a customer explicitly sets the public addr, i don't think overriding it with this field makes a bunch of sense. I also wanted this field to be implied based on the non-existence of the public_addr instead of set but, its better to be explicit with both

@avatus avatus force-pushed the avatus/use_proxy_addr branch from 60a6439 to 86e0fac Compare March 3, 2025 15:57
Copy link

github-actions bot commented Mar 3, 2025

Amplify deployment status

Branch Commit Job ID Status Preview Updated (UTC)
avatus/use_proxy_addr d317cc6 6 ✅SUCCEED avatus-use-proxy-addr 2025-03-05 19:23:16

Comment on lines 1906 to 1908
if application.AlwaysUseProxyPublicAddr && application.PublicAddr != "" {
return trace.BadParameter("public_addr cannot be explicitly set if always_use_proxy_public_addr is set to true")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be done in CheckAndSetDefaults instead?

I don't think this check should only be applied to Apps coming from a configuration file.
We also have:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. however, for file config, the public addr is derived before CheckAndSetDefaults
so the publicAddr would already be set implicitly before the check making clusters unable to start.

i would be ok with removing the check all together and just having some really good documentation on this field saying it will overwrite any set public addr (since they are both optional fields). what are your thoughts?

Comment on lines 1066 to 1070
// AlwaysUseProxyPublicAddr will rebuild this app's fqdn based on the proxy public addr that the
// request originated from.
bool AlwaysUseProxyPublicAddr = 14 [(gogoproto.jsontag) = "always_use_proxy_public_addr,omitempty"];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we expand on this? When should this be used vs using a custom public addr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i can update it for sure.

essentially, the apps public addr, if not set explicitly, is appName.proxyPublicAddr[0]. however, we allow multiple publicAddr for proxies and we would like to be able to access apps from any configured proxy public addr.

for the web UI (which the follow up PR targets), we can use the apps publicAddr/fqdn to authenticate the app. This doesn't work if the user is authenticated to the proxy of a different publicAddr.

so instead, what we want to do is something like make the apps fqdn be "appName.WhateverProxyPublicAddrTheRequestCameFrom". this AlwaysUseProxyPublicAddr field is a way for us to differentiate between using the public addr that was implicitly set (proxyPublicAddr[0]) and one that can be crafted per request (proxyPublicAddr[x])

My original plan was to just not set the proxyPublicAddr implicitly and do someting like "if not set, just use the proxy public addr from request", but that wouldn't work because there are a lot of other places the code base expects this field to exist. so instead, i added AlwaysUseProxyPublicAddr as an optional field to "override" this implicit publicAddr.

Now, I just have to distill all of that into a godoc comment lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i actually think itd be better to remove this check and just add docs to this field and to publicAddr saying if "if always_use_proxy_public_addr" is set, it will use that to build the apps FQDN for the web UI.

it makes it explicit what is happening in the background, and wont break/change any existing configs unless they want to use this new field (which they would have to read the docs for anyway)

@avatus avatus force-pushed the avatus/use_proxy_addr branch from ab39323 to 6e528e2 Compare March 5, 2025 18:51
@avatus avatus force-pushed the avatus/use_proxy_addr branch from 6e528e2 to 2715530 Compare March 5, 2025 19:00
@avatus avatus force-pushed the avatus/use_proxy_addr branch from 2715530 to 2fa7b9d Compare March 5, 2025 19:07
This adds a new field, AlwaysUseProxyPublicAddr, that will be used to
tell the web api/ui to rebuild the request app's FQDN based on the proxy
public addr that the request came from (upcoming PR to consume this)
@avatus avatus force-pushed the avatus/use_proxy_addr branch from 2fa7b9d to d317cc6 Compare March 5, 2025 19:17
@avatus avatus requested a review from marcoandredinis March 5, 2025 19:27
Copy link
Contributor

@marcoandredinis marcoandredinis left a comment

Choose a reason for hiding this comment

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

The name is not very explicit about its implications, but the comment is super clear, and I don't have a better suggestion 😅

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants