fix: remove unnecessary ValidateResourceClaim call from resourclaim ValidateUpdate

[aaron-prindle]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR modifies - pkg/registry/resource/resourceclaim/strategy.go so that ValidateUpdate no longer calls ValidateResourceClaim which is unnecessary as this is called on CREATE and then on UPDATE we do an immutability check where no field in the Spec can change. If no fields can change and we validated on CREATE then we shouldn't need to re-validate on update. Status fields are wiped for the root resource UPDATE call so this is correct logic for the root endpoint by just having spec be immutable.

A test change is needed here as previously this test:

		"drop-fields-prioritized-list": {
			oldObj:                obj,
			newObj:                objWithPrioritizedList,
			prioritizedList:       false,
			expectValidationError: deviceRequestError,
			verify: func(t *testing.T, as []testclient.Action) {
				if len(as) != 0 {
					t.Errorf("expected no action to be taken")
				}
			},
		},

triggered the fieldImmutableError error (changing spec and namespace) AND triggered the deviceRequestError - see debug logs here from the original test case before this PR:

    strategy_test.go:775: Error found: spec.devices.requests[0]: Required value: exactly one of `exactly` or `firstAvailable` is required
    strategy_test.go:775: Error found: metadata.namespace: Invalid value: "default": field is immutable
    strategy_test.go:775: Error found: spec: Invalid value: ...: field is immutable
    strategy_test.go:782: CONFIRMED: Namespace immutability error was present.

but with this change to strategy.go now only the immutability error would be present. As such the test was updated to check for fieldImmutableError

Which issue(s) this PR is related to:

Special notes for your reviewer:

Alternatively we could keep the ValidateResourceClaim call but have the ValidateResourceClaimUpdate occur first and short-circuit.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[aaron-prindle]

/cc @pohly

[aaron-prindle]

/retest

[pohly]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 1efb5179719d1d821f5dd6fb9a3d9f23b4b0b560

[aaron-prindle]

/assign @liggitt

[jpbetz]

Doesn't this drop the call to ValidateObjectMeta ?

If so the fact that this passed CI is deeply concerning...

[aaron-prindle]

validation.ValidateResourceClaimUpdate calls

• corevalidation.ValidateObjectMetaUpdate
• apimachineryvalidation.ValidateImmutableField

corevalidation.ValidateObjectMetaUpdate calls

• apimachineryvalidation.ValidateObjectMetaUpdate
• (and validateKubeFinalizerName over finalizers)

===

You are correct that this would make it so that Update no longer calls ValidateObjectMeta but it still calls ValidateObjectMetaUpdate, is ValidateObjectMeta necessary on non-CREATE calls?

ValidateObjectMeta comment states -

// ValidateObjectMeta validates an object's metadata on creation. It expects that name generation has already
// been performed.

(link to ValidateObjectMeta and ValidateObjectMetaUpdate source)
https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L399-L420

[aaron-prindle]

I can add this validation back directly as part of strategy.go, it is only the dupe ResourceClaim validations that are an issue. ObjectMeta is orthogonal to Spec so to make sure validation is still 1:1 we should likely still do this check. The only thing I was uncertain of is if ValidateObjectMeta was desired on non-CREATE operations.

[liggitt]

but it still calls ValidateObjectMetaUpdate

That's what makes it ok

[aaron-prindle]

To reduce the risk here, I've added the ValidateObjectMeta call back:

// ValidateResourceClaimUpdate tests if an update to ResourceClaim is valid.
func ValidateResourceClaimUpdate(resourceClaim, oldClaim *resource.ResourceClaim) field.ErrorList {
	allErrs := corevalidation.ValidateObjectMeta(&resourceClaim.ObjectMeta, true, corevalidation.ValidateResourceClaimName, field.NewPath("metadata"))
	allErrs = append(allErrs, corevalidation.ValidateObjectMetaUpdate(&resourceClaim.ObjectMeta, &oldClaim.ObjectMeta, field.NewPath("metadata"))...)
	// The spec is immutable. On update, we only check for immutability.
	// Re-validating other fields is skipped because the user cannot change them;
	// the only actionable error is for the immutability violation.
	allErrs = append(allErrs, apimachineryvalidation.ValidateImmutableField(resourceClaim.Spec, oldClaim.Spec, field.NewPath("spec"))...)
	return allErrs
}

now the only effective delta from this change is no longer calling:

	allErrs = append(allErrs, validateResourceClaimSpec(&resourceClaim.Spec, field.NewPath("spec"), false)...)

[aaron-prindle]

/retest

[aaron-prindle]

/retest

[aaron-prindle]

/retest

[aaron-prindle]

/sig api-machinery

[liggitt]

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 7c1466e12a45c590645caaae2909aec0ec383f74

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaron-prindle, liggitt, pohly

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:

• pkg/apis/OWNERS [liggitt]
• pkg/registry/OWNERS [liggitt]

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