Merge pull request #63587 from sttts/sttts-crd-test-conflict-errs

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

apiextensions: handle CRD conflict errs in integration tests

In the integration tests we assume that no other party modifies CRDs while the test is updating them repeatedly. Due to semantic changes for CRD conditions (https://github.com/kubernetes/kubernetes/pull/63068) and the introduction of `status.storedVersions` (https://github.com/kubernetes/kubernetes/pull/63518), this assumption will not hold true in the future. This PR prepares the test to handle conflict errors gracefully.

staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go

@@ -22,7 +22,6 @@ import (
 	"sort"
 	"strings"
 	"testing"
-	"time"
 
 	autoscaling "k8s.io/api/autoscaling/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -30,7 +29,6 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/wait"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing"
 	"k8s.io/client-go/dynamic"
@@ -396,9 +394,8 @@ func TestValidateOnlyStatus(t *testing.T) {
 	// UpdateStatus should validate only status
 	// 1. create a crd with max value of .spec.num = 10 and .status.num = 10
 	// 2. create a cr with .spec.num = 10 and .status.num = 10 (valid)
-	// 3. update the crd so that max value of .spec.num = 5 and .status.num = 10
+	// 3. update the spec of the cr with .spec.num = 15 (spec is invalid), expect no error
-	// 4. update the status of the cr with .status.num = 5 (spec is invalid)
+	// 4. update the spec of the cr with .spec.num = 15 (spec is invalid), expect error
-	// validation passes becauses spec is not validated
 
 	// max value of spec.num = 10 and status.num = 10
 	schema := &apiextensionsv1beta1.JSONSchemaProps{
@@ -447,58 +444,31 @@ func TestValidateOnlyStatus(t *testing.T) {
 		t.Fatalf("unable to create noxu instance: %v", err)
 	}
 
-	gottenCRD, err := apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Get("noxus.mygroup.example.com", metav1.GetOptions{})
+	// update the spec with .spec.num = 15, expecting no error
+	err = unstructured.SetNestedField(createdNoxuInstance.Object, int64(15), "spec", "num")
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("unexpected error setting .spec.num: %v", err)
 	}
-
+	createdNoxuInstance, err = noxuStatusResourceClient.Update(createdNoxuInstance)
-	// update the crd so that max value of spec.num = 5 and status.num = 10
-	gottenCRD.Spec.Validation.OpenAPIV3Schema = &apiextensionsv1beta1.JSONSchemaProps{
-		Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{
-			"spec": {
-				Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{
-					"num": {
-						Type:    "integer",
-						Maximum: float64Ptr(5),
-					},
-				},
-			},
-			"status": {
-				Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{
-					"num": {
-						Type:    "integer",
-						Maximum: float64Ptr(10),
-					},
-				},
-			},
-		},
-	}
-
-	if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(gottenCRD); err != nil {
-		t.Fatal(err)
-	}
-
-	// update the status with .status.num = 5
-	err = unstructured.SetNestedField(createdNoxuInstance.Object, int64(5), "status", "num")
 	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
+		t.Errorf("unexpected error: %v", err)
 	}
 
-	// cr is updated even though spec is invalid
+	// update with .status.num = 15, expecting an error
-	err = wait.Poll(500*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) {
+	err = unstructured.SetNestedField(createdNoxuInstance.Object, int64(15), "status", "num")
-		_, err := noxuStatusResourceClient.Update(createdNoxuInstance)
-		if statusError, isStatus := err.(*apierrors.StatusError); isStatus {
-			if strings.Contains(statusError.Error(), "is invalid") {
-				return false, nil
-			}
-		}
-		if err != nil {
-			return false, err
-		}
-		return true, nil
-	})
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("unexpected error setting .status.num: %v", err)
+	}
+	createdNoxuInstance, err = noxuStatusResourceClient.Update(createdNoxuInstance)
+	if err == nil {
+		t.Fatal("expected error, but got none")
+	}
+	statusError, isStatus := err.(*apierrors.StatusError)
+	if !isStatus || statusError == nil {
+		t.Fatalf("expected status error, got %T: %v", err, err)
+	}
+	if !strings.Contains(statusError.Error(), "Invalid value") {
+		t.Fatalf("expected 'Invalid value' in error, got: %v", err)
 	}
 }
 

staging/src/k8s.io/apiextensions-apiserver/test/integration/testserver/resources.go

@@ -275,6 +275,25 @@ func checkForWatchCachePrimed(crd *apiextensionsv1beta1.CustomResourceDefinition
 	}
 }
 
+// UpdateCustomResourceDefinition updates a CRD, retrying up to 5 times on version conflict errors.
+func UpdateCustomResourceDefinition(client clientset.Interface, name string, update func(*apiextensionsv1beta1.CustomResourceDefinition)) (*apiextensionsv1beta1.CustomResourceDefinition, error) {
+	for i := 0; i < 5; i++ {
+		crd, err := client.ApiextensionsV1beta1().CustomResourceDefinitions().Get(name, metav1.GetOptions{})
+		if err != nil {
+			return nil, fmt.Errorf("failed to get CustomResourceDefinition %q: %v", name, err)
+		}
+		update(crd)
+		crd, err = client.ApiextensionsV1beta1().CustomResourceDefinitions().Update(crd)
+		if err == nil {
+			return crd, nil
+		}
+		if !errors.IsConflict(err) {
+			return nil, fmt.Errorf("failed to update CustomResourceDefinition %q: %v", name, err)
+		}
+	}
+	return nil, fmt.Errorf("too many retries after conflicts updating CustomResourceDefinition %q", name)
+}
+
 func DeleteCustomResourceDefinition(crd *apiextensionsv1beta1.CustomResourceDefinition, apiExtensionsClient clientset.Interface) error {
 	if err := apiExtensionsClient.Apiextensions().CustomResourceDefinitions().Delete(crd.Name, nil); err != nil {
 		return err

staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go

@@ -348,14 +348,11 @@ func TestCRValidationOnCRDUpdate(t *testing.T) {
 		t.Fatalf("unexpected non-error: CR should be rejected")
 	}
 
-	gottenCRD, err := apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Get("noxus.mygroup.example.com", metav1.GetOptions{})
-	if err != nil {
-		t.Fatal(err)
-	}
-
 	// update the CRD to a less stricter schema
-	gottenCRD.Spec.Validation.OpenAPIV3Schema.Required = []string{"alpha", "beta"}
+	_, err = testserver.UpdateCustomResourceDefinition(apiExtensionClient, "noxus.mygroup.example.com", func(crd *apiextensionsv1beta1.CustomResourceDefinition) {
-	if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(gottenCRD); err != nil {
+		crd.Spec.Validation.OpenAPIV3Schema.Required = []string{"alpha", "beta"}
+	})
+	if err != nil {
 		t.Fatal(err)
 	}