fix: Config file overrides apply to some fields but not other

[rashmichandrashekar]

What this PR does / why we need it:
Fixes bug where some option values are overriden with config file but not others.
Provides optional behavior for backward compatiblity

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
Does not change cardinality

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2704

[dgrisonnet]

/assign @rexagod
/cc
/triage accepted

[rashmichandrashekar]

Hi @rexagod - You have been assigned as a reviewer. Could you please review this PR? Thanks!

[rashmichandrashekar]

@dgrisonnet @CatherineF-dev - Gentle ping, pls take a look at the pr and provide your review, thanks!

[rashmichandrashekar]

@CatherineF-dev - Added tests as per your feedback, pls take a look. Thanks!

[rexagod]

Suggested change

	// This is only needed for types that are a slice of structs because the default behaviour of yaml.Unmarshal is to append instead of overwrite
	func configureResourcesAndMetrics(opts *options.Options, configFile []byte) *options.Options {
	// If the config file is set, we will overwrite the opts with the config file.
	// This is only needed for maps because the default behaviour of yaml.Unmarshal is to append keys (and overwrite any conflicting ones).

I'd suggest adding a comment about the config file taking precedence over options in the flag description.

Also, I believe we need to do this for AnnotationsAllowList and LabelsAllowList as well?

[rashmichandrashekar]

Good catch. Added this too. @rexagod - please take a look

[richabanker]

@rashmichandrashekar this seems to be running into some CI failures.

[rashmichandrashekar]

@rashmichandrashekar this seems to be running into some CI failures.

Thanks @richabanker. Could you pls retrigger the runs?

[mrueg]

Triggered the CI run

/ok-to-test
/lgtm

for others to review:
/hold

[rashmichandrashekar]

Pinging other reviewers - @dgrisonnet @CatherineF-dev - This PR is waiting on additional reviewers. Could you pls take a look? Thanks!

[rexagod]

I'd suggest adding a comment about the config file taking precedence over options in the flag description.

I believe this is remaining here?

[rashmichandrashekar]

I'd suggest adding a comment about the config file taking precedence over options in the flag description.

I believe this is remaining here?

Thanks! added

[CatherineF-dev]

In apiserver, command-line arguments override any settings provided in a configuration file.

https://kubernetes.io/docs/tasks/administer-cluster/kubelet-config-file/#start-a-kubelet-process-configured-via-the-config-file

@richabanker and I find standard practice is using command-line arguments to override configuration file.
@dgrisonnet

If so, is your project blocked by this? Could you find other ways?

[rashmichandrashekar]

In apiserver, command-line arguments override any settings provided in a configuration file.

https://kubernetes.io/docs/tasks/administer-cluster/kubelet-config-file/#start-a-kubelet-process-configured-via-the-config-file

@richabanker and I find standard practice is using command-line arguments to override configuration file. @dgrisonnet

If so, is your project blocked by this? Could you find other ways?

@CatherineF-dev - The current behavior is that the config file overrides the cli args. This PR is making it consistent for all params. Yes, if the change is to break this behavior, it is blocking.

[rashmichandrashekar]

/ok-to-test

[richabanker]

/lgtm

Re-iterating from the sig meeting, since this fix makes the behavior of overriding CLI flags with config file consistent for slice params, makes sense to me to have this fix merged now.

While this is opposite to the pattern we have for the main K8s components, but this was a choice made early on, which will require a separate dedicated discussion/enhancement if we want to adopt that for KSM due to the breaking nature of the change.

[richabanker]

/hold
for @CatherineF-dev and @rexagod to express any outstanding concerns

[rexagod]

Could you please squash the commits here as well?

[rashmichandrashekar]

/ok-to-test

[rashmichandrashekar]

/ok-to-test

[rashmichandrashekar]

Could you please squash the commits here as well?

Done

[rashmichandrashekar]

@rexagod - Addressed all comments. pls take a look

[CatherineF-dev]

/lgtm on using config file to override command flags.
/hold for others

[rexagod]

/lgtm
/unhold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CatherineF-dev, mrueg, rashmichandrashekar, rexagod, richabanker

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [CatherineF-dev,mrueg,rexagod]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment