instancetype: Reject Matcher updates without updating RevisionName #9421

lyarwood · 2023-03-13T16:18:27Z

/area instancetype

What this PR does / why we need it:

As set out in issue #9132 requests to update the Name of an {Instancetype,Preference}Matcher were always accepted regardless of the RevisionName also being updated. This would result in an older and likely unwanted ControllerRevision being used containing an outdated or completely wrong Instancetype or Preference.

With this change the VM mutation webhook will now reject requests to update the target Name of a {Instancetype,Preference}Matcher without also updating the RevisionName.

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 #9132

Special notes for your reviewer:

Release note:

 Requests to update the target `Name` of a `{Instancetype,Preference}Matcher` without also updating the `RevisionName` are now rejected.

0xFelix · 2023-03-14T12:38:11Z

pkg/virt-api/webhooks/mutating-webhook/mutators/vm-mutator.go

+		if oldMatcher.GetName() != newMatcher.GetName() && newMatcher.GetRevisionName() != "" {
+			return fmt.Errorf("the Matcher Name has been updated without clearing the RevisionName")
+		}


Change this to

if oldMatcher.GetName() != newMatcher.GetName() && oldMatcher.GetRevisionName() == newMatcher.GetRevisionName() { return fmt.Errorf("the Matcher Name has been updated without updating the RevisionName") }

to allow changing to a different matcher with a specific revision in one go?

lyarwood · 2023-03-15T13:56:14Z

/retest-required

0xFelix

/lgtm

Thanks!

lyarwood · 2023-03-16T14:41:31Z

/cherrypick release-0.59

kubevirt-bot · 2023-03-16T14:41:37Z

@lyarwood: once the present PR merges, I will cherry-pick it on top of release-0.59 in a new PR and assign it to you.

In response to this:

/cherrypick release-0.59

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.

xpivarc · 2023-03-16T15:41:08Z

pkg/virt-api/webhooks/mutating-webhook/mutators/vm-mutator.go

+	return nil
+}
+
+func validateMatcherUpdate[M v1.Matcher](oldMatcher M, newMatcher M) error {


Why generic?

To save duplicating the logic below between {Instancetype,Preference}Matcher.

func validateMatcherUpdate(oldMatcher, newMatcher v1.Matcher) error works fine. Can you elaborate?

@xpivarc hopefully that answered your question, would you mind taking another look?

func validateMatcherUpdate(oldMatcher, newMatcher v1.Matcher) error works fine. Can you elaborate?

facepalm I'll drop this now sorry.

xpivarc · 2023-03-16T15:41:20Z

pkg/virt-api/webhooks/mutating-webhook/mutators/vm-mutator_test.go

@@ -125,6 +147,7 @@ var _ = Describe("VirtualMachine Mutator", func() {

 		k8sClient = k8sfake.NewSimpleClientset()
 		virtClient.EXPECT().CoreV1().Return(k8sClient.CoreV1()).AnyTimes()
+		virtClient.EXPECT().AppsV1().Return(k8sClient.AppsV1()).AnyTimes()


Why is this required?

Good question, the mutation webhook will attempt to lookup any preference related ControllerRevisions but ultimately swallow any errors later in the mutation flow:

kubevirt/pkg/virt-api/webhooks/mutating-webhook/mutators/vm-mutator.go

Lines 136 to 141 in 7d90329

preferenceSpec, err := mutator.InstancetypeMethods.FindPreferenceSpec(vm)

if err != nil {

// Log but ultimately swallow any preference lookup errors here and let the validating webhook handle them

log.Log.Reason(err).Error("Ignoring error attempting to lookup PreferredMachineType.")

return nil

}

But it is required for the introduced context and not for this one. So can we move it?

Yup that's true, I'll move it down.

xpivarc

Sorry I forgot to submit my review

xpivarc · 2023-03-21T10:21:48Z

pkg/virt-api/webhooks/mutating-webhook/mutators/vm-mutator.go

+	return nil
+}
+
+func validateMatcherUpdate[M v1.Matcher](oldMatcher M, newMatcher M) error {


func validateMatcherUpdate(oldMatcher, newMatcher v1.Matcher) error works fine. Can you elaborate?

xpivarc · 2023-03-21T10:23:00Z

pkg/virt-api/webhooks/mutating-webhook/mutators/vm-mutator_test.go

@@ -125,6 +147,7 @@ var _ = Describe("VirtualMachine Mutator", func() {

 		k8sClient = k8sfake.NewSimpleClientset()
 		virtClient.EXPECT().CoreV1().Return(k8sClient.CoreV1()).AnyTimes()
+		virtClient.EXPECT().AppsV1().Return(k8sClient.AppsV1()).AnyTimes()


But it is required for the introduced context and not for this one. So can we move it?

xpivarc

/approve
@0xFelix Can you give this one proper look?

kubevirt-bot · 2023-03-21T15:13:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

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

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

0xFelix · 2023-03-21T15:30:31Z

pkg/virt-api/webhooks/mutating-webhook/mutators/vm-mutator.go

+	if oldPreferenceMatcher == nil && newPreferenceMatcher == nil {
+		return nil
+	}
+	if err := validateMatcherUpdate(*oldPreferenceMatcher, *newPreferenceMatcher); err != nil {


Can you pass the matchers without derefing them?

edit - Apologies replied to the wrong comment, staticcheck was complaining about something before when I tried this but it's working now. Updated.

0xFelix · 2023-03-21T15:31:49Z

pkg/virt-api/webhooks/mutating-webhook/mutators/vm-mutator.go

+}
+
+func validatePreferenceMatcherUpdate(oldPreferenceMatcher *v1.PreferenceMatcher, newPreferenceMatcher *v1.PreferenceMatcher) []metav1.StatusCause {
+	if oldPreferenceMatcher == nil && newPreferenceMatcher == nil {


What happens if only one matcher is nil? Will it panic?

Yeah good catch, updated the && above, should be || to skip the check when one side isn't provided when they are initially introduced or later dropped.

As set out in issue kubevirt#9132 requests to update the Name of an {Instancetype,Preference}Matcher were always accepted regardless of the RevisionName being cleared or updated. This would result in an older and likely unwanted ControllerRevision being used containing an outdated or completely wrong Instancetype or Preference. With this change the VM mutation webhook will now reject requests to update the target Name of a {Instancetype,Preference}Matcher without also clearing or updating the RevisionName. Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

0xFelix · 2023-03-22T08:29:37Z

Thanks!

/lgtm
/retest

kubevirt-bot · 2023-03-22T10:31:22Z

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

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-fossa	`0bad7c2`	link	false	`/test pull-kubevirt-fossa`

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

lyarwood · 2023-03-22T13:01:38Z

/retest-required

kubevirt-commenter-bot · 2023-03-22T16:42:43Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2023-03-22T22:45:44Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot · 2023-03-23T04:59:26Z

@lyarwood: new pull request created: #9500

In response to this:

/cherrypick release-0.59

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.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. area/instancetype dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 13, 2023

kubevirt-bot requested review from EdDev and maiqueb March 13, 2023 16:18

kubevirt-bot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L labels Mar 13, 2023

0xFelix reviewed Mar 14, 2023

View reviewed changes

lyarwood force-pushed the bug-9132 branch from 68a4428 to a091e32 Compare March 14, 2023 15:51

0xFelix approved these changes Mar 15, 2023

View reviewed changes

kubevirt-bot assigned 0xFelix Mar 15, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2023

lyarwood changed the title ~~instancetype: Reject Matcher updates without clearing RevisionName~~ instancetype: Reject Matcher updates without updating RevisionName Mar 16, 2023

xpivarc reviewed Mar 16, 2023

View reviewed changes

xpivarc reviewed Mar 21, 2023

View reviewed changes

lyarwood force-pushed the bug-9132 branch from a091e32 to 4d3855f Compare March 21, 2023 15:09

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2023

xpivarc reviewed Mar 21, 2023

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 21, 2023

0xFelix reviewed Mar 21, 2023

View reviewed changes

lyarwood force-pushed the bug-9132 branch from 4d3855f to 625dfd1 Compare March 21, 2023 15:43

lyarwood force-pushed the bug-9132 branch from 625dfd1 to 0bad7c2 Compare March 21, 2023 16:08

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2023

kubevirt-bot merged commit 93366a8 into kubevirt:main Mar 23, 2023

kubevirt-bot mentioned this pull request Mar 23, 2023

[release-0.59] instancetype: Reject Matcher updates without updating RevisionName #9500

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

instancetype: Reject Matcher updates without updating RevisionName #9421

instancetype: Reject Matcher updates without updating RevisionName #9421

lyarwood commented Mar 13, 2023 •

edited

0xFelix Mar 14, 2023

lyarwood commented Mar 15, 2023

0xFelix left a comment

lyarwood commented Mar 16, 2023

kubevirt-bot commented Mar 16, 2023

xpivarc Mar 16, 2023

lyarwood Mar 16, 2023

xpivarc Mar 21, 2023

lyarwood Mar 21, 2023

lyarwood Mar 21, 2023

xpivarc Mar 16, 2023

lyarwood Mar 16, 2023

xpivarc Mar 21, 2023

lyarwood Mar 21, 2023

xpivarc left a comment

xpivarc Mar 21, 2023

xpivarc Mar 21, 2023

xpivarc left a comment

kubevirt-bot commented Mar 21, 2023

0xFelix Mar 21, 2023

lyarwood Mar 21, 2023 •

edited

0xFelix Mar 21, 2023

lyarwood Mar 21, 2023 •

edited

0xFelix commented Mar 22, 2023

kubevirt-bot commented Mar 22, 2023 •

edited

lyarwood commented Mar 22, 2023

kubevirt-commenter-bot commented Mar 22, 2023

kubevirt-commenter-bot commented Mar 22, 2023

kubevirt-bot commented Mar 23, 2023

	preferenceSpec, err := mutator.InstancetypeMethods.FindPreferenceSpec(vm)
	if err != nil {
	// Log but ultimately swallow any preference lookup errors here and let the validating webhook handle them
	log.Log.Reason(err).Error("Ignoring error attempting to lookup PreferredMachineType.")
	return nil
	}

instancetype: Reject Matcher updates without updating RevisionName #9421

instancetype: Reject Matcher updates without updating RevisionName #9421

Conversation

lyarwood commented Mar 13, 2023 • edited

Choose a reason for hiding this comment

lyarwood commented Mar 15, 2023

0xFelix left a comment

Choose a reason for hiding this comment

lyarwood commented Mar 16, 2023

kubevirt-bot commented Mar 16, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpivarc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpivarc left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Mar 21, 2023

Choose a reason for hiding this comment

lyarwood Mar 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyarwood Mar 21, 2023 • edited

Choose a reason for hiding this comment

0xFelix commented Mar 22, 2023

kubevirt-bot commented Mar 22, 2023 • edited

lyarwood commented Mar 22, 2023

kubevirt-commenter-bot commented Mar 22, 2023

kubevirt-commenter-bot commented Mar 22, 2023

kubevirt-bot commented Mar 23, 2023

lyarwood commented Mar 13, 2023 •

edited

lyarwood Mar 21, 2023 •

edited

lyarwood Mar 21, 2023 •

edited

kubevirt-bot commented Mar 22, 2023 •

edited