require scheme on userAccessibleOauthIssuerHost #8273
Conversation
|
could you update the comment on the proto and regenerate it? https://github.com/pachyderm/pachyderm/blob/master/src/auth/auth.proto#L70 |
|
Ok I pushed up a change making it explicitly non-breaking. PTAL |
| {{- else if (include "pachyderm.host" .) -}} | ||
| {{- (include "pachyderm.host" .) -}} | ||
| {{- printf "%s://%s" (include "pachyderm.hostproto" .) (include "pachyderm.host" .) -}} |
There was a problem hiding this comment.
I don't think you should use this necessarily, but there is actually a URL building function built into helm: https://helm.sh/docs/chart_template_guide/function_list/#urljoin
There was a problem hiding this comment.
Nice, going to leave it this way now for consistency, but might make sense to switch how we construct must url's to that in a followup PR.
|
BTW @BOsterbuhr @seslattery just to be ultra up front about this, it's still potentially a breaking change for people that supply their own auth secret (pachyderm-auth). I'm not sure what the semantics would look like if we accounted for that; on the one hand, generating a manifest with our chart and then expecting it to work forever is the wrong outlook, but on the other hand, people do do it. That said, the one customer I've seen doing this does have a valid URL there, and like I said, I'm not sure what we would do without a scheme on that URL anyway. Assume HTTP? |
Ah good callout on auth-secret. I mean I could assume http, but even then it's still a potentially breaking change in the chance they actually wanted https. I guess I could fallback to the old behavior and look at the issuerURI scheme? But I really don't know that it's worth adding so many layers of logic indirection to try and prevent breakages, as I think it becomes much harder to support in the future. Though we could support the old behavior here, and log.Warn an explicit message that field will require an explicit scheme set in 2.4.0+? @tybritten @BOsterbuhr thoughts? |
Codecov Report
@@ Coverage Diff @@
## master #8273 +/- ##
==========================================
- Coverage 9.00% 8.45% -0.56%
==========================================
Files 352 348 -4
Lines 104826 104066 -760
==========================================
- Hits 9435 8794 -641
+ Misses 93704 93696 -8
+ Partials 1687 1576 -111
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Setting oidc.issuerURI: "http://pachd:30658/dex" but wanting a userAccessibleOauthIssuerHost with a scheme of https is currently incompatible, as
pachctl auth login -bwould use the scheme from the oidc.issuerURI. This is a breaking config change, where we now require a scheme to be explicitly set when configuring userAccessibleOauthIssuerHost, and a guard clause has been added to try and prevent that misconfiguration.