diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go index 9e9013fd737..8f2c3864501 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go @@ -166,9 +166,9 @@ func parseSubresourcePath(subresourcePath string) ([]string, error) { // compareDeclarativeErrorsAndEmitMismatches checks for mismatches between imperative and declarative validation // and logs + emits metrics when inconsistencies are found -func compareDeclarativeErrorsAndEmitMismatches(ctx context.Context, imperativeErrs, declarativeErrs field.ErrorList, takeover bool, validationIdentifier string, normalizationRules []field.NormalizationRule) { +func compareDeclarativeErrorsAndEmitMismatches(ctx context.Context, imperativeErrs, declarativeErrs field.ErrorList, enforced bool, validationIdentifier string, normalizationRules []field.NormalizationRule) { logger := klog.FromContext(ctx) - mismatchDetails := gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs, takeover, normalizationRules) + mismatchDetails := gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs, enforced, normalizationRules) for _, detail := range mismatchDetails { // Log information about the mismatch using contextual logger logger.Error(nil, detail) @@ -180,17 +180,23 @@ func compareDeclarativeErrorsAndEmitMismatches(ctx context.Context, imperativeEr // gatherDeclarativeValidationMismatches compares imperative and declarative validation errors // and returns detailed information about any mismatches found. Errors are compared via type, field, and origin -func gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs field.ErrorList, takeover bool, normalizationRules []field.NormalizationRule) []string { +func gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs field.ErrorList, enforced bool, normalizationRules []field.NormalizationRule) []string { var mismatchDetails []string // short circuit here to minimize allocs for usual case of 0 validation errors if len(imperativeErrs) == 0 && len(declarativeErrs) == 0 { return mismatchDetails } - // recommendation based on takeover status - recommendation := "This difference should not affect system operation since hand written validation is authoritative." - if takeover { - recommendation = "Consider disabling the DeclarativeValidationTakeover feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes." + // default recommendation based on enforcement status + const ( + authoritativeMsg = "This difference should not affect system operation since hand written validation is authoritative." + disableBetaMsg = "Consider disabling the DeclarativeValidationBeta feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes." + ) + + defaultRecommendation := authoritativeMsg + if enforced { + defaultRecommendation = disableBetaMsg } + fuzzyMatcher := field.ErrorMatcher{}.ByType().ByOrigin().RequireOriginWhenInvalid().ByFieldNormalized(normalizationRules) exactMatcher := field.ErrorMatcher{}.Exactly() @@ -244,12 +250,18 @@ func gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs field } if matchCount == 0 { + rec := defaultRecommendation + // If the imperative error is explicitly Alpha, it is never enforced, so HV is authoritative. + if iErr.ValidationStabilityLevel.String() == "alpha" { + rec = authoritativeMsg + } + mismatchDetails = append(mismatchDetails, fmt.Sprintf( "Unexpected difference between hand written validation and declarative validation error results, unmatched error(s) found %s. "+ "This indicates an issue with declarative validation. %s", fuzzyMatcher.Render(iErr), - recommendation, + rec, ), ) } @@ -259,12 +271,18 @@ func gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs field // Any remaining unmatched declarative errors are considered "extra" for _, dErr := range remaining { + rec := defaultRecommendation + // If the declarative error is Alpha, it is never enforced (shadowed), so HV is authoritative. + if dErr.ValidationStabilityLevel.String() == "alpha" { + rec = authoritativeMsg + } + mismatchDetails = append(mismatchDetails, fmt.Sprintf( "Unexpected difference between hand written validation and declarative validation error results, extra error(s) found %s. "+ "This indicates an issue with declarative validation. %s", fuzzyMatcher.Render(dErr), - recommendation, + rec, ), ) } diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go index ef145dd94a3..51f52951456 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/validate_test.go @@ -210,184 +210,147 @@ func (p Pod) DeepCopyObject() runtime.Object { // scenarios across imperative and declarative errors for // the gatherDeclarativeValidationMismatches function func TestGatherDeclarativeValidationMismatches(t *testing.T) { - replicasPath := field.NewPath("spec").Child("replicas") - minReadySecondsPath := field.NewPath("spec").Child("minReadySeconds") - selectorPath := field.NewPath("spec").Child("selector") + pathStandard := field.NewPath("spec", "standard") + pathBeta := field.NewPath("spec", "beta") + pathAlpha := field.NewPath("spec", "alpha") + pathOther := field.NewPath("spec", "other") - errA := field.Invalid(replicasPath, nil, "regular error A") - errB := field.Invalid(minReadySecondsPath, -1, "covered error B").WithOrigin("minimum") - coveredErrB := field.Invalid(minReadySecondsPath, -1, "covered error B").WithOrigin("minimum") - errBWithDiffDetail := field.Invalid(minReadySecondsPath, -1, "covered error B - different detail").WithOrigin("minimum") - errBWithDiffPath := field.Invalid(field.NewPath("spec").Child("fakeminReadySeconds"), -1, "covered error B").WithOrigin("minimum") - coveredErrB.CoveredByDeclarative = true - errC := field.Invalid(replicasPath, nil, "covered error C").WithOrigin("minimum") - coveredErrC := field.Invalid(replicasPath, nil, "covered error C").WithOrigin("minimum") - coveredErrC.CoveredByDeclarative = true - errCWithDiffOrigin := field.Invalid(replicasPath, nil, "covered error C").WithOrigin("maximum") - errD := field.Invalid(selectorPath, nil, "regular error D") + // Standard Errors + errHVStandard := field.Invalid(pathStandard, "val", "impStandard").MarkCoveredByDeclarative().WithOrigin("min") + errDVStandard := field.Invalid(pathStandard, "val", "decStandard").WithOrigin("min") + + // Beta Errors + errHVBeta := field.Invalid(pathBeta, "val", "impBeta").MarkCoveredByDeclarative().MarkBeta().WithOrigin("min") + + // Alpha Errors + errDVAlpha := field.Invalid(pathAlpha, "val", "decAlpha").MarkAlpha().WithOrigin("min") + // Note: No HV Alpha by convention, so if it exists it would be a mismatch if DV is missing. + // But we usually don't mark HV as covered for Alpha rules. + + // Normalization / Fuzzy match helpers + errDVStandardDiffDetail := field.Invalid(pathStandard, "val", "decStandardDiffDetail").WithOrigin("min") + errDVStandardDiffPath := field.Invalid(field.NewPath("spec", "standardAliased"), "val", "decStandard").WithOrigin("min") + + const ( + authoritativeMsg = "This difference should not affect system operation since hand written validation is authoritative." + disableBetaMsg = "Consider disabling the DeclarativeValidationBeta feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes." + ) testCases := []struct { name string imperativeErrors field.ErrorList declarativeErrors field.ErrorList - takeover bool + enforced bool expectMismatches bool expectDetailsContaining []string normalizedRules []field.NormalizationRule }{ { - name: "Declarative and imperative return 0 errors - no mismatch", - imperativeErrors: field.ErrorList{}, - declarativeErrors: field.ErrorList{}, - takeover: false, - expectMismatches: false, - expectDetailsContaining: []string{}, - }, - { - name: "Declarative returns multiple errors with different origins, errors match - no mismatch", - imperativeErrors: field.ErrorList{ - errA, - coveredErrB, - coveredErrC, - errD, - }, - declarativeErrors: field.ErrorList{ - errB, - errC, - }, - takeover: false, - expectMismatches: false, - expectDetailsContaining: []string{}, - }, - { - name: "Declarative returns multiple errors with different origins, errors don't match - mismatch case", - imperativeErrors: field.ErrorList{ - errA, - coveredErrB, - coveredErrC, - }, - declarativeErrors: field.ErrorList{ - errB, - errCWithDiffOrigin, - }, - takeover: true, - expectMismatches: true, - expectDetailsContaining: []string{ - "Unexpected difference between hand written validation and declarative validation error results", - "unmatched error(s) found", - "extra error(s) found", - "replicas", - "Consider disabling the DeclarativeValidationTakeover feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes", - }, - }, - { - name: "Declarative and imperative return exactly 1 error, errors match - no mismatch", - imperativeErrors: field.ErrorList{ - coveredErrB, - }, - declarativeErrors: field.ErrorList{ - errB, - }, - takeover: false, - expectMismatches: false, - expectDetailsContaining: []string{}, - }, - { - name: "Declarative and imperative exactly 1 error, errors don't match - mismatch", - imperativeErrors: field.ErrorList{ - coveredErrB, - }, - declarativeErrors: field.ErrorList{ - errC, - }, - takeover: false, - expectMismatches: true, - expectDetailsContaining: []string{ - "Unexpected difference between hand written validation and declarative validation error results", - "unmatched error(s) found", - "minReadySeconds", - "extra error(s) found", - "replicas", - "This difference should not affect system operation since hand written validation is authoritative", - }, - }, - { - name: "Declarative returns 0 errors, imperative returns 1 covered error - mismatch", - imperativeErrors: field.ErrorList{ - coveredErrB, - }, + name: "No errors - no mismatch", + imperativeErrors: field.ErrorList{}, declarativeErrors: field.ErrorList{}, - takeover: true, + expectMismatches: false, + }, + { + name: "Clean match - no mismatch", + imperativeErrors: field.ErrorList{errHVStandard}, + declarativeErrors: field.ErrorList{errDVStandard}, + expectMismatches: false, + }, + { + name: "Mismatch: Missing DV (Standard) + Enforced -> Disable Beta Gate", + imperativeErrors: field.ErrorList{errHVStandard}, + declarativeErrors: field.ErrorList{}, + enforced: true, expectMismatches: true, expectDetailsContaining: []string{ - "Unexpected difference between hand written validation and declarative validation error results", "unmatched error(s) found", - "minReadySeconds", - "Consider disabling the DeclarativeValidationTakeover feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes", + "spec.standard", + disableBetaMsg, }, }, { - name: "Declarative returns 0 errors, imperative returns 1 uncovered error - no mismatch", - imperativeErrors: field.ErrorList{ - errB, - }, - declarativeErrors: field.ErrorList{}, - takeover: false, - expectMismatches: false, - expectDetailsContaining: []string{}, - }, - { - name: "Declarative returns 1 error, imperative returns 0 error - mismatch", - imperativeErrors: field.ErrorList{}, - declarativeErrors: field.ErrorList{ - errB, - }, - takeover: false, - expectMismatches: true, + name: "Mismatch: Extra DV (Standard) + Not Enforced -> HV Authoritative", + imperativeErrors: field.ErrorList{}, + declarativeErrors: field.ErrorList{errDVStandard}, + enforced: false, + expectMismatches: true, expectDetailsContaining: []string{ - "Unexpected difference between hand written validation and declarative validation error results", "extra error(s) found", - "minReadySeconds", - "This difference should not affect system operation since hand written validation is authoritative", + "spec.standard", + authoritativeMsg, }, }, { - name: "Declarative returns 1 error, imperative returns 3 matching errors - no mismatch", - imperativeErrors: field.ErrorList{ - coveredErrB, + name: "Mismatch: Missing DV (Beta) + Enforced -> Disable Beta Gate", + imperativeErrors: field.ErrorList{errHVBeta}, + declarativeErrors: field.ErrorList{}, + enforced: true, + expectMismatches: true, + expectDetailsContaining: []string{ + "unmatched error(s) found", + "spec.beta", + disableBetaMsg, }, - declarativeErrors: field.ErrorList{ - errB, - errB, - errBWithDiffDetail, - }, - takeover: false, - expectMismatches: false, - expectDetailsContaining: []string{}, }, { - name: "Field normalization, errors don't match - mismatch", - imperativeErrors: field.ErrorList{ - coveredErrB, - }, - declarativeErrors: field.ErrorList{ - errBWithDiffPath, + name: "Mismatch: Extra DV (Alpha) + Enforced -> HV Authoritative (Override)", + imperativeErrors: field.ErrorList{}, + declarativeErrors: field.ErrorList{errDVAlpha}, + enforced: true, + expectMismatches: true, + expectDetailsContaining: []string{ + "extra error(s) found", + "spec.alpha", + authoritativeMsg, }, + }, + { + name: "Fuzzy matching (different detail) - no mismatch", + imperativeErrors: field.ErrorList{errHVStandard}, + declarativeErrors: field.ErrorList{errDVStandardDiffDetail}, + expectMismatches: false, + }, + { + name: "Field normalization - no mismatch", + imperativeErrors: field.ErrorList{errHVStandard}, + declarativeErrors: field.ErrorList{errDVStandardDiffPath}, normalizedRules: []field.NormalizationRule{ { - Regexp: regexp.MustCompile(`spec.fakeminReadySeconds`), - Replacement: "spec.minReadySeconds", + Regexp: regexp.MustCompile(`spec.standardAliased`), + Replacement: "spec.standard", }, }, - takeover: false, - expectMismatches: false, - expectDetailsContaining: []string{}, + expectMismatches: false, + }, + { + name: "Multiple mismatches - combined log info", + imperativeErrors: field.ErrorList{errHVBeta}, + declarativeErrors: field.ErrorList{errDVAlpha}, + enforced: true, + expectMismatches: true, + expectDetailsContaining: []string{ + "unmatched error(s) found", + "spec.beta", + disableBetaMsg, + "extra error(s) found", + "spec.alpha", + authoritativeMsg, + }, + }, + { + name: "Uncovered HV error - no mismatch (ignored)", + imperativeErrors: field.ErrorList{ + field.Invalid(pathOther, "val", "other").WithOrigin("min"), + }, + declarativeErrors: field.ErrorList{}, + expectMismatches: false, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - details := gatherDeclarativeValidationMismatches(tc.imperativeErrors, tc.declarativeErrors, tc.takeover, tc.normalizedRules) + details := gatherDeclarativeValidationMismatches(tc.imperativeErrors, tc.declarativeErrors, tc.enforced, tc.normalizedRules) // Check if mismatches were found if expected if tc.expectMismatches && len(details) == 0 { t.Errorf("Expected mismatches but got none") @@ -423,7 +386,7 @@ func TestCompareDeclarativeErrorsAndEmitMismatches(t *testing.T) { name string imperativeErrs field.ErrorList declarativeErrs field.ErrorList - takeover bool + enforced bool expectLogs bool expectedRegex string }{ @@ -431,16 +394,16 @@ func TestCompareDeclarativeErrorsAndEmitMismatches(t *testing.T) { name: "mismatched errors, log info", imperativeErrs: field.ErrorList{coveredErrB}, declarativeErrs: field.ErrorList{errA}, - takeover: true, + enforced: true, expectLogs: true, // logs have a prefix of the form - E0309 21:05:33.865030 1926106 validate.go:199] - expectedRegex: "E.*Unexpected difference between hand written validation and declarative validation error results.*Consider disabling the DeclarativeValidationTakeover feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes", + expectedRegex: "E.*Unexpected difference between hand written validation and declarative validation error results.*Consider disabling the DeclarativeValidationBeta feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes", }, { name: "matching errors, don't log info", imperativeErrs: field.ErrorList{coveredErrB}, declarativeErrs: field.ErrorList{errB}, - takeover: true, + enforced: true, expectLogs: false, expectedRegex: "", }, @@ -454,7 +417,7 @@ func TestCompareDeclarativeErrorsAndEmitMismatches(t *testing.T) { defer klog.LogToStderr(true) ctx := context.Background() - compareDeclarativeErrorsAndEmitMismatches(ctx, tc.imperativeErrs, tc.declarativeErrs, tc.takeover, "test_validationIdentifier", nil) + compareDeclarativeErrorsAndEmitMismatches(ctx, tc.imperativeErrs, tc.declarativeErrs, tc.enforced, "test_validationIdentifier", nil) klog.Flush() logOutput := buf.String()