Merge pull request #129506 from JoelSpeed/fix-status-ratcheting

Fix CRD status subresource ratcheting
This commit is contained in:
Kubernetes Prow Robot 2025-01-13 05:06:32 -08:00 committed by GitHub
commit 728a4d2a48
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 172 additions and 99 deletions

View File

@ -22,11 +22,17 @@ import (
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel"
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model"
structurallisttype "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype"
"k8s.io/apiextensions-apiserver/pkg/apiserver/validation"
apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
celconfig "k8s.io/apiserver/pkg/apis/cel"
"k8s.io/apiserver/pkg/cel/common"
utilfeature "k8s.io/apiserver/pkg/util/feature"
)
type statusStrategy struct {
@ -94,8 +100,17 @@ func (a statusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Obj
return field.ErrorList{field.Invalid(field.NewPath(""), old, fmt.Sprintf("has type %T. Must be a pointer to an Unstructured type", old))}
}
var options []validation.ValidationOption
var celOptions []cel.Option
var correlatedObject *common.CorrelatedObject
if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) {
correlatedObject = common.NewCorrelatedObject(uNew.Object, uOld.Object, &model.Structural{Structural: a.structuralSchema})
options = append(options, validation.WithRatcheting(correlatedObject.Key("status")))
celOptions = append(celOptions, cel.WithRatcheting(correlatedObject))
}
var errs field.ErrorList
errs = append(errs, a.customResourceStrategy.validator.ValidateStatusUpdate(ctx, uNew, uOld, a.scale)...)
errs = append(errs, a.customResourceStrategy.validator.ValidateStatusUpdate(ctx, uNew, uOld, a.scale, options...)...)
// ratcheting validation of x-kubernetes-list-type value map and set
if newErrs := structurallisttype.ValidateListSetsAndMaps(nil, a.structuralSchema, uNew.Object); len(newErrs) > 0 {
@ -109,10 +124,15 @@ func (a statusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Obj
if has, err := hasBlockingErr(errs); has {
errs = append(errs, err)
} else {
err, _ := celValidator.Validate(ctx, nil, a.customResourceStrategy.structuralSchema, uNew.Object, uOld.Object, celconfig.RuntimeCELCostBudget)
err, _ := celValidator.Validate(ctx, nil, a.customResourceStrategy.structuralSchema, uNew.Object, uOld.Object, celconfig.RuntimeCELCostBudget, celOptions...)
errs = append(errs, err...)
}
}
// No-op if not attached to context
if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) {
validation.Metrics.ObserveRatchetingTime(*correlatedObject.Duration)
}
return errs
}

View File

@ -96,7 +96,7 @@ func validateKubeFinalizerName(stringValue string, fldPath *field.Path) []string
return allWarnings
}
func (a customResourceValidator) ValidateStatusUpdate(ctx context.Context, obj, old *unstructured.Unstructured, scale *apiextensions.CustomResourceSubresourceScale) field.ErrorList {
func (a customResourceValidator) ValidateStatusUpdate(ctx context.Context, obj, old *unstructured.Unstructured, scale *apiextensions.CustomResourceSubresourceScale, options ...apiextensionsvalidation.ValidationOption) field.ErrorList {
if errs := a.ValidateTypeMeta(ctx, obj); len(errs) > 0 {
return errs
}
@ -105,7 +105,7 @@ func (a customResourceValidator) ValidateStatusUpdate(ctx context.Context, obj,
allErrs = append(allErrs, validation.ValidateObjectMetaAccessorUpdate(obj, old, field.NewPath("metadata"))...)
if status, hasStatus := obj.UnstructuredContent()["status"]; hasStatus {
allErrs = append(allErrs, apiextensionsvalidation.ValidateCustomResourceUpdate(field.NewPath("status"), status, old.UnstructuredContent()["status"], a.statusSchemaValidator)...)
allErrs = append(allErrs, apiextensionsvalidation.ValidateCustomResourceUpdate(field.NewPath("status"), status, old.UnstructuredContent()["status"], a.statusSchemaValidator, options...)...)
}
allErrs = append(allErrs, a.ValidateScaleStatus(ctx, obj, scale)...)

View File

@ -81,6 +81,7 @@ type ratchetingTestContext struct {
*testing.T
DynamicClient dynamic.Interface
APIExtensionsClient clientset.Interface
StatusSubresource bool
}
type ratchetingTestOperation interface {
@ -164,7 +165,7 @@ func (a applyPatchOperation) Do(ctx *ratchetingTestContext) error {
patch := &unstructured.Unstructured{}
if obj, ok := a.patch.(map[string]interface{}); ok {
patch.Object = obj
patch.Object = runtime.DeepCopyJSON(obj)
} else if str, ok := a.patch.(string); ok {
str = FixTabsOrDie(str)
if err := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(str), len(str)).Decode(&patch.Object); err != nil {
@ -174,24 +175,31 @@ func (a applyPatchOperation) Do(ctx *ratchetingTestContext) error {
return fmt.Errorf("invalid patch type: %T", a.patch)
}
if ctx.StatusSubresource {
patch.Object = map[string]interface{}{"status": patch.Object}
}
patch.SetKind(kind)
patch.SetAPIVersion(a.gvr.GroupVersion().String())
patch.SetName(a.name)
patch.SetNamespace("default")
_, err := ctx.DynamicClient.
Resource(a.gvr).
Namespace(patch.GetNamespace()).
Apply(
context.TODO(),
patch.GetName(),
patch,
metav1.ApplyOptions{
FieldManager: "manager",
})
c := ctx.DynamicClient.Resource(a.gvr).Namespace(patch.GetNamespace())
if ctx.StatusSubresource {
if _, err := c.Get(context.TODO(), patch.GetName(), metav1.GetOptions{}); apierrors.IsNotFound(err) {
// ApplyStatus will not automatically create an object, we must make sure it exists before we can
// apply the status to it.
_, err := c.Create(context.TODO(), patch, metav1.CreateOptions{})
if err != nil {
return err
}
}
_, err := c.ApplyStatus(context.TODO(), patch.GetName(), patch, metav1.ApplyOptions{FieldManager: "manager"})
return err
}
_, err := c.Apply(context.TODO(), patch.GetName(), patch, metav1.ApplyOptions{FieldManager: "manager"})
return err
}
func (a applyPatchOperation) Description() string {
@ -228,10 +236,20 @@ func (u updateMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error {
}},
}
if ctx.StatusSubresource {
sch = &apiextensionsv1.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextensionsv1.JSONSchemaProps{
"status": *sch,
},
}
}
for _, v := range myCRD.Spec.Versions {
if v.Name != myCRDV1Beta1.Version {
continue
}
v.Schema.OpenAPIV3Schema = sch
}
@ -282,8 +300,17 @@ type patchMyCRDV1Beta1Schema struct {
}
func (p patchMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error {
patch := p.patch
if ctx.StatusSubresource {
patch = map[string]interface{}{
"properties": map[string]interface{}{
"status": patch,
},
}
}
var err error
patchJSON, err := json.Marshal(p.patch)
patchJSON, err := json.Marshal(patch)
if err != nil {
return err
}
@ -315,7 +342,12 @@ func (p patchMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error {
return updateMyCRDV1Beta1Schema{
newSchema: &parsed,
}.Do(ctx)
}.Do(&ratchetingTestContext{
T: ctx.T,
DynamicClient: ctx.DynamicClient,
APIExtensionsClient: ctx.APIExtensionsClient,
StatusSubresource: false, // We have already handled the status subresource.
})
}
return fmt.Errorf("could not find version %v in CRD %v", myCRDV1Beta1.Version, myCRD.Name)
@ -329,6 +361,7 @@ type ratchetingTestCase struct {
Name string
Disabled bool
Operations []ratchetingTestOperation
SkipStatus bool
}
func runTests(t *testing.T, cases []ratchetingTestCase) {
@ -372,9 +405,15 @@ func runTests(t *testing.T, cases []ratchetingTestCase) {
},
},
},
"status": {
Type: "object",
},
},
},
},
Subresources: &apiextensionsv1.CustomResourceSubresources{
Status: &apiextensionsv1.CustomResourceSubresourceStatus{},
},
}},
Names: apiextensionsv1.CustomResourceDefinitionNames{
Plural: resource,
@ -394,13 +433,7 @@ func runTests(t *testing.T, cases []ratchetingTestCase) {
continue
}
t.Run(c.Name, func(t *testing.T) {
ctx := &ratchetingTestContext{
T: t,
DynamicClient: dynamicClient,
APIExtensionsClient: apiExtensionClient,
}
run := func(t *testing.T, ctx *ratchetingTestContext) {
for i, op := range c.Operations {
t.Logf("Performing Operation: %v", op.Description())
if err := op.Do(ctx); err != nil {
@ -413,7 +446,26 @@ func runTests(t *testing.T, cases []ratchetingTestCase) {
if err != nil {
t.Fatal(err)
}
}
t.Run(c.Name, func(t *testing.T) {
run(t, &ratchetingTestContext{
T: t,
DynamicClient: dynamicClient,
APIExtensionsClient: apiExtensionClient,
})
})
if !c.SkipStatus {
t.Run("Status: "+c.Name, func(t *testing.T) {
run(t, &ratchetingTestContext{
T: t,
DynamicClient: dynamicClient,
APIExtensionsClient: apiExtensionClient,
StatusSubresource: true,
})
})
}
}
}
@ -443,23 +495,23 @@ func TestRatchetingFunctionality(t *testing.T) {
myCRDV1Beta1,
myCRDInstanceName,
map[string]interface{}{
"hasMinimum": 0,
"hasMaximum": 1000,
"hasMinimumAndMaximum": 50,
"hasMinimum": int64(0),
"hasMaximum": int64(1000),
"hasMinimumAndMaximum": int64(50),
}},
patchMyCRDV1Beta1Schema{
"Add stricter minimums and maximums that violate the previous object",
map[string]interface{}{
"properties": map[string]interface{}{
"hasMinimum": map[string]interface{}{
"minimum": 10,
"minimum": int64(10),
},
"hasMaximum": map[string]interface{}{
"maximum": 20,
"maximum": int64(20),
},
"hasMinimumAndMaximum": map[string]interface{}{
"minimum": 10,
"maximum": 20,
"minimum": int64(10),
"maximum": int64(20),
},
"noRestrictions": map[string]interface{}{
"type": "integer",
@ -471,33 +523,33 @@ func TestRatchetingFunctionality(t *testing.T) {
myCRDV1Beta1,
myCRDInstanceName,
map[string]interface{}{
"noRestrictions": 50,
"noRestrictions": int64(50),
}},
expectError{
applyPatchOperation{
"Change a single old field to be invalid",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"hasMinimum": 5,
"hasMinimum": int64(5),
}},
},
expectError{
applyPatchOperation{
"Change multiple old fields to be invalid",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"hasMinimum": 5,
"hasMaximum": 21,
"hasMinimum": int64(5),
"hasMaximum": int64(21),
}},
},
applyPatchOperation{
"Change single old field to be valid",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"hasMinimum": 11,
"hasMinimum": int64(11),
}},
applyPatchOperation{
"Change multiple old fields to be valid",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"hasMaximum": 19,
"hasMinimumAndMaximum": 15,
"hasMaximum": int64(19),
"hasMinimumAndMaximum": int64(15),
}},
},
},
@ -576,8 +628,8 @@ func TestRatchetingFunctionality(t *testing.T) {
"Create an instance",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"nums": map[string]interface{}{
"num1": 1,
"num2": 1000000,
"num1": int64(1),
"num2": int64(1000000),
},
"content": map[string]interface{}{
"k1": "some content",
@ -590,7 +642,7 @@ func TestRatchetingFunctionality(t *testing.T) {
"properties": map[string]interface{}{
"nums": map[string]interface{}{
"additionalProperties": map[string]interface{}{
"minimum": 1000,
"minimum": int64(1000),
},
},
},
@ -599,16 +651,16 @@ func TestRatchetingFunctionality(t *testing.T) {
"updating validating field num2 to another validating value, but rachet invalid field num1",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"nums": map[string]interface{}{
"num1": 1,
"num2": 2000,
"num1": int64(1),
"num2": int64(2000),
},
}},
expectError{applyPatchOperation{
"update field num1 to different invalid value",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"nums": map[string]interface{}{
"num1": 2,
"num2": 2000,
"num1": int64(2),
"num2": int64(2000),
},
}}},
},
@ -646,8 +698,8 @@ func TestRatchetingFunctionality(t *testing.T) {
map[string]interface{}{
"properties": map[string]interface{}{
"restricted": map[string]interface{}{
"minProperties": 1,
"maxProperties": 1,
"minProperties": int64(1),
"maxProperties": int64(1),
},
},
}},
@ -679,7 +731,7 @@ func TestRatchetingFunctionality(t *testing.T) {
map[string]interface{}{
"properties": map[string]interface{}{
"restricted": map[string]interface{}{
"minProperties": 2,
"minProperties": int64(2),
"maxProperties": nil,
},
},
@ -709,7 +761,7 @@ func TestRatchetingFunctionality(t *testing.T) {
"properties": map[string]interface{}{
"restricted": map[string]interface{}{
"minProperties": nil,
"maxProperties": 1,
"maxProperties": int64(1),
},
},
}},
@ -763,7 +815,7 @@ func TestRatchetingFunctionality(t *testing.T) {
map[string]interface{}{
"properties": map[string]interface{}{
"array": map[string]interface{}{
"minItems": 10,
"minItems": int64(10),
},
},
}},
@ -825,7 +877,7 @@ func TestRatchetingFunctionality(t *testing.T) {
map[string]interface{}{
"properties": map[string]interface{}{
"array": map[string]interface{}{
"maxItems": 1,
"maxItems": int64(1),
},
},
}},
@ -884,10 +936,10 @@ func TestRatchetingFunctionality(t *testing.T) {
map[string]interface{}{
"properties": map[string]interface{}{
"minField": map[string]interface{}{
"minLength": 10,
"minLength": int64(10),
},
"maxField": map[string]interface{}{
"maxLength": 15,
"maxLength": int64(15),
},
},
}},
@ -1084,17 +1136,17 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": []interface{}{
map[string]interface{}{
"name": "nginx",
"port": 443,
"port": int64(443),
"field": "value",
},
map[string]interface{}{
"name": "etcd",
"port": 2379,
"port": int64(2379),
"field": "value",
},
map[string]interface{}{
"name": "kube-apiserver",
"port": 6443,
"port": int64(6443),
"field": "value",
},
},
@ -1104,7 +1156,7 @@ func TestRatchetingFunctionality(t *testing.T) {
map[string]interface{}{
"properties": map[string]interface{}{
"field": map[string]interface{}{
"maxItems": 2,
"maxItems": int64(2),
},
},
}},
@ -1114,17 +1166,17 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": []interface{}{
map[string]interface{}{
"name": "kube-apiserver",
"port": 6443,
"port": int64(6443),
"field": "value",
},
map[string]interface{}{
"name": "nginx",
"port": 443,
"port": int64(443),
"field": "value",
},
map[string]interface{}{
"name": "etcd",
"port": 2379,
"port": int64(2379),
"field": "value",
},
},
@ -1136,22 +1188,22 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": []interface{}{
map[string]interface{}{
"name": "kube-apiserver",
"port": 6443,
"port": int64(6443),
"field": "value",
},
map[string]interface{}{
"name": "nginx",
"port": 443,
"port": int64(443),
"field": "value",
},
map[string]interface{}{
"name": "etcd",
"port": 2379,
"port": int64(2379),
"field": "value",
},
map[string]interface{}{
"name": "dev",
"port": 8080,
"port": int64(8080),
"field": "value",
},
},
@ -1165,7 +1217,7 @@ func TestRatchetingFunctionality(t *testing.T) {
"items": map[string]interface{}{
"properties": map[string]interface{}{
"port": map[string]interface{}{
"multipleOf": 2,
"multipleOf": int64(2),
},
},
},
@ -1179,17 +1231,17 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": []interface{}{
map[string]interface{}{
"name": "nginx",
"port": 443,
"port": int64(443),
"field": "value",
},
map[string]interface{}{
"name": "etcd",
"port": 2379,
"port": int64(2379),
"field": "value",
},
map[string]interface{}{
"name": "kube-apiserver",
"port": 6443,
"port": int64(6443),
"field": "value",
},
},
@ -1201,22 +1253,22 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": []interface{}{
map[string]interface{}{
"name": "nginx",
"port": 443,
"port": int64(443),
"field": "value",
},
map[string]interface{}{
"name": "etcd",
"port": 2379,
"port": int64(2379),
"field": "value",
},
map[string]interface{}{
"name": "kube-apiserver",
"port": 6443,
"port": int64(6443),
"field": "this is a changed value for an an invalid but grandfathered key",
},
map[string]interface{}{
"name": "dev",
"port": 8080,
"port": int64(8080),
"field": "value",
},
},
@ -1259,7 +1311,7 @@ func TestRatchetingFunctionality(t *testing.T) {
"values": map[string]interface{}{
"items": map[string]interface{}{
"additionalProperties": map[string]interface{}{
"minLength": 6,
"minLength": int64(6),
},
},
},
@ -1330,7 +1382,8 @@ func TestRatchetingFunctionality(t *testing.T) {
},
},
{
Name: "CEL Optional OldSelf",
Name: "CEL Optional OldSelf",
SkipStatus: true, // oldSelf can never be null for a status update.
Operations: []ratchetingTestOperation{
updateMyCRDV1Beta1Schema{&apiextensionsv1.JSONSchemaProps{
Type: "object",
@ -1461,15 +1514,15 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": map[string]interface{}{
"object1": map[string]interface{}{
"stringField": "a string",
"intField": 5,
"intField": int64(5),
},
"object2": map[string]interface{}{
"stringField": "another string",
"intField": 15,
"intField": int64(15),
},
"object3": map[string]interface{}{
"stringField": "a third string",
"intField": 7,
"intField": int64(7),
},
},
}},
@ -1499,19 +1552,19 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": map[string]interface{}{
"object1": map[string]interface{}{
"stringField": "a string",
"intField": 5,
"intField": int64(5),
},
"object2": map[string]interface{}{
"stringField": "another string",
"intField": 15,
"intField": int64(15),
},
"object3": map[string]interface{}{
"stringField": "a third string",
"intField": 7,
"intField": int64(7),
},
"object4": map[string]interface{}{
"stringField": "k8s third string",
"intField": 7,
"intField": int64(7),
},
},
}},
@ -1521,12 +1574,12 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": map[string]interface{}{
"object1": map[string]interface{}{
"stringField": "a string",
"intField": 15,
"intField": int64(15),
},
"object2": map[string]interface{}{
"stringField": "another string",
"intField": 10,
"otherIntField": 20,
"intField": int64(10),
"otherIntField": int64(20),
},
},
}},
@ -1571,11 +1624,11 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": map[string]interface{}{
"object1": map[string]interface{}{
"stringField": "a string", // invalid. even number length, no k8s prefix
"intField": 1000,
"intField": int64(1000),
},
"object4": map[string]interface{}{
"stringField": "k8s third string", // invalid. even number length. ratcheted
"intField": 7000,
"intField": int64(7000),
},
},
}},
@ -1586,11 +1639,11 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": map[string]interface{}{
"object1": map[string]interface{}{
"stringField": "k8s third string",
"intField": 1000,
"intField": int64(1000),
},
"object4": map[string]interface{}{
"stringField": "a string",
"intField": 7000,
"intField": int64(7000),
},
},
}}},
@ -1600,11 +1653,11 @@ func TestRatchetingFunctionality(t *testing.T) {
"field": map[string]interface{}{
"object1": map[string]interface{}{
"stringField": "k8s a stringy",
"intField": 1000,
"intField": int64(1000),
},
"object4": map[string]interface{}{
"stringField": "k8s third stringy",
"intField": 7000,
"intField": int64(7000),
},
},
}},
@ -1643,7 +1696,7 @@ func TestRatchetingFunctionality(t *testing.T) {
"reate a list of numbers with duplicates using the old simple schema",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"values": map[string]interface{}{
"dups": []interface{}{1, 2, 2, 3, 1000, 2000},
"dups": []interface{}{int64(1), int64(2), int64(2), int64(3), int64(1000), int64(2000)},
},
}},
patchMyCRDV1Beta1Schema{
@ -1662,15 +1715,15 @@ func TestRatchetingFunctionality(t *testing.T) {
"change original without removing duplicates",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"values": map[string]interface{}{
"dups": []interface{}{1, 2, 2, 3, 1000, 2000, 3},
"dups": []interface{}{int64(1), int64(2), int64(2), int64(3), int64(1000), int64(2000), int64(3)},
},
}}},
expectError{applyPatchOperation{
"add another list with duplicates",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"values": map[string]interface{}{
"dups": []interface{}{1, 2, 2, 3, 1000, 2000},
"dups2": []interface{}{1, 2, 2, 3, 1000, 2000},
"dups": []interface{}{int64(1), int64(2), int64(2), int64(3), int64(1000), int64(2000)},
"dups2": []interface{}{int64(1), int64(2), int64(2), int64(3), int64(1000), int64(2000)},
},
}}},
// Can add a valid sibling field
@ -1680,8 +1733,8 @@ func TestRatchetingFunctionality(t *testing.T) {
"add a valid sibling field",
myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{
"values": map[string]interface{}{
"dups": []interface{}{1, 2, 2, 3, 1000, 2000},
"otherField": []interface{}{1, 2, 3},
"dups": []interface{}{int64(1), int64(2), int64(2), int64(3), int64(1000), int64(2000)},
"otherField": []interface{}{int64(1), int64(2), int64(3)},
},
}},
// Can remove dups to make valid
@ -1694,8 +1747,8 @@ func TestRatchetingFunctionality(t *testing.T) {
myCRDInstanceName,
map[string]interface{}{
"values": map[string]interface{}{
"dups": []interface{}{1, 3, 1000, 2000},
"otherField": []interface{}{1, 2, 3},
"dups": []interface{}{int64(1), int64(3), int64(1000), int64(2000)},
"otherField": []interface{}{int64(1), int64(2), int64(3)},
},
}},
},