Merge pull request #133068 from yongruilin/master_vg-csr-subresource

feat: Enabledeclarative validation in CertificateSigningRequest subresources updates
This commit is contained in:
Kubernetes Prow Robot 2025-07-21 11:58:33 -07:00 committed by GitHub
commit e84a6cb7ef
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 113 additions and 47 deletions

View file

@ -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)
}

View file

@ -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.