fix: base href does not work #11401 #10194 #2134

[sonbui00]

Fix #11401
Fix #10194
Fix #11643
Fix #7767
Related #2134

Motivation

Support BASE_HREF on the document https://argoproj.github.io/argo-workflows/argo-server/

Modifications

Add handle base href for static file and test

Verification

make test

More

As I check this issue, I see the lib to embed static files into Go binary has been deprecated (https://github.com/bouk/staticfiles). We should replace it.

[agilgur5]

Hmm I need to take a look at this more closely, but this looks more like partial rootpath support rather than base href from a glance.

With just base href, the proxy in front should be rewriting these subpaths. The Argo Server still sees all requests from /. Or, effectively, base href primarily impacts links, such as those on the front-end

[sonbui00]

@agilgur5 what "partial rootpath support" mean?

With just base href, the proxy in front should be rewriting these subpaths

=> Can you give me an example?

The Argo Server still sees all requests from /

=> I tried to keep this compatible with other functions, ex: readiness check

[agilgur5]

@agilgur5 what "partial rootpath support" mean?

As in a partial implementation of #7767.

=> Can you give me an example?

#3080 has many examples of ingress config with rewrites. This one from the top is pretty concise and uses the common NGINX IGC

[sonbui00]

@agilgur5 I'm sorry, but I do not understand. Let's me know if you want any info here

[agilgur5]

@agilgur5 I'm sorry, but I do not understand.

Let me try two other explanations:

1. <base> is an HTML element. Per the MDN docs, the href attribute only affects HTML links. It does not change anything on the back-end.

   • With a Single Page App (SPA) on the front-end, JS links need to be changed in addition to HTML. Hence the JS I linked before.

2. /argotest/main.js should never occur. The proxy in front of the server is supposed to rewrite that to /main.js. The server only sees /main.js and never sees /argotest/main.js because there is a proxy in front.

   • Without rootpath support (i.e. Implement rootpath similar to argo-cd #7767), the back-end basically has no knowledge of the rewrite. Rootpath support is similar to implementing the rewrite within Argo.

[agilgur5]

I don't think this is actionable as a "fix" to base href. base href is working correctly.

This could be used as part of a feature to implement rootpath (#7767), but that should probably be done as one larger PR

[sonbui00]

I don't think this is actionable as a "fix" to base href. base href is working correctly.

Can you make a test? I don't think it work like the document and I also made some test before. That's why many people report this in some issues. Also you can read the fix code to know the reason

[sonbui00]

@agilgur5 with ingress rewrite, I think this is not a issue. I tried without ingress rewrite for this

[agilgur5]

Can you make a test?

The back-end is working correctly. Per #11401 (comment), the Server logs are correct, correctly detecting the environment variable. Per #11643 (comment), the Server correctly sets <base href="/argo/">.

That is the only impact to the back-end that base href has. All other impacts are front-end facing.

That's why many people report this in some issues

Per above, both users' own issues show that it is working correctly.
Per #10194 (comment), I think many users are misinterpreting the base href feature for rootpath, but those are two separate features.
Base href is for the front-end, rootpath is for the back-end.

[agilgur5]

@agilgur5 with ingress rewrite, I think this is not a issue. I tried without ingress rewrite for this

Yes exactly! Base href is intended to be used in conjunction to an ingress rewrite.

The docs do show using both together. They've received a few PRs in the past too to try and clarify that for users.

[sonbui00]

Yes exactly! Base href is intended to be used in conjunction to an ingress rewrite

@agilgur5 Maybe ingress rewrite added to the document after have this issue. This fix can run without ingress rewrite. The document updated after this issue #3080

[agilgur5]

This fix can run without ingress rewrite.

That's just a separate feature entirely. That is part of rootpath support #7767. And totally feel free to work on #7767!

Base href and rootpath are just two separate (but related) features. Note that the opening comment of #7767 uses both --basehref and --rootpath together.

"Base href" specifically is a front-end only feature designed to be used with ingress rewrites. It is making the front-end aware of an existing back-end configuration. The back-end configuration can be done either with an ingress rewrite or with --rootpath.

The document updated after this issue #3080

Yea that is specifically because people misinterpreted base href to mean rootpath, but they are not identical features. So that was a clarification that base href should not be used by itself alone -- it should be used with a similar back-end configuration.
As Alex mentioned in #3080 (comment), it is a common configuration, but also unfortunately commonly confused.

[sonbui00]

@agilgur5
I think in this project basehref and rootpath is the same. You will see that they are set to same value.
Also please note on #7767, purpose of the the issue is to fix running argo-workflows on a subpath requires rewrites on the reverse proxy . The suggest about rootpath is a way to archive the purpose. I think this PR also fix #7767

[agilgur5]

I think in this project basehref and rootpath is the same.

No they are separate, it's just that rootpath was never implemented in Workflows.
I also think it is good for Argo projects to be consistent where possible (and where it makes sense), so if Argo CD has those as two separate flags, Argo Workflows can implement that consistently.

Moreover, "base href" refers to a Web standard, literally the HTML base element and its href attribute. I don't think we should redefine a Web standard. Things like that tend to cause more confusion rather than less. We should try to teach users about the existing standards and how they work -- which is why those were good docs changes. We might be able to further improve those docs.

--rootpath also would require more changes. Ingress rewrites change the external API route as well (e.g. my-domain.com/api -> my-domain.com/argo/api). So we'd need to do the same thing for --rootpath. #7767 (comment) shows several of the changes in CD required (like the GRPC gateway).

[sonbui00]

@agilgur5 Could you give me a real life example for rootpath and basehref with different values for running Argo Workflow?

[sonbui00]

API request my-domain.com/argo/apiwith prefix /argo/ -> should work without rewrite on additional controller for #7767

I think my PR do not handle this

[sonbui00]

@agilgur5 Maybe we also need rootpath to handle all cases. let me try to figure it out

[agilgur5]

@agilgur5 Could you give me a real life example for rootpath and basehref with different values for running Argo Workflow?

The main one is an ingress rewrite. For instance, if I have my-cluster.com as my main cluster domain and all services as subpaths, then I could have my-cluster.com/argo-cd/, my-cluster.com/argo-workflows/, my-cluster.com/argo-events/, my-cluster.com/prometheus/, etc.

If all those services did not support rootpath, it would be easier to configure ingress rewrites for all of them in the same way -- only the subpath would be different between them. If they did all support rootpath, they may all also require different configuration (e.g. a configmap vs a flag vs an env var, etc), so rewrites may be easier/more consistent as well.

I can think of some other wacky use-cases, but that's the only one I can think of off the top of my head that is a "real life example" that I and others have definitely used many times before.

[agilgur5]

I think my PR do not handle this

@agilgur5 Maybe we also need rootpath to handle all cases. let me try to figure it out

Yea that's what I meant when I said that full rootpath support to handle all cases would require a larger PR.
Feel free to take that on if you want!