From 485741ed672abb904e4e5fa08f9f54ab51b8d7f0 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 27 Aug 2019 15:19:50 -0400 Subject: [PATCH] Skip deleting custom resource instances that overlap with storage for built-in types --- .../pkg/controller/finalizer/BUILD | 1 + .../pkg/controller/finalizer/crd_finalizer.go | 21 +- test/integration/etcd/BUILD | 9 + .../etcd/crd_overlap_storage_test.go | 380 ++++++++++++++++++ 4 files changed, 410 insertions(+), 1 deletion(-) create mode 100644 test/integration/etcd/crd_overlap_storage_test.go diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/BUILD index 5acb5f3ce99..bc7e213210e 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/BUILD @@ -18,6 +18,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go index c89d781bdf6..bea14d4f1ad 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go @@ -26,6 +26,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" utilerrors "k8s.io/apimachinery/pkg/util/errors" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" @@ -41,6 +42,16 @@ import ( listers "k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/internalversion" ) +// OverlappingBuiltInResources returns the set of built-in group/resources that are persisted +// in storage paths that overlap with CRD storage paths, and should not be deleted +// by this controller if an associated CRD is deleted. +func OverlappingBuiltInResources() map[schema.GroupResource]bool { + return map[schema.GroupResource]bool{ + {Group: "apiregistration.k8s.io", Resource: "apiservices"}: true, + {Group: "apiextensions.k8s.io", Resource: "customresourcedefinitions"}: true, + } +} + // CRDFinalizer is a controller that finalizes the CRD by deleting all the CRs associated with it. type CRDFinalizer struct { crdClient client.CustomResourceDefinitionsGetter @@ -126,7 +137,15 @@ func (c *CRDFinalizer) sync(key string) error { // Now we can start deleting items. We should use the REST API to ensure that all normal admission runs. // Since we control the endpoints, we know that delete collection works. No need to delete if not established. - if apiextensions.IsCRDConditionTrue(crd, apiextensions.Established) { + if OverlappingBuiltInResources()[schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Spec.Names.Plural}] { + // Skip deletion, explain why, and proceed to remove the finalizer and delete the CRD + apiextensions.SetCRDCondition(crd, apiextensions.CustomResourceDefinitionCondition{ + Type: apiextensions.Terminating, + Status: apiextensions.ConditionFalse, + Reason: "OverlappingBuiltInResource", + Message: "instances overlap with built-in resources in storage", + }) + } else if apiextensions.IsCRDConditionTrue(crd, apiextensions.Established) { cond, deleteErr := c.deleteInstances(crd) apiextensions.SetCRDCondition(crd, cond) if deleteErr != nil { diff --git a/test/integration/etcd/BUILD b/test/integration/etcd/BUILD index a76bd60cbd5..e49ae2ada0e 100644 --- a/test/integration/etcd/BUILD +++ b/test/integration/etcd/BUILD @@ -6,6 +6,7 @@ go_test( name = "go_default_test", size = "large", srcs = [ + "crd_overlap_storage_test.go", "etcd_cross_group_test.go", "etcd_storage_path_test.go", "main_test.go", @@ -18,15 +19,23 @@ go_test( deps = [ "//cmd/kube-apiserver/app/options:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", + "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1:go_default_library", + "//staging/src/k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1:go_default_library", + "//staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/watch:go_default_library", "//staging/src/k8s.io/client-go/dynamic:go_default_library", + "//staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1:go_default_library", + "//staging/src/k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/typed/apiregistration/v1:go_default_library", "//test/integration/framework:go_default_library", "//vendor/github.com/coreos/etcd/clientv3:go_default_library", ], diff --git a/test/integration/etcd/crd_overlap_storage_test.go b/test/integration/etcd/crd_overlap_storage_test.go new file mode 100644 index 00000000000..1966582a90f --- /dev/null +++ b/test/integration/etcd/crd_overlap_storage_test.go @@ -0,0 +1,380 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package etcd + +import ( + "encoding/json" + "strings" + "testing" + "time" + + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + crdclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1" + "k8s.io/apiextensions-apiserver/pkg/controller/finalizer" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/dynamic" + apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" + apiregistrationclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/typed/apiregistration/v1" +) + +// TestOverlappingBuiltInResources ensures the list of group-resources the custom resource finalizer should skip is up to date +func TestOverlappingBuiltInResources(t *testing.T) { + // Verify built-in resources that overlap with computed CRD storage paths are listed in OverlappingBuiltInResources() + detectedOverlappingResources := map[schema.GroupResource]bool{} + for gvr, gvrData := range GetEtcdStorageData() { + if !strings.HasSuffix(gvr.Group, ".k8s.io") { + // only fully-qualified group names can exist as CRDs + continue + } + if !strings.Contains(gvrData.ExpectedEtcdPath, "/"+gvr.Group+"/"+gvr.Resource+"/") { + // CRDs persist in storage under ...///... + continue + } + detectedOverlappingResources[gvr.GroupResource()] = true + } + + for detected := range detectedOverlappingResources { + if !finalizer.OverlappingBuiltInResources()[detected] { + t.Errorf("built-in resource %#v would overlap with custom resource storage if a CRD was created for the same group/resource", detected) + t.Errorf("add %#v to the OverlappingBuiltInResources() list to prevent deletion by the CRD finalizer", detected) + } + } + for skip := range finalizer.OverlappingBuiltInResources() { + if !detectedOverlappingResources[skip] { + t.Errorf("resource %#v does not overlap with any built-in resources in storage, but is skipped for CRD finalization by OverlappingBuiltInResources()", skip) + t.Errorf("remove %#v from OverlappingBuiltInResources() to ensure CRD finalization cleans up stored custom resources", skip) + } + } +} + +// TestOverlappingCustomResourceAPIService ensures creating and deleting a custom resource overlapping with APIServices does not destroy APIService data +func TestOverlappingCustomResourceAPIService(t *testing.T) { + master := StartRealMasterOrDie(t) + defer master.Cleanup() + + apiServiceClient, err := apiregistrationclient.NewForConfig(master.Config) + if err != nil { + t.Fatal(err) + } + crdClient, err := crdclient.NewForConfig(master.Config) + if err != nil { + t.Fatal(err) + } + dynamicClient, err := dynamic.NewForConfig(master.Config) + if err != nil { + t.Fatal(err) + } + + // Verify APIServices can be listed + apiServices, err := apiServiceClient.APIServices().List(metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + apiServiceNames := sets.NewString() + for _, s := range apiServices.Items { + apiServiceNames.Insert(s.Name) + } + if len(apiServices.Items) == 0 { + t.Fatal("expected APIService objects, got none") + } + + // Create a CRD defining an overlapping apiregistration.k8s.io apiservices resource with an incompatible schema + crdCRD, err := crdClient.CustomResourceDefinitions().Create(&apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "apiservices.apiregistration.k8s.io", + Annotations: map[string]string{"api-approved.kubernetes.io": "unapproved, testing only"}, + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "apiregistration.k8s.io", + Scope: apiextensionsv1.ClusterScoped, + Names: apiextensionsv1.CustomResourceDefinitionNames{Plural: "apiservices", Singular: "customapiservice", Kind: "CustomAPIService", ListKind: "CustomAPIServiceList"}, + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + Schema: &apiextensionsv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Required: []string{"foo"}, + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "foo": {Type: "string"}, + "bar": {Type: "string", Default: &apiextensionsv1.JSON{Raw: []byte(`"default"`)}}, + }, + }, + }, + }, + }, + }, + }) + if err != nil { + t.Fatal(err) + } + + // Wait until it is established + if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { + crd, err := crdClient.CustomResourceDefinitions().Get(crdCRD.Name, metav1.GetOptions{}) + if err != nil { + return false, err + } + for _, condition := range crd.Status.Conditions { + if condition.Status == apiextensionsv1.ConditionTrue && condition.Type == apiextensionsv1.Established { + return true, nil + } + } + conditionJSON, _ := json.Marshal(crd.Status.Conditions) + t.Logf("waiting for establishment (conditions: %s)", string(conditionJSON)) + return false, nil + }); err != nil { + t.Fatal(err) + } + + // Make sure API requests are still handled by the built-in handler (and return built-in kinds) + + // Listing v1 succeeds + v1DynamicList, err := dynamicClient.Resource(schema.GroupVersionResource{Group: "apiregistration.k8s.io", Version: "v1", Resource: "apiservices"}).List(metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + // Result was served by built-in handler, not CR handler + if _, hasDefaultedCRField := v1DynamicList.Items[0].Object["spec"].(map[string]interface{})["bar"]; hasDefaultedCRField { + t.Fatalf("expected no CR defaulting, got %#v", v1DynamicList.Items[0].Object) + } + + // Creating v1 succeeds (built-in validation, not CR validation) + testAPIService, err := apiServiceClient.APIServices().Create(&apiregistrationv1.APIService{ + ObjectMeta: metav1.ObjectMeta{Name: "v1.example.com"}, + Spec: apiregistrationv1.APIServiceSpec{ + Group: "example.com", + Version: "v1", + VersionPriority: 100, + GroupPriorityMinimum: 100, + }, + }) + if err != nil { + t.Fatal(err) + } + err = apiServiceClient.APIServices().Delete(testAPIService.Name, &metav1.DeleteOptions{}) + if err != nil { + t.Fatal(err) + } + + // discovery is handled by the built-in handler + v1Resources, err := master.Client.Discovery().ServerResourcesForGroupVersion("apiregistration.k8s.io/v1") + if err != nil { + t.Fatal(err) + } + for _, r := range v1Resources.APIResources { + if r.Name == "apiservices" { + if r.Kind != "APIService" { + t.Errorf("expected kind=APIService in discovery, got %s", r.Kind) + } + } + } + v2Resources, err := master.Client.Discovery().ServerResourcesForGroupVersion("apiregistration.k8s.io/v2") + if err == nil { + t.Fatalf("expected error looking up apiregistration.k8s.io/v2 discovery, got %#v", v2Resources) + } + + // Delete the overlapping CRD + err = crdClient.CustomResourceDefinitions().Delete(crdCRD.Name, &metav1.DeleteOptions{}) + if err != nil { + t.Fatal(err) + } + + // Make sure the CRD deletion succeeds + if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { + crd, err := crdClient.CustomResourceDefinitions().Get(crdCRD.Name, metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + return true, nil + } + if err != nil { + return false, err + } + conditionJSON, _ := json.Marshal(crd.Status.Conditions) + t.Logf("waiting for deletion (conditions: %s)", string(conditionJSON)) + return false, nil + }); err != nil { + t.Fatal(err) + } + + // Make sure APIService objects are not removed + time.Sleep(5 * time.Second) + finalAPIServices, err := apiServiceClient.APIServices().List(metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + if len(finalAPIServices.Items) != len(apiServices.Items) { + t.Fatalf("expected %d APIService objects, got %d", len(apiServices.Items), len(finalAPIServices.Items)) + } +} + +// TestOverlappingCustomResourceCustomResourceDefinition ensures creating and deleting a custom resource overlapping with CustomResourceDefinition does not destroy CustomResourceDefinition data +func TestOverlappingCustomResourceCustomResourceDefinition(t *testing.T) { + master := StartRealMasterOrDie(t) + defer master.Cleanup() + + crdClient, err := crdclient.NewForConfig(master.Config) + if err != nil { + t.Fatal(err) + } + dynamicClient, err := dynamic.NewForConfig(master.Config) + if err != nil { + t.Fatal(err) + } + + // Verify CustomResourceDefinitions can be listed + crds, err := crdClient.CustomResourceDefinitions().List(metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + crdNames := sets.NewString() + for _, s := range crds.Items { + crdNames.Insert(s.Name) + } + if len(crds.Items) == 0 { + t.Fatal("expected CustomResourceDefinition objects, got none") + } + + // Create a CRD defining an overlapping apiregistration.k8s.io apiservices resource with an incompatible schema + crdCRD, err := crdClient.CustomResourceDefinitions().Create(&apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "customresourcedefinitions.apiextensions.k8s.io", + Annotations: map[string]string{"api-approved.kubernetes.io": "unapproved, testing only"}, + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "apiextensions.k8s.io", + Scope: apiextensionsv1.ClusterScoped, + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "customresourcedefinitions", + Singular: "customcustomresourcedefinition", + Kind: "CustomCustomResourceDefinition", + ListKind: "CustomAPIServiceList", + }, + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ + { + Name: "v1", + Served: true, + Storage: true, + Schema: &apiextensionsv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Required: []string{"foo"}, + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "foo": {Type: "string"}, + "bar": {Type: "string", Default: &apiextensionsv1.JSON{Raw: []byte(`"default"`)}}, + }, + }, + }, + }, + }, + }, + }) + if err != nil { + t.Fatal(err) + } + + // Wait until it is established + if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { + crd, err := crdClient.CustomResourceDefinitions().Get(crdCRD.Name, metav1.GetOptions{}) + if err != nil { + return false, err + } + for _, condition := range crd.Status.Conditions { + if condition.Status == apiextensionsv1.ConditionTrue && condition.Type == apiextensionsv1.Established { + return true, nil + } + } + conditionJSON, _ := json.Marshal(crd.Status.Conditions) + t.Logf("waiting for establishment (conditions: %s)", string(conditionJSON)) + return false, nil + }); err != nil { + t.Fatal(err) + } + + // Make sure API requests are still handled by the built-in handler (and return built-in kinds) + + // Listing v1 succeeds + v1DynamicList, err := dynamicClient.Resource(schema.GroupVersionResource{Group: "apiextensions.k8s.io", Version: "v1", Resource: "customresourcedefinitions"}).List(metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + // Result was served by built-in handler, not CR handler + if _, hasDefaultedCRField := v1DynamicList.Items[0].Object["spec"].(map[string]interface{})["bar"]; hasDefaultedCRField { + t.Fatalf("expected no CR defaulting, got %#v", v1DynamicList.Items[0].Object) + } + + // Updating v1 succeeds (built-in validation, not CR validation) + _, err = crdClient.CustomResourceDefinitions().Patch(crdCRD.Name, types.MergePatchType, []byte(`{"metadata":{"annotations":{"test":"updated"}}}`)) + if err != nil { + t.Fatal(err) + } + + // discovery is handled by the built-in handler + v1Resources, err := master.Client.Discovery().ServerResourcesForGroupVersion("apiextensions.k8s.io/v1") + if err != nil { + t.Fatal(err) + } + for _, r := range v1Resources.APIResources { + if r.Name == "customresourcedefinitions" { + if r.Kind != "CustomResourceDefinition" { + t.Errorf("expected kind=CustomResourceDefinition in discovery, got %s", r.Kind) + } + } + } + v2Resources, err := master.Client.Discovery().ServerResourcesForGroupVersion("apiextensions.k8s.io/v2") + if err == nil { + t.Fatalf("expected error looking up apiregistration.k8s.io/v2 discovery, got %#v", v2Resources) + } + + // Delete the overlapping CRD + err = crdClient.CustomResourceDefinitions().Delete(crdCRD.Name, &metav1.DeleteOptions{}) + if err != nil { + t.Fatal(err) + } + + // Make sure the CRD deletion succeeds + if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { + crd, err := crdClient.CustomResourceDefinitions().Get(crdCRD.Name, metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + return true, nil + } + if err != nil { + return false, err + } + conditionJSON, _ := json.Marshal(crd.Status.Conditions) + t.Logf("waiting for deletion (conditions: %s)", string(conditionJSON)) + return false, nil + }); err != nil { + t.Fatal(err) + } + + // Make sure other CustomResourceDefinition objects are not removed + time.Sleep(5 * time.Second) + finalCRDs, err := crdClient.CustomResourceDefinitions().List(metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + if len(finalCRDs.Items) != len(crds.Items) { + t.Fatalf("expected %d APIService objects, got %d", len(crds.Items), len(finalCRDs.Items)) + } +}