Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes #323

sebsoto · 2021-03-01T15:22:27Z

This commit fixes an issue where the public key hash annotation was being
added to not all nodes, not just Windows nodes. This commit also adds
logic to remove the annotation from Linux nodes when the operator is
installed.

As part of this fix, Windows node selection in the secret controller
now uses the label "kubernetes.io/os=windows". There is a mix of this
label and "node.openshift.io/os_id=Windows" in the node selection logic
throughout WMCO, and we should move towards just using one. Future work
should be done to only use the "kubernetes.io/os" label for node
selection throughout WMCO.

sebsoto · 2021-03-01T15:24:32Z

/approve cancel

openshift-ci-robot · 2021-03-02T20:28:44Z

@sebsoto: This pull request references Bugzilla bug 1930791, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes

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/test-infra repository.

openshift-ci-robot · 2021-03-02T20:33:14Z

@sebsoto: This pull request references Bugzilla bug 1934281, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1934281: [wmco] Stop adding pub-key-hash label to all nodes

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/test-infra repository.

openshift-ci-robot · 2021-03-02T20:38:11Z

@sebsoto: This pull request references Bugzilla bug 1930791, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes

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/test-infra repository.

aravindhp

Thanks for working on this, @sebsoto. Please address my comments.

aravindhp · 2021-03-02T20:48:50Z

controllers/secret/secret_controller.go

@@ -180,7 +184,13 @@ func (r *ReconcileSecret) Reconcile(request reconcile.Request) (reconcile.Result
 			return reconcile.Result{}, errors.Wrap(err, "error creating signer from private key")
 		}
 		nodes := &core.NodeList{}
-		err = r.client.List(context.TODO(), nodes, client.HasLabels{nodeconfig.WindowsOSLabel})


Can't you do the equivalent of tc.client.K8s.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{LabelSelector: nodeconfig.WindowsOSLabel}) that we have in our e2e tests? i.e use the label selector.

I ended up taking @VaishnaviHire suggestion and using the core.LabelOSStable label which makes this simpler

While I am fine with the change, it has again been made silently. Please call this out in the commit message. Also add a follow up story to replace nodeconfig.WindowsOSLabel with core.LabelOSStable

aravindhp · 2021-03-02T20:50:48Z

controllers/secret/secret_controller.go

@@ -213,6 +223,28 @@ func (r *ReconcileSecret) Reconcile(request reconcile.Request) (reconcile.Result
 	}
 }

+// removeDepreciatedAnnotations corrects annotations applied by previous versions of WMCO.


I suggest renaming this to removeInvalidAnnotationsFromLinuxNodes or something to that effect. Please mention the annotation that you are deleting. I don't see any reason to future proof given this is just a helper.

aravindhp · 2021-03-02T20:51:45Z

controllers/secret/secret_controller.go

+// removeDepreciatedAnnotations corrects annotations applied by previous versions of WMCO.
+func (r *ReconcileSecret) removeDepreciatedAnnotations() error {
+	nodes := &core.NodeList{}
+	err := r.client.List(context.TODO(), nodes)


Shouldn't you just list Linux nodes?

controllers/secret/secret_controller.go

aravindhp · 2021-03-02T20:53:43Z

controllers/secret/secret_controller.go

+		if _, present := node.Annotations[nodeconfig.PubKeyHashAnnotation]; present == true &&
+			node.Labels[core.LabelOSStable] != "windows" {


If you only list Linux nodes using the label selector you won't have to do this check.

VaishnaviHire

@sebsoto Thanks for the fix. PTAL at my comments:

Commit message needs to be updated to annotation was being added to all nodes,

VaishnaviHire · 2021-03-02T20:58:38Z

controllers/secret/secret_controller.go

-		err = r.client.List(context.TODO(), nodes, client.HasLabels{nodeconfig.WindowsOSLabel})
+		// The controller-runtime client requires a map for label matching, so the Windows identifying label var needs
+		// to be split into a key/value pair.
+		windowsLabelKeyValuePair := strings.SplitN(nodeconfig.WindowsOSLabel, "=", 2)


Should we use core.LabelOSStable as we are doing in windows_machine controller ?

I was considering making this change but was on the fence about it. Since we are using it in the node mapper I'm happy to use it here.

VaishnaviHire · 2021-03-03T15:41:08Z

/lgtm

aravindhp

/approve

with the caveat that you address the minor comments.

aravindhp · 2021-03-03T15:54:05Z

controllers/secret/secret_controller.go

@@ -180,7 +184,13 @@ func (r *ReconcileSecret) Reconcile(request reconcile.Request) (reconcile.Result
 			return reconcile.Result{}, errors.Wrap(err, "error creating signer from private key")
 		}
 		nodes := &core.NodeList{}
-		err = r.client.List(context.TODO(), nodes, client.HasLabels{nodeconfig.WindowsOSLabel})


While I am fine with the change, it has again been made silently. Please call this out in the commit message. Also add a follow up story to replace nodeconfig.WindowsOSLabel with core.LabelOSStable

aravindhp · 2021-03-03T15:54:52Z

controllers/secret/secret_controller.go

@@ -59,6 +59,10 @@ func newReconciler(mgr manager.Manager) (reconcile.Reconciler, error) {
 	}

 	reconciler := &ReconcileSecret{client: client, scheme: mgr.GetScheme()}
+	if err = reconciler.removeInvalidAnnotationsFromLinuxNodes(); err != nil {
+		log.Error(err, "unable to clean up annotations from previous WMCO versions")


...annotations on Linux nodes...

aravindhp · 2021-03-03T15:58:18Z

/hold
so that you can address the comments.

openshift-ci-robot · 2021-03-03T15:59:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp

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~~ [aravindhp]

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

This commit fixes an issue where the public key hash annotation was being added to not all nodes, not just Windows nodes. This commit also adds logic to remove the annotation from Linux nodes when the operator is installed. As part of this fix, Windows node selection in the secret controller now uses the label "kubernetes.io/os=windows". There is a mix of this label and "node.openshift.io/os_id=Windows" in the node selection logic throughout WMCO, and we should move towards just using one. Future work should be done to only use the "kubernetes.io/os" label for node selection throughout WMCO.

openshift-ci-robot · 2021-03-03T16:15:38Z

@sebsoto: This pull request references Bugzilla bug 1930791, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes

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/test-infra repository.

VaishnaviHire · 2021-03-03T16:29:00Z

/lgtm

sebsoto · 2021-03-03T20:57:26Z

/hold cancel

openshift-bot · 2021-03-03T21:08:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-03T21:21:22Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2021-03-04T00:05:42Z

@sebsoto: All pull requests linked via external trackers have merged:

openshift/windows-machine-config-operator#323

Bugzilla bug 1930791 has been moved to the MODIFIED state.

In response to this:

Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes

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/test-infra repository.

sebsoto · 2021-03-31T14:15:00Z

/cherry-pick release-4.7

openshift-cherrypick-robot · 2021-03-31T14:18:55Z

@sebsoto: new pull request created: #370

In response to this:

/cherry-pick release-4.7

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/test-infra repository.

sebsoto · 2021-03-31T14:19:35Z

/cherry-pick community-4.7

openshift-cherrypick-robot · 2021-03-31T14:23:08Z

@sebsoto: new pull request created: #371

In response to this:

/cherry-pick community-4.7

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/test-infra repository.

Run hybrid-overlay as a Windows service in e2e tests

sebsoto changed the title ~~[wmco] Stop adding pub-key-hash label to all nodes~~ [WIP] [wmco] Stop adding pub-key-hash label to all nodes Mar 1, 2021

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 1, 2021

openshift-ci-robot requested a review from aravindhp March 1, 2021 15:22

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 1, 2021

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 1, 2021

sebsoto force-pushed the labelBug branch from 2a5530e to 4d494fa Compare March 2, 2021 20:27

sebsoto changed the title ~~[WIP] [wmco] Stop adding pub-key-hash label to all nodes~~ Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes Mar 2, 2021

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 2, 2021

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Mar 2, 2021

sebsoto changed the title ~~Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes~~ Bug 1934281: [wmco] Stop adding pub-key-hash label to all nodes Mar 2, 2021

sebsoto changed the title ~~Bug 1934281: [wmco] Stop adding pub-key-hash label to all nodes~~ Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes Mar 2, 2021

aravindhp suggested changes Mar 2, 2021

View reviewed changes

VaishnaviHire reviewed Mar 2, 2021

View reviewed changes

sebsoto force-pushed the labelBug branch 3 times, most recently from 7373048 to b2ad5da Compare March 3, 2021 15:18

openshift-ci-robot assigned VaishnaviHire Mar 3, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2021

aravindhp approved these changes Mar 3, 2021

View reviewed changes

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 3, 2021

sebsoto force-pushed the labelBug branch from b2ad5da to 260324e Compare March 3, 2021 16:06

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2021

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2021

openshift-merge-robot merged commit 1b5f54b into openshift:master Mar 4, 2021

openshift-cherrypick-robot mentioned this pull request Mar 31, 2021

Bug 1945248: [wmco] Stop adding pub-key-hash label to all nodes #370

Merged

openshift-cherrypick-robot mentioned this pull request Mar 31, 2021

[community-4.7][wmco] Stop adding pub-key-hash label to all nodes #371

Merged

wgahnagl pushed a commit to wgahnagl/windows-machine-config-operator that referenced this pull request Mar 6, 2023

Merge pull request openshift#323 from sebsoto/hybridOverlayFix

b44204f

Run hybrid-overlay as a Windows service in e2e tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes #323

Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes #323

sebsoto commented Mar 1, 2021 •

edited

sebsoto commented Mar 1, 2021

openshift-ci-robot commented Mar 2, 2021

openshift-ci-robot commented Mar 2, 2021

openshift-ci-robot commented Mar 2, 2021

aravindhp left a comment

aravindhp Mar 2, 2021

sebsoto Mar 3, 2021

aravindhp Mar 3, 2021

aravindhp Mar 2, 2021

aravindhp Mar 2, 2021

aravindhp Mar 2, 2021

VaishnaviHire left a comment

VaishnaviHire Mar 2, 2021

sebsoto Mar 2, 2021

VaishnaviHire commented Mar 3, 2021

aravindhp left a comment

aravindhp Mar 3, 2021

aravindhp Mar 3, 2021

aravindhp commented Mar 3, 2021

openshift-ci-robot commented Mar 3, 2021

openshift-ci-robot commented Mar 3, 2021

VaishnaviHire commented Mar 3, 2021

sebsoto commented Mar 3, 2021

openshift-bot commented Mar 3, 2021

openshift-bot commented Mar 3, 2021

openshift-ci-robot commented Mar 4, 2021

sebsoto commented Mar 31, 2021

openshift-cherrypick-robot commented Mar 31, 2021

sebsoto commented Mar 31, 2021

openshift-cherrypick-robot commented Mar 31, 2021

		if _, present := node.Annotations[nodeconfig.PubKeyHashAnnotation]; present == true &&
		node.Labels[core.LabelOSStable] != "windows" {

Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes #323

Bug 1930791: [wmco] Stop adding pub-key-hash label to all nodes #323

Conversation

sebsoto commented Mar 1, 2021 • edited

sebsoto commented Mar 1, 2021

openshift-ci-robot commented Mar 2, 2021

openshift-ci-robot commented Mar 2, 2021

openshift-ci-robot commented Mar 2, 2021

aravindhp left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishnaviHire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VaishnaviHire commented Mar 3, 2021

aravindhp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindhp commented Mar 3, 2021

openshift-ci-robot commented Mar 3, 2021

openshift-ci-robot commented Mar 3, 2021

VaishnaviHire commented Mar 3, 2021

sebsoto commented Mar 3, 2021

openshift-bot commented Mar 3, 2021

openshift-bot commented Mar 3, 2021

openshift-ci-robot commented Mar 4, 2021

sebsoto commented Mar 31, 2021

openshift-cherrypick-robot commented Mar 31, 2021

sebsoto commented Mar 31, 2021

openshift-cherrypick-robot commented Mar 31, 2021

sebsoto commented Mar 1, 2021 •

edited