Rebuild app fqdn based on inferred proxy public addr #52718

avatus · 2025-03-03T18:30:20Z

This allows web apps to be launched and authenticated from any registered proxy public addr. The web launcher will use the fqdn-only launch path for apps with this flag (AlwaysUseProxyPublicAddr).

Requires #52693

to test:
in your proxy config, add a couple different public_addr. make sure your certs are valid for these domains

you can add always_use_proxy_public_addr in your app spec. in the web UI, you should now see something like "appName.proxyPublicAddrYouAreOn" in the webUI on the app's card. this value should change depending on what proxy public addr you are on. also, when you authenticate the app, the destination should match that public addr. apps without these field set should work as expected (show the first public addr regardless of what proxy public addr youre on)

flyinghermit · 2025-03-04T03:13:24Z

lib/utils/net.go

+// InferProxyPublicAddr tries to extract a proxyDNSName from a given fqdn and the list of proxy public addrs.
+// If a proxyDNS name is not found, the default param will be returned.
+func InferProxyPublicAddr(fqdn string, proxyDNSNames []string, defaultPublicAddr string) string {
+	if fqdn == "" || defaultPublicAddr == "" {
+		return defaultPublicAddr
+	}


Two questions:

What is the source of defaultPublicAddr? Does it come from app config spec or is calculated from some other values?

What are the implication of returning an empty string? Should we return an error here if the value is empty?

the source is at the callsite. its up to the caller to pass in a default value, and if they pass in an empty string, it would be because they want to default to empty if the proxyDNSName cannot be inferred (i dont have a use case for it, but i dont think an error should be called here)

however, i wont die on this hill. tbh, the signature without any error is easier to deal with where this is called but, thats my only excuse

Got it. From the name of the function, it seems like caller depends on the correct address to be inferred. As such, I would expect it to be explicitly return an inferred value or an error if it cant.

Alternatively, if we are sure that the empty return value is handeld at every call site, then maybe just explaining that in the comment would be ok too.

Also please double check that if the we expect public_addr field from app spec to be not empty. I did a quick look and only found the value is checked to be a valid fqdn. I think whatever value this function returns, we should ensure the value is verified exactly as we do for the public_addr field.

lib/web/apps.go

flyinghermit · 2025-03-04T03:22:33Z

lib/web/ui/app.go

+		RequiresRequest:          c.RequiresRequest,
+		Integration:              app.GetIntegration(),
+		PermissionSets:           permissionSets,
+		AlwaysUseProxyPublicAddr: app.GetAlwaysUseProxyPublicAddr(),


suggestion for the field name: useProxyPublicAddrAsFQDN so it conveys what the proxy public addr will be used for. Also a Godoc to explain that please.

its not quite as simple as that name suggests, but i do agree my name isnt the best either.
whats actually happening is
"UseProxyPublicAddrFromThisRequestPrefixedWithAppNameAsFQDN" lol but that doesnt flow off the tongue

right now, our proxy allows multiple configured publicAddr, but for apps, unless they have a publicaddr specified explicitly, the apps public addr is just "appName.proxyPublicAddrs[0]".

however, this means if someone wants to access the app from some other proxyPublicAddr, they would have to then get redirected and authenticated with that publicaddr and all that yada yada yada.

I think I'll keep this field name but i will definitely expand upon the godoc for sure, also see here #52693 for more convo. maybe the field name will change there as well

I think it is "always" that makes the name a bit controversial lol. Still think useProxyPublicAddrAsFQDN better suits here but I will let you decide on that. We add app name as prefix to that is an implementation detailed explained in a comment should suffice.

This allows web apps to be launched and authenticated from any registered proxy public addr. The web launcher will use the fqdn-only launch path for apps with this flag (AlwaysUseProxyPublicAddr).

avatus · 2025-03-05T19:42:02Z

added steps to test in the description

flyinghermit

Looks good overall

flyinghermit · 2025-03-06T16:15:27Z

lib/utils/net.go

@@ -35,3 +37,23 @@ func ClientIPFromConn(conn net.Conn) (string, error) {

 	return clientIP, nil
 }
+
+// InferProxyPublicAddr tries to extract a proxyDNSName from a given fqdn and the list of proxy public addrs.


Suggested change

// InferProxyPublicAddr tries to extract a proxyDNSName from a given fqdn and the list of proxy public addrs.

// InferProxyPublicAddr tries to extract a proxyDNSName from a given app FQDN and the list of proxy public addrs.

I think this comment also warrant some more explanation on what each params field expects and how should the caller treat empty string return value.

flyinghermit · 2025-03-06T16:28:31Z

lib/utils/net.go

+// InferProxyPublicAddr tries to extract a proxyDNSName from a given fqdn and the list of proxy public addrs.
+// If a proxyDNS name is not found, the default param will be returned.
+func InferProxyPublicAddr(fqdn string, proxyDNSNames []string, defaultPublicAddr string) string {
+	if fqdn == "" || defaultPublicAddr == "" {
+		return defaultPublicAddr
+	}


Got it. From the name of the function, it seems like caller depends on the correct address to be inferred. As such, I would expect it to be explicitly return an inferred value or an error if it cant.

Alternatively, if we are sure that the empty return value is handeld at every call site, then maybe just explaining that in the comment would be ok too.

flyinghermit · 2025-03-06T16:37:08Z

lib/utils/net_test.go

+			name:              "Exact match",
+			fqdn:              "proxy.example.com",
+			proxyDNSNames:     []string{"proxy.example.com"},
+			defaultPublicAddr: "default",


what is defaultPublicAddr. Is it the public_addr value from app spec?

Also can we use fqdn value instead of "default" string.

flyinghermit · 2025-03-06T16:43:46Z

lib/web/ui/app.go

+		RequiresRequest:          c.RequiresRequest,
+		Integration:              app.GetIntegration(),
+		PermissionSets:           permissionSets,
+		AlwaysUseProxyPublicAddr: app.GetAlwaysUseProxyPublicAddr(),


I think it is "always" that makes the name a bit controversial lol. Still think useProxyPublicAddrAsFQDN better suits here but I will let you decide on that. We add app name as prefix to that is an implementation detailed explained in a comment should suffice.

flyinghermit · 2025-03-06T16:58:44Z

lib/utils/net.go

+// InferProxyPublicAddr tries to extract a proxyDNSName from a given fqdn and the list of proxy public addrs.
+// If a proxyDNS name is not found, the default param will be returned.
+func InferProxyPublicAddr(fqdn string, proxyDNSNames []string, defaultPublicAddr string) string {
+	if fqdn == "" || defaultPublicAddr == "" {
+		return defaultPublicAddr
+	}


Also please double check that if the we expect public_addr field from app spec to be not empty. I did a quick look and only found the value is checked to be a valid fqdn. I think whatever value this function returns, we should ensure the value is verified exactly as we do for the public_addr field.

flyinghermit · 2025-03-06T17:06:28Z

lib/web/ui/app.go

@@ -121,7 +128,9 @@ type MakeAppsConfig struct {
 // MakeApp creates an application object for the WebUI.
 func MakeApp(app types.Application, c MakeAppsConfig) App {
 	labels := ui.MakeLabelsWithoutInternalPrefixes(app.GetAllLabels())
-	fqdn := utils.AssembleAppFQDN(c.LocalClusterName, c.LocalProxyDNSName, c.AppClusterName, app)
+	proxyPublicAddr := utils.InferProxyPublicAddr(c.RequestHost, c.ProxyPublicAddrs, c.LocalProxyDNSName)


In this case, the value of c.RequestHost should always match one of the value in the ProxyPublicAddrs right? cause this request handled in the proxy itself.

Is it that if AlwaysUseProxyPublicAddr is true, can we directly use proxyPublicAddr = c.RequestHost ?

flyinghermit · 2025-03-06T17:09:53Z

lib/web/ui/app.go

-	fqdn := utils.AssembleAppFQDN(c.LocalClusterName, c.LocalProxyDNSName, c.AppClusterName, app)
+	proxyPublicAddr := utils.InferProxyPublicAddr(c.RequestHost, c.ProxyPublicAddrs, c.LocalProxyDNSName)
+
+	fqdn := utils.AssembleAppFQDN(c.LocalClusterName, proxyPublicAddr, c.AppClusterName, app)


Can we update TestMakeApps to cover that fqdn is derived as expected when AlwaysUseProxyPublicAddr is true/false?

avatus added the no-changelog Indicates that a PR does not require a changelog entry label Mar 3, 2025

avatus requested review from flyinghermit and kimlisa March 3, 2025 18:31

avatus marked this pull request as ready for review March 3, 2025 18:48

github-actions bot added size/md ui labels Mar 3, 2025

avatus marked this pull request as draft March 3, 2025 18:59

avatus force-pushed the avatus/web_proxy_addr branch from 62b3472 to 2d56b07 Compare March 3, 2025 19:28

avatus marked this pull request as ready for review March 4, 2025 00:18

github-actions bot requested review from gzdunek and ravicious March 4, 2025 00:18

flyinghermit reviewed Mar 4, 2025

View reviewed changes

avatus mentioned this pull request Mar 5, 2025

Add AlwaysUseProxyPublicAddr to app spec protos #52693

Open

avatus removed request for ravicious and gzdunek March 5, 2025 18:04

avatus force-pushed the avatus/use_proxy_addr branch from ab39323 to 6e528e2 Compare March 5, 2025 18:51

avatus force-pushed the avatus/web_proxy_addr branch from 2d56b07 to 34acece Compare March 5, 2025 18:54

avatus force-pushed the avatus/use_proxy_addr branch from 6e528e2 to 2715530 Compare March 5, 2025 19:00

avatus force-pushed the avatus/web_proxy_addr branch from 34acece to abcfcd8 Compare March 5, 2025 19:01

avatus force-pushed the avatus/use_proxy_addr branch from 2715530 to 2fa7b9d Compare March 5, 2025 19:07

avatus force-pushed the avatus/web_proxy_addr branch from abcfcd8 to 8660d82 Compare March 5, 2025 19:08

avatus force-pushed the avatus/use_proxy_addr branch from 2fa7b9d to d317cc6 Compare March 5, 2025 19:17

Rebuild app fqdn based on inferred proxy public addr

7cfc788

This allows web apps to be launched and authenticated from any registered proxy public addr. The web launcher will use the fqdn-only launch path for apps with this flag (AlwaysUseProxyPublicAddr).

avatus force-pushed the avatus/web_proxy_addr branch from 8660d82 to 7cfc788 Compare March 5, 2025 19:19

Removehostname

6c06747

avatus requested a review from flyinghermit March 5, 2025 19:27

Fix test

73ea200

flyinghermit reviewed Mar 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebuild app fqdn based on inferred proxy public addr #52718

Rebuild app fqdn based on inferred proxy public addr #52718

avatus commented Mar 3, 2025 •

edited

Loading

flyinghermit Mar 4, 2025

avatus Mar 5, 2025

flyinghermit Mar 6, 2025

flyinghermit Mar 6, 2025

flyinghermit Mar 4, 2025 •

edited

Loading

avatus Mar 5, 2025

flyinghermit Mar 6, 2025

avatus commented Mar 5, 2025

flyinghermit left a comment

flyinghermit Mar 6, 2025

flyinghermit Mar 6, 2025

flyinghermit Mar 6, 2025

flyinghermit Mar 6, 2025

flyinghermit Mar 6, 2025

flyinghermit Mar 6, 2025

flyinghermit Mar 6, 2025

	// InferProxyPublicAddr tries to extract a proxyDNSName from a given fqdn and the list of proxy public addrs.
	// InferProxyPublicAddr tries to extract a proxyDNSName from a given app FQDN and the list of proxy public addrs.

Rebuild app fqdn based on inferred proxy public addr #52718

Are you sure you want to change the base?

Rebuild app fqdn based on inferred proxy public addr #52718

Conversation

avatus commented Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyinghermit Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avatus commented Mar 5, 2025

flyinghermit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avatus commented Mar 3, 2025 •

edited

Loading

flyinghermit Mar 4, 2025 •

edited

Loading