add support for minimumKubeletVersion #1754

haircommander · 2024-10-14T16:28:27Z

No description provided.

haircommander · 2024-10-22T14:30:49Z

/retest

p0lyn0mial · 2024-11-21T08:03:34Z

How are we planning to test this observer? Are we going to have an integration test? At the very least, we could check if the configuration contains the current node version, right? I think we could have a more end-to-end test in origin.

haircommander · 2024-11-21T17:53:33Z

I was planning on doing an e2e test in origin. I'm not seeing integration tests in this repository, do you have a pointer @p0lyn0mial ?

benluddy · 2024-11-25T17:47:26Z

...com/openshift/library-go/pkg/operator/configobserver/node/observe_minimum_kubelet_version.go

+var minimumKubeletVersionConfigPath = "minimumKubeletVersion"
+
+// ObserveKubeletMinimumVersion watches the node configuration and generates the minimumKubeletVersion
+func ObserveMinimumKubeletVersion(genericListers configobserver.Listers, _ events.Recorder, existingConfig map[string]interface{}) (ret map[string]interface{}, errs []error) {


This should be feature gated too (and will now need to be aware of the new authorization-mode in the carry patch).

haircommander · 2024-11-26T19:04:18Z

I decided to take config observation from library-go as iterating is tricky and AFAIU this code won't be used elsewhere

haircommander · 2024-12-06T00:03:19Z

/retest

haircommander · 2024-12-06T14:40:06Z

/retest

bindata/config.go

pkg/operator/configobservation/node/listers.go

pkg/operator/configobservation/node/observe_minimum_kubelet_version.go

pkg/cmd/render/render.go

pkg/operator/configobservation/node/observe_minimum_kubelet_version.go

p0lyn0mial · 2024-12-09T09:43:09Z

pkg/operator/configobservation/node/observe_minimum_kubelet_version.go

+	if on {
+		defaultAuthModes = append(defaultAuthModes, ModeMinimumKubeletVersion)
+	}
+	sort.Sort(sort.StringSlice(defaultAuthModes))


if the list is hardcoded and we always append at the end then maybe we don't have to sort.

it was initially added at the suggestion of @benluddy I'm fine either way

p0lyn0mial · 2024-12-09T09:45:21Z

pkg/operator/configobservation/node/observe_minimum_kubelet_version.go

+// removes it instead.
+// This function assumes MinimumKubeletVersion auth mode isn't present by default,
+// and should likely be removed when it is.
+func SetAPIServerArgumentsToEnforceMinimumKubeletVersion(defaultAuthModes []string, newConfig map[string]interface{}, on bool) error {


Could we also rename this method to AddMinimumKubeletVersionAuthorizationMode or something like that?

went for AddAuthorizationModes as suggested above

p0lyn0mial · 2024-12-09T10:10:10Z

BTW: do we want to merge this pr before openshift/kubernetes#2104 ?

haircommander · 2024-12-09T17:50:17Z

BTW: do we want to merge this pr before openshift/kubernetes#2104 ?

I think I'd actually prefer o/k first

p0lyn0mial · 2024-12-09T19:06:27Z

pkg/operator/configobservation/node/observe_authorization_mode.go

+// removes it instead.
+// This function assumes MinimumKubeletVersion auth mode isn't present by default,
+// and should likely be removed when it is.
+func AddAuthorizationModes(observedConfig map[string]interface{}, isMinimumKubeletVersionEnabled bool) error {


ok, please double-check if we need to call this method on a default configuration from somewhere here.

cluster-kube-apiserver-operator/pkg/operator/targetconfigcontroller/targetconfigcontroller.go

Line 227 in 992114a

defaultConfig := bindata.MustAsset("assets/config/defaultconfig.yaml")

just in case the observer won't be executed/flags weren't initialised.

Maybe we could split this method into two, one that accepts the value from the FG and another that adds hardcoded/default values.

hm the approach taken for pod security (the other featuregated observer) is to set an invalid config in the default so we know it's overridden. I'll do that instead I think

Do you have a link? I think that most of the time it will work. I just want to ensure it will always work even if something is wrong with the observer ...

cluster-kube-apiserver-operator/bindata/assets/config/defaultconfig.yaml

Lines 17 to 22 in 992114a

enforce: "invalid-to-force-substitution"

enforce-version: "invalid-to-force-substitution"

audit: "invalid-to-force-substitution"

audit-version: "invalid-to-force-substitution"

warn: "invalid-to-force-substitution"

warn-version: "invalid-to-force-substitution"

which is then unconditionally overridden by

cluster-kube-apiserver-operator/pkg/cmd/render/render.go

Lines 350 to 357 in 992114a

if !featureGates.Enabled(features.FeatureGateOpenShiftPodSecurityAdmission) {

if err := auth.SetPodSecurityAdmissionToEnforcePrivileged(defaultConfig); err != nil {

return nil, err

}

} else {

if err := auth.SetPodSecurityAdmissionToEnforceRestricted(defaultConfig); err != nil {

return nil, err

}

I think that the render command creates the bootstrap manifest, and the target controller creates a manifest for each revision. We need to ensure that the target controller also applies the default set of the auth methods (just in case). WDYT ?

p0lyn0mial · 2024-12-10T10:05:50Z

BTW: do we want to merge this pr before openshift/kubernetes#2104 ?

I think I'd actually prefer o/k first

should we merge the origin tests before the o/k PR?

haircommander · 2024-12-10T14:43:21Z

I wouldn't expect it to pass without the tests, but if we're okay with a window where techpreview serial jobs fail then I'm open. If I were to choose the order I'd say o/k, o/ckaso, o/origin. Since the feature is gated in both implementation PRs there shouldn't be a risk to merging without a test

haircommander · 2024-12-10T21:10:17Z

as expected openshift/origin#29353 failed

haircommander · 2024-12-11T22:04:45Z

/retest

p0lyn0mial · 2024-12-12T07:59:22Z

I wouldn't expect it to pass without the tests, but if we're okay with a window where techpreview serial jobs fail then I'm open

We cannot afford it. Is there a way to test these three PRs to ensure everything works as expected?
Could you also get a review on the test and maybe even a tag, so that all PRs can land roughly at the same time?
We could put a hold on the test PR until the other two PRs land.

p0lyn0mial · 2024-12-12T08:00:37Z

bindata/assets/config/defaultconfig.yaml

-    - SystemMasters
-    - RBAC
-    - Node
+    invalidMap: invalidToForceSubstitution


I would remove it and add the default set to the target controller

xref: #1754 (comment)

I ended up doing so in the last commit

pkg/operator/configobservation/node/observe_minimum_kubelet_version.go

pkg/operator/configobservation/node/observe_authorization_mode.go

pkg/operator/configobservation/node/observe_minimum_kubelet_version.go

openshift-ci · 2024-12-13T21:04:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haircommander
Once this PR has been reviewed and has the lgtm label, please assign tkashem for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

haircommander · 2024-12-14T01:36:09Z

/retest

haircommander · 2024-12-16T14:39:59Z

/retest

p0lyn0mial

LGTM

not tagging because we want to merge the other PR(s) first.

p0lyn0mial · 2024-12-17T08:42:09Z

pkg/operator/targetconfigcontroller/targetconfigcontroller.go

@@ -228,12 +229,21 @@ func manageKubeAPIServerConfig(ctx context.Context, client coreclientv1.ConfigMa
 	configOverrides := bindata.MustAsset("assets/config/config-overrides.yaml")
 	specialMergeRules := map[string]resourcemerge.MergeFunc{}

+	// Guarantee the authorization-mode will be present in the base config, regardless of whether the observer is running
+	authModeOverride := map[string]interface{}{}


nit: technically, this is our default configuration, so I would rename it to defaultAuthModes.

Signed-off-by: Peter Hunt <[email protected]>

…ify strictly with observer Signed-off-by: Peter Hunt <[email protected]>

…er doesn't run Signed-off-by: Peter Hunt <[email protected]>

openshift-ci · 2024-12-19T23:39:17Z

@haircommander: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-operator-encryption-rotation-aesgcm	`0481182`	link	false	`/test e2e-gcp-operator-encryption-rotation-aesgcm`
ci/prow/e2e-gcp-operator-encryption-rotation-single-node	`0481182`	link	false	`/test e2e-gcp-operator-encryption-rotation-single-node`
ci/prow/e2e-gcp-operator-encryption-single-node	`0481182`	link	false	`/test e2e-gcp-operator-encryption-single-node`
ci/prow/e2e-gcp-operator-encryption-perf-single-node	`0481182`	link	false	`/test e2e-gcp-operator-encryption-perf-single-node`
ci/prow/e2e-gcp-operator-encryption-aescbc	`0481182`	link	false	`/test e2e-gcp-operator-encryption-aescbc`
ci/prow/e2e-gcp-operator	`2d97288`	link	true	`/test e2e-gcp-operator`
ci/prow/e2e-gcp-operator-single-node	`2d97288`	link	false	`/test e2e-gcp-operator-single-node`
ci/prow/unit	`2d97288`	link	true	`/test unit`
ci/prow/e2e-aws-ovn-single-node	`2d97288`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-aws-operator-disruptive-single-node	`2d97288`	link	false	`/test e2e-aws-operator-disruptive-single-node`

Full PR test history. Your PR dashboard.

Instructions 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. I understand the commands that are listed here.

openshift-ci bot requested review from benluddy and p0lyn0mial October 14, 2024 16:28

haircommander force-pushed the min-kubelet-version branch 5 times, most recently from 5aa38d0 to 0b6e63b Compare October 21, 2024 19:32

haircommander changed the title ~~Min kubelet version~~ WIP add support for minimumKubeletVersion Oct 21, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2024

haircommander force-pushed the min-kubelet-version branch from 0b6e63b to 2f76862 Compare October 21, 2024 20:34

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2024

haircommander mentioned this pull request Oct 28, 2024

add minimumKubeletVersion openshift/api#2059

Merged

haircommander force-pushed the min-kubelet-version branch from 2f76862 to ba2bc46 Compare October 30, 2024 17:20

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2024

haircommander force-pushed the min-kubelet-version branch from ba2bc46 to 0481182 Compare November 18, 2024 19:59

haircommander mentioned this pull request Nov 20, 2024

add support for minimumKubeletVersion openshift/library-go#1831

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2024

haircommander force-pushed the min-kubelet-version branch from 0481182 to e019d98 Compare November 21, 2024 21:07

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2024

benluddy reviewed Nov 25, 2024

View reviewed changes

haircommander force-pushed the min-kubelet-version branch 2 times, most recently from 2c99747 to 5f87e9b Compare November 26, 2024 18:58

haircommander changed the title ~~WIP add support for minimumKubeletVersion~~ add support for minimumKubeletVersion Nov 26, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 26, 2024

haircommander mentioned this pull request Nov 26, 2024

configobserver: drop observer for minimum kubelet version openshift/library-go#1898

Merged

haircommander force-pushed the min-kubelet-version branch from 5f87e9b to 6d45957 Compare November 26, 2024 19:45

haircommander force-pushed the min-kubelet-version branch from ed2d94b to 07665c1 Compare December 5, 2024 17:07

haircommander mentioned this pull request Dec 6, 2024

extended: add minimum kubelet version test openshift/origin#29353

Open

haircommander force-pushed the min-kubelet-version branch from 07665c1 to c509f79 Compare December 6, 2024 19:51

p0lyn0mial reviewed Dec 9, 2024

View reviewed changes

haircommander force-pushed the min-kubelet-version branch 2 times, most recently from f51e26e to 3b1fb7d Compare December 9, 2024 17:49

p0lyn0mial reviewed Dec 9, 2024

View reviewed changes

haircommander force-pushed the min-kubelet-version branch from 3604289 to d2e41cb Compare December 11, 2024 17:55

p0lyn0mial reviewed Dec 12, 2024

View reviewed changes

haircommander force-pushed the min-kubelet-version branch from d2e41cb to efcac32 Compare December 13, 2024 21:02

p0lyn0mial reviewed Dec 17, 2024

View reviewed changes

haircommander added 3 commits December 19, 2024 09:50

configobserver: add minimumkubeletversion

607e4fb

Signed-off-by: Peter Hunt <[email protected]>

add MinimumKubeletVersion authorization-mode if feature is on

e4c99d6

Signed-off-by: Peter Hunt <[email protected]>

observeAuthMode: drop authorization-mode from default config and spec…

5e27987

…ify strictly with observer Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the min-kubelet-version branch from efcac32 to c0745cb Compare December 19, 2024 14:50

targetconfigcontroller: inject authorization-mode just in case observ…

2d97288

…er doesn't run Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the min-kubelet-version branch from c0745cb to 2d97288 Compare December 19, 2024 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for minimumKubeletVersion #1754

add support for minimumKubeletVersion #1754

haircommander commented Oct 14, 2024

haircommander commented Oct 22, 2024

p0lyn0mial commented Nov 21, 2024

haircommander commented Nov 21, 2024

benluddy Nov 25, 2024

haircommander commented Nov 26, 2024

haircommander commented Dec 6, 2024

haircommander commented Dec 6, 2024

p0lyn0mial Dec 9, 2024

haircommander Dec 9, 2024

p0lyn0mial Dec 9, 2024

haircommander Dec 9, 2024

p0lyn0mial commented Dec 9, 2024

haircommander commented Dec 9, 2024

p0lyn0mial Dec 9, 2024

p0lyn0mial Dec 9, 2024

haircommander Dec 9, 2024

p0lyn0mial Dec 10, 2024

haircommander Dec 10, 2024

p0lyn0mial Dec 12, 2024

p0lyn0mial commented Dec 10, 2024

haircommander commented Dec 10, 2024

haircommander commented Dec 10, 2024

haircommander commented Dec 11, 2024

p0lyn0mial commented Dec 12, 2024

p0lyn0mial Dec 12, 2024

haircommander Dec 13, 2024

openshift-ci bot commented Dec 13, 2024

haircommander commented Dec 14, 2024

haircommander commented Dec 16, 2024

p0lyn0mial left a comment

p0lyn0mial Dec 17, 2024

openshift-ci bot commented Dec 19, 2024

	enforce: "invalid-to-force-substitution"
	enforce-version: "invalid-to-force-substitution"
	audit: "invalid-to-force-substitution"
	audit-version: "invalid-to-force-substitution"
	warn: "invalid-to-force-substitution"
	warn-version: "invalid-to-force-substitution"

	if !featureGates.Enabled(features.FeatureGateOpenShiftPodSecurityAdmission) {
	if err := auth.SetPodSecurityAdmissionToEnforcePrivileged(defaultConfig); err != nil {
	return nil, err
	}
	} else {
	if err := auth.SetPodSecurityAdmissionToEnforceRestricted(defaultConfig); err != nil {
	return nil, err
	}

add support for minimumKubeletVersion #1754

Are you sure you want to change the base?

add support for minimumKubeletVersion #1754

Conversation

haircommander commented Oct 14, 2024

haircommander commented Oct 22, 2024

p0lyn0mial commented Nov 21, 2024

haircommander commented Nov 21, 2024

Choose a reason for hiding this comment

haircommander commented Nov 26, 2024

haircommander commented Dec 6, 2024

haircommander commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Dec 9, 2024

haircommander commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Dec 10, 2024

haircommander commented Dec 10, 2024

haircommander commented Dec 10, 2024

haircommander commented Dec 11, 2024

p0lyn0mial commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 13, 2024

haircommander commented Dec 14, 2024

haircommander commented Dec 16, 2024

p0lyn0mial left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 19, 2024