fix: accept legacy custom_resource_config_file as deprecated alias#2926
Conversation
|
This issue is currently awaiting triage. If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the The DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
cc @bhope |
|
Thanks for the PR! /lgtm |
| } | ||
|
|
||
| yaml.Unmarshal(configFile, opts) | ||
|
|
There was a problem hiding this comment.
Can you add a todo comment to remove it in ksm 2.21?
There was a problem hiding this comment.
Can also be in the larger comment below just use the TODO as a keyword
There was a problem hiding this comment.
Pull request overview
Restores backward compatibility for kube-state-metrics option YAML parsing by accepting the legacy custom_resource_config_file key (deprecated) as an alias for custom_resource_state_config_file, addressing silent CRS config breakage for upgrades from v2.16 → v2.17+.
Changes:
- Add a deprecated
custom_resource_config_fileYAML key alias and log a deprecation notice when it’s used. - Copy legacy key into the canonical CRS config file option when the canonical key is unset.
- Add unit tests for YAML unmarshal precedence and update CRS docs to mention the deprecated alias.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pkg/options/options.go |
Adds a deprecated Options field to accept the legacy YAML key. |
internal/wrapper.go |
Logs deprecation + applies aliasing during config merge at startup. |
pkg/options/options_test.go |
Adds tests validating YAML unmarshal behavior for legacy vs canonical keys. |
docs/metrics/extend/customresourcestate-metrics.md |
Documents canonical YAML key and legacy deprecated alias. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if opts.CustomResourceConfigFileDeprecated != "" { | ||
| klog.InfoS("The 'custom_resource_config_file' config key is deprecated and will be removed in a future release; use 'custom_resource_state_config_file' instead") |
There was a problem hiding this comment.
The deprecation notice is logged with InfoS, but the struct/docs describe it as a warning; consider using a warning-level log (e.g., klog.Warningf/WarningS) so it’s visible in typical production log filters. Also, gating on opts.CustomResourceConfigFileDeprecated != "" won’t warn (or alias) if the legacy key is explicitly present but empty; if the intent is “warn whenever the key is present”, detect key presence via Viper (cfgViper.IsSet("custom_resource_config_file")) or a YAML node-based unmarshal.
| if opts.CustomResourceConfigFileDeprecated != "" { | |
| klog.InfoS("The 'custom_resource_config_file' config key is deprecated and will be removed in a future release; use 'custom_resource_state_config_file' instead") | |
| if cfgViper.IsSet("custom_resource_config_file") { | |
| klog.Warning("The 'custom_resource_config_file' config key is deprecated and will be removed in a future release; use 'custom_resource_state_config_file' instead") |
| CustomResourceConfigFileDeprecated string `yaml:"custom_resource_config_file"` | ||
| ContinueWithoutCustomResourceConfigFile bool `yaml:"continue_without_custom_resource_state_config_file"` |
There was a problem hiding this comment.
This struct field formatting looks like it hasn’t been gofmt’d (extra spaces between name/type/tag). Please run gofmt so the struct remains consistently formatted with the rest of the file.
| } | ||
|
|
||
| yaml.Unmarshal(configFile, opts) | ||
|
|
||
| if opts.CustomResourceConfigFileDeprecated != "" { | ||
| klog.InfoS("The 'custom_resource_config_file' config key is deprecated and will be removed in a future release; use 'custom_resource_state_config_file' instead") | ||
| if opts.CustomResourceConfigFile == "" { | ||
| opts.CustomResourceConfigFile = opts.CustomResourceConfigFileDeprecated | ||
| } |
There was a problem hiding this comment.
os.ReadFile / yaml.Unmarshal errors are ignored here. If reading the file fails, configFile can be nil/empty and opts will silently remain unchanged; if YAML is invalid, unmarshalling failure will also be silent. Consider returning/exiting (honoring ContinueWithoutConfig) on read failure and handling the yaml.Unmarshal error explicitly so configuration issues aren’t silently ignored.
| } | |
| yaml.Unmarshal(configFile, opts) | |
| if opts.CustomResourceConfigFileDeprecated != "" { | |
| klog.InfoS("The 'custom_resource_config_file' config key is deprecated and will be removed in a future release; use 'custom_resource_state_config_file' instead") | |
| if opts.CustomResourceConfigFile == "" { | |
| opts.CustomResourceConfigFile = opts.CustomResourceConfigFileDeprecated | |
| } | |
| if !opts.ContinueWithoutConfig { | |
| klog.FlushAndExit(klog.ExitFlushTimeout, 1) | |
| } | |
| } else { | |
| if err := yaml.Unmarshal(configFile, opts); err != nil { | |
| klog.ErrorS(err, "failed to unmarshal options configuration file", "file", file) | |
| if !opts.ContinueWithoutConfig { | |
| klog.FlushAndExit(klog.ExitFlushTimeout, 1) | |
| } | |
| } else if opts.CustomResourceConfigFileDeprecated != "" { | |
| klog.InfoS("The 'custom_resource_config_file' config key is deprecated and will be removed in a future release; use 'custom_resource_state_config_file' instead") | |
| if opts.CustomResourceConfigFile == "" { | |
| opts.CustomResourceConfigFile = opts.CustomResourceConfigFileDeprecated | |
| } | |
| } |
There was a problem hiding this comment.
Can you errcheck on the yaml. Unmarshal call? Thanks
Signed-off-by: Nour <nurmn3m@gmail.com>
73aec31 to
9f66f83
Compare
|
/lgtm |
…ml.Unmarshal Signed-off-by: Nour <nurmn3m@gmail.com>
|
/lgtm Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bhope, mrueg, nmn3m The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
The
custom_resource_config_fileconfig-file key was renamed tocustom_resource_state_config_filein #2703 so it would match the CLI flag--custom-resource-state-config-file. Because YAML unmarshalling silently ignores unknown keys, users upgrading from v2.16 to v2.17+ saw their custom resource state config silently stop loading, with no error or warning.This PR restores backward compatibility by honoring the legacy
custom_resource_config_filekey as a deprecated alias:CustomResourceConfigFileso the CRS file loads as it did pre-v2.17.custom_resource_state_config_filetakes precedence.docs/metrics/extend/customresourcestate-metrics.md) now documents both keys and notes the alias is scheduled for removal.Coverage was added in
pkg/options/options_test.gofor the unmarshal contract (alias populates the deprecated field; canonical wins when both are set).How does this change affect the cardinality of KSM: does not change cardinality
Which issue(s) this PR fixes:
Fixes #2803