feat(helm): Add support for external authentication #2104
Conversation
adutra
force-pushed
the
helm-auth
branch
2 times, most recently
from
6bbb635 to
062e54b
Compare
| */}} | ||
| {{- define "polaris.dictToString" -}} |
There was a problem hiding this comment.
The template polaris.dictToString was unused.
| {{- $_ = set $map (printf "%s.token-broker.max-token-generation" $prefix) (dig "tokenBroker" "maxTokenGeneration" "PT1H" $auth) -}} | ||
| {{- $secretName := dig "tokenBroker" "secret" "name" "" $auth -}} | ||
| {{- if $secretName -}} | ||
| {{- $subpath := empty $realm | ternary "" (printf "%s/" (urlquery $realm)) -}} |
There was a problem hiding this comment.
This is a best-effort to produce valid filesystem paths if the realm ID contains weird characters. The only function available in Helm that vaguely serves this purpose is urlquery.
There was a problem hiding this comment.
Another option is to output the hash the realm ID. It's safer, but less readable. Lmk if that's preferable.
| {{- end -}} | ||
|
|
||
| {{/* | ||
| Convert a dict into a string formed by a comma-separated list of key-value pairs: key1=value1,key2=value2, ... | ||
| Escapes a property key to be used in a configmap, conforming with the Java parsisng rules for |
There was a problem hiding this comment.
We were missing until now a proper escape mechanism for config option keys. Before this change, any realm containing a key termination character such as a space or =, would produce invalid configuration.
| tokenService: | ||
| type: default | ||
| # -- The type of token broker to use. Two built-in types are supported: rsa-key-pair and symmetric-key. | ||
| # -- The `TokenBroker` implementation to use. Two built-in types are supported: rsa-key-pair and symmetric-key. |
There was a problem hiding this comment.
nit: TokenBroker is not applicable to the "external" auth type, right? Should we support disabling it?
There was a problem hiding this comment.
tokenBroker and tokenService are indeed only relevant when using internal (or mixed) authentication.
Setting these options when using external auth doesn't hurt, but in practice, the configmap won't even contain those config options if the authentication type is external.
There was a problem hiding this comment.
ah, thx for the explanation!.. I missed that behaviour 🤦
helm/polaris/values.yaml
Outdated
| client: | ||
| # -- The client ID to use when authenticating with the authentication server. | ||
| id: polaris | ||
| # -- The secret to pull the client secret from. |
There was a problem hiding this comment.
nit: the OIDC secret is optional, right?
There was a problem hiding this comment.
Yes. If secret.name is nil, no client secret is included in the configuration. This follows a pattern that is already used for other secrets.
There was a problem hiding this comment.
Do you think we could mention that in the doc comment for clarity? Current text feels like the secret might be required 🤔
There was a problem hiding this comment.
Done. I seized the opportunity to clarify tokenBroker and tokenService as well.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
| @@ -189,21 +189,24 @@ ct install --namespace polaris --charts ./helm/polaris | |||
| |-----|------|---------|-------------| | |||
| | advancedConfig | object | `{}` | Advanced configuration. You can pass here any valid Polaris or Quarkus configuration property. Any property that is defined here takes precedence over all the other configuration values generated by this chart. Properties can be passed "flattened" or as nested YAML objects (see examples below). Note: values should be strings; avoid using numbers, booleans, or other types. | | |||
There was a problem hiding this comment.
Can we copy this to site/content/in-dev/unreleased/helm.md as well?
There was a problem hiding this comment.
@MonkeyCanCode isn't this copy done automatically? Or should I make a manual copy-paste?
There was a problem hiding this comment.
Okay I copied the file manually. We should look into automate this.
There was a problem hiding this comment.
Yes, the automation is in the Makefile which is still under review: #2027 (make helm-doc-generate)
| @@ -336,3 +427,33 @@ tests: | |||
| asserts: | |||
| - matchRegex: { path: 'data["application.properties"]', pattern: "polaris.tasks.max-concurrent-tasks=10" } | |||
| - matchRegex: { path: 'data["application.properties"]', pattern: "polaris.tasks.max-queued-tasks=20" } | |||
|
|
|||
| - it: should configure OIDC | |||
There was a problem hiding this comment.
Probably not in the scope of this PR. But we should add the doc for OIDC #1327 and have some docker compose with it (e.g. keycloak?)
There was a problem hiding this comment.
We already have some docs for OIDC / external IDP:
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
No description provided.