Solve the problem with upstreams being prohibited when alphaconfig is enabled #301
Conversation
EvanCarroll
changed the title
|
@EvanCarroll The PR also lacks the chart bump and the message for ArtifactHub |
Upstreams is no longer allowed if you have alphaConfig enabled, because the option is available in alphaConfig. However, setting the default in alphaConfig won't work, because people may have it disabled, and it's disabled by default. This means we need this value to remain in configFile as a default if alphaConfig isn't enabled. Fixes oauth2-proxy#226
|
@pierluigilenoci that good? |
|
@EvanCarroll, the chart bump is correct. Now, let's wait for @tuunit to approve it. |
| @@ -13,6 +13,13 @@ metadata: | |||
| name: {{ template "oauth2-proxy.fullname" . }} | |||
| namespace: {{ template "oauth2-proxy.namespace" $ }} | |||
| data: | |||
| {{- if and (not .Values.alphaConfig.enabled) .Values.config.legacyDefaults }} | |||
There was a problem hiding this comment.
Isn't legacyDefaults always defined through the default values in values.yaml and even if a user reconfigures it to be an empty object or empty string wouldn't the templating still work?
I'm just asking myself if we really need the if else condition here? Furthermore if required, I would rather prefer to have it surround the actual templating line for the legacyDefaults instead of the whole oauth2_proxy.cfg block
{{ tpl .Values.config.legacyDefaults $ | indent 4 }}
There was a problem hiding this comment.
That would probably work too, but I'm done w/ this one so someone else will have to pick it up. If I was going to spend any more time on this I would just put the remaining option into alphaConfig so the whole legacyDefaults can go away. This isn't supposed to be a long lasting fix. Just a patch to get you to the next point.
There was a problem hiding this comment.
I appreciate the attempt to help, but after checking this PR introduces a breaking change for certain configuration setups, and we can’t merge it as-is. I know it seems trivial but at the scale oauth2-proxy is being utilized (we are talking 30+M downloads a month), even minor tweaks need thorough validation! Throwing in a patch and declaring you’re ‘done’ isn’t how stable software gets maintained.
| {{ tpl .Values.config.legacyDefaults $ | indent 4 }} | ||
|
|
||
| {{ tpl .Values.config.configFile $ | indent 4 }} |
There was a problem hiding this comment.
This will actually cause a breaking change for existing deployments:
[2025/04/30 18:30:31] [main.go:42] ERROR: failed to load config: unable to load config file: While parsing config: toml: key upstreams is already defined
Upstreams is no longer allowed if you have alphaConfig enabled, because the option is available in alphaConfig. However, setting the default in alphaConfig won't work, because people may have it disabled, and it's disabled by default.
This means we need this value to remain in configFile as a default if alphaConfig isn't enabled.
#226