diff --git a/pkg/registry/certificates/certificates/declarative_validation_test.go b/pkg/registry/certificates/certificates/declarative_validation_test.go index 39a5e178125..c7bfc196d40 100644 --- a/pkg/registry/certificates/certificates/declarative_validation_test.go +++ b/pkg/registry/certificates/certificates/declarative_validation_test.go @@ -17,8 +17,10 @@ limitations under the License. package certificates import ( + "context" "testing" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -30,6 +32,11 @@ import ( var apiVersions = []string{"v1", "v1alpha1", "v1beta1"} +type validationStrategy interface { + Validate(ctx context.Context, obj runtime.Object) field.ErrorList + ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList +} + func TestDeclarativeValidateForDeclarative(t *testing.T) { for _, apiVersion := range apiVersions { testDeclarativeValidateForDeclarative(t, apiVersion) @@ -89,64 +96,87 @@ func TestValidateUpdateForDeclarative(t *testing.T) { } func testValidateUpdateForDeclarative(t *testing.T, apiVersion string) { - ctx := genericapirequest.WithRequestInfo(genericapirequest.NewDefaultContext(), &genericapirequest.RequestInfo{ - APIGroup: "certificates.k8s.io", - APIVersion: apiVersion, - }) testCases := map[string]struct { old api.CertificateSigningRequest update api.CertificateSigningRequest expectedErrs field.ErrorList + subresources []string }{ // TODO: Add test cases } for k, tc := range testCases { - t.Run(k, func(t *testing.T) { - tc.old.ResourceVersion = "1" - tc.update.ResourceVersion = "1" - var declarativeTakeoverErrs field.ErrorList - var imperativeErrs field.ErrorList - for _, gateVal := range []bool{true, false} { - // We only need to test both gate enabled and disabled together, because - // 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled. - // 2) the validation output, when only DeclarativeValidation is enabled, is the same as when both gates are disabled. - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidation, gateVal) - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidationTakeover, gateVal) - errs := Strategy.ValidateUpdate(ctx, &tc.update, &tc.old) - if gateVal { - declarativeTakeoverErrs = errs - } else { - imperativeErrs = errs + for _, subresource := range tc.subresources { + t.Run(k+" subresource="+subresource, func(t *testing.T) { + ctx := createContextForSubresource(apiVersion, subresource) + var strategy validationStrategy + switch subresource { + case "/": + strategy = Strategy + case "/approval": + strategy = ApprovalStrategy + case "/status": + strategy = StatusStrategy } - // The errOutputMatcher is used to verify the output matches the expected errors in test cases. - errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() - if len(tc.expectedErrs) > 0 { - errOutputMatcher.Test(t, tc.expectedErrs, errs) - } else if len(errs) != 0 { - t.Errorf("expected no errors, but got: %v", errs) - } - } - // The equivalenceMatcher is used to verify the output errors from hand-written imperative validation - // are equivalent to the output errors when DeclarativeValidationTakeover is enabled. - equivalenceMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() - // TODO: remove this once ErrorMatcher has been extended to handle this form of deduplication. - dedupedImperativeErrs := field.ErrorList{} - for _, err := range imperativeErrs { - found := false - for _, existingErr := range dedupedImperativeErrs { - if equivalenceMatcher.Matches(existingErr, err) { - found = true - break + tc.old.ResourceVersion = "1" + tc.update.ResourceVersion = "1" + var declarativeTakeoverErrs field.ErrorList + var imperativeErrs field.ErrorList + for _, gateVal := range []bool{true, false} { + // We only need to test both gate enabled and disabled together, because + // 1) the DeclarativeValidationTakeover won't take effect if DeclarativeValidation is disabled. + // 2) the validation output, when only DeclarativeValidation is enabled, is the same as when both gates are disabled. + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidation, gateVal) + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DeclarativeValidationTakeover, gateVal) + errs := strategy.ValidateUpdate(ctx, &tc.update, &tc.old) + if gateVal { + declarativeTakeoverErrs = errs + } else { + imperativeErrs = errs + } + // The errOutputMatcher is used to verify the output matches the expected errors in test cases. + errOutputMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() + + if len(tc.expectedErrs) > 0 { + errOutputMatcher.Test(t, tc.expectedErrs, errs) + } else if len(errs) != 0 { + t.Errorf("expected no errors, but got: %v", errs) } } - if !found { - dedupedImperativeErrs = append(dedupedImperativeErrs, err) + // The equivalenceMatcher is used to verify the output errors from hand-written imperative validation + // are equivalent to the output errors when DeclarativeValidationTakeover is enabled. + equivalenceMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin() + // TODO: remove this once ErrorMatcher has been extended to handle this form of deduplication. + dedupedImperativeErrs := field.ErrorList{} + for _, err := range imperativeErrs { + found := false + for _, existingErr := range dedupedImperativeErrs { + if equivalenceMatcher.Matches(existingErr, err) { + found = true + break + } + } + if !found { + dedupedImperativeErrs = append(dedupedImperativeErrs, err) + } } - } - equivalenceMatcher.Test(t, dedupedImperativeErrs, declarativeTakeoverErrs) + equivalenceMatcher.Test(t, dedupedImperativeErrs, declarativeTakeoverErrs) - apitesting.VerifyVersionedValidationEquivalence(t, &tc.update, &tc.old) - }) + apitesting.VerifyVersionedValidationEquivalence(t, &tc.update, &tc.old) + }) + } } } + +func createContextForSubresource(apiVersion, subresource string) context.Context { + requestInfo := &genericapirequest.RequestInfo{ + APIGroup: "certificates.k8s.io", + APIVersion: apiVersion, + } + + if subresource != "/" { + requestInfo.Subresource = subresource[1:] // Remove leading "/" + } + + return genericapirequest.WithRequestInfo(genericapirequest.NewDefaultContext(), requestInfo) +} diff --git a/pkg/registry/certificates/certificates/strategy.go b/pkg/registry/certificates/certificates/strategy.go index 318c4760578..fde7fa70fe3 100644 --- a/pkg/registry/certificates/certificates/strategy.go +++ b/pkg/registry/certificates/certificates/strategy.go @@ -277,7 +277,25 @@ func populateConditionTimestamps(newCSR, oldCSR *certificates.CertificateSigning } func (csrStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { - return validation.ValidateCertificateSigningRequestStatusUpdate(obj.(*certificates.CertificateSigningRequest), old.(*certificates.CertificateSigningRequest)) + newCSR := obj.(*certificates.CertificateSigningRequest) + oldCSR := old.(*certificates.CertificateSigningRequest) + errs := validation.ValidateCertificateSigningRequestStatusUpdate(newCSR, oldCSR) + if utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidation) { + // Determine if takeover is enabled + takeover := utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidationTakeover) + + // Run declarative update validation with panic recovery + declarativeErrs := rest.ValidateUpdateDeclaratively(ctx, legacyscheme.Scheme, newCSR, oldCSR, rest.WithTakeover(takeover)) + + // Compare imperative and declarative errors and emit metric if there's a mismatch + rest.CompareDeclarativeErrorsAndEmitMismatches(ctx, errs, declarativeErrs, takeover) + + // Only apply declarative errors if takeover is enabled + if takeover { + errs = append(errs.RemoveCoveredByDeclarative(), declarativeErrs...) + } + } + return errs } // WarningsOnUpdate returns warnings for the given update. @@ -329,7 +347,25 @@ func (csrApprovalStrategy) PrepareForUpdate(ctx context.Context, obj, old runtim } func (csrApprovalStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { - return validation.ValidateCertificateSigningRequestApprovalUpdate(obj.(*certificates.CertificateSigningRequest), old.(*certificates.CertificateSigningRequest)) + newCSR := obj.(*certificates.CertificateSigningRequest) + oldCSR := old.(*certificates.CertificateSigningRequest) + errs := validation.ValidateCertificateSigningRequestApprovalUpdate(newCSR, oldCSR) + if utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidation) { + // Determine if takeover is enabled + takeover := utilfeature.DefaultFeatureGate.Enabled(features.DeclarativeValidationTakeover) + + // Run declarative update validation with panic recovery + declarativeErrs := rest.ValidateUpdateDeclaratively(ctx, legacyscheme.Scheme, newCSR, oldCSR, rest.WithTakeover(takeover)) + + // Compare imperative and declarative errors and emit metric if there's a mismatch + rest.CompareDeclarativeErrorsAndEmitMismatches(ctx, errs, declarativeErrs, takeover) + + // Only apply declarative errors if takeover is enabled + if takeover { + errs = append(errs.RemoveCoveredByDeclarative(), declarativeErrs...) + } + } + return errs } // WarningsOnUpdate returns warnings for the given update.