Skip to content

Skip creating storages for non-stored and non-served versions #130704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -818,6 +818,11 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
return nil, fmt.Errorf("the server could not properly serve the list kind")
}

// Do not construct storage if version is neither served nor stored
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think short-circuiting at this point in the loop is correct, but it's really hard to reason about which things in this loop have side-effects we need to preserve.

I think it is just the RegisterKindFor calls, but I'm not 100% sure.

I think it would be clearer to add a loop before the storage-constructing loop, which we do not short-circuit, that does whatever validation / equivalent kind registration we want to do for all versions:

	// ensure accepted names are valid
	if len(crd.Status.AcceptedNames.Plural) == 0 {
		utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.plural", crd.Name))
		return nil, fmt.Errorf("the server could not properly serve the resource")
	}
	if len(crd.Status.AcceptedNames.Singular) == 0 {
		utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.singular", crd.Name))
		return nil, fmt.Errorf("the server could not properly serve the resource")
	}
	if len(crd.Status.AcceptedNames.Kind) == 0 {
		utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.kind", crd.Name))
		return nil, fmt.Errorf("the server could not properly serve the kind")
	}
	if len(crd.Status.AcceptedNames.ListKind) == 0 {
		utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.listKind", crd.Name))
		return nil, fmt.Errorf("the server could not properly serve the list kind")
	}

	var (
		versionToResource         = map[string]schema.GroupVersionResource{}
		versionToSingularResource = map[string]schema.GroupVersionResource{}
		versionToKind             = map[string]schema.GroupVersionKind{}
		versionToListKind         = map[string]schema.GroupVersionKind{}
		versionToSubresources     = map[string]*apiextensionsv1.CustomResourceSubresources{}
	)
	// compute resource, singularResource, kind, subresources, and register equivalent kinds for all versions
	for _, v := range crd.Spec.Versions {
		versionToResource[v.Name] = schema.GroupVersionResource{Group: crd.Spec.Group, Version: v.Name, Resource: crd.Status.AcceptedNames.Plural}
		versionToSingularResource[v.Name] = schema.GroupVersionResource{Group: crd.Spec.Group, Version: v.Name, Resource: crd.Status.AcceptedNames.Singular}
		versionToKind[v.Name] = schema.GroupVersionKind{Group: crd.Spec.Group, Version: v.Name, Kind: crd.Status.AcceptedNames.Kind}
		versionToListKind[v.Name] = schema.GroupVersionKind{Group: crd.Spec.Group, Version: v.Name, Kind: crd.Status.AcceptedNames.ListKind}
		versionToSubresources[v.Name], err = apiextensionshelpers.GetSubresourcesForVersion(crd, v.Name)
		if err != nil {
			utilruntime.HandleError(err)
			return nil, fmt.Errorf("the server could not properly serve the CR subresources")
		}

		// Register equivalent kinds
		equivalentResourceRegistry.RegisterKindFor(versionToResource[v.Name], "", versionToKind[v.Name])
		if versionToSubresources[v.Name] != nil {
			if versionToSubresources[v.Name].Status != nil {
				equivalentResourceRegistry.RegisterKindFor(versionToResource[v.Name], "status", versionToKind[v.Name])
			}
			if versionToSubresources[v.Name].Scale != nil {
				equivalentResourceRegistry.RegisterKindFor(versionToResource[v.Name], "scale", autoscalingv1.SchemeGroupVersion.WithKind("Scale"))
			}
		}
	}

and then short-circuit this loop at the very top

+       // construct storage for served / stored versions
        for _, v := range crd.Spec.Versions {
+               if !v.Storage && !v.Served {
+                       continue
+               }
+
                // In addition to Unstructured objects (Custom Resources), we also may sometimes need to
                // decode unversioned Options objects, so we delegate to parameterScheme for such types.
                parameterScheme := runtime.NewScheme()
@@ -735,22 +787,9 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
                )
                parameterCodec := runtime.NewParameterCodec(parameterScheme)
 
-               resource := schema.GroupVersionResource{Group: crd.Spec.Group, Version: v.Name, Resource: crd.Status.AcceptedNames.Plural}
-               if len(resource.Resource) == 0 {
-                       utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.plural", crd.Name))
-                       return nil, fmt.Errorf("the server could not properly serve the resource")
-               }
-               singularResource := schema.GroupVersionResource{Group: crd.Spec.Group, Version: v.Name, Resource: crd.Status.AcceptedNames.Singular}
-               if len(singularResource.Resource) == 0 {
-                       utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.singular", crd.Name))
-                       return nil, fmt.Errorf("the server could not properly serve the resource")
-               }
-               kind := schema.GroupVersionKind{Group: crd.Spec.Group, Version: v.Name, Kind: crd.Status.AcceptedNames.Kind}
-               if len(kind.Kind) == 0 {
-                       utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.kind", crd.Name))
-                       return nil, fmt.Errorf("the server could not properly serve the kind")
-               }
-               equivalentResourceRegistry.RegisterKindFor(resource, "", kind)
+               resource := versionToResource[v.Name]
+               singularResource := versionToSingularResource[v.Name]
+               kind := versionToKind[v.Name]
 
                typer := newUnstructuredObjectTyper(parameterScheme)
                creator := unstructuredCreator{}
@@ -776,13 +815,8 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
 
                var statusSpec *apiextensionsinternal.CustomResourceSubresourceStatus
                var statusValidator apiservervalidation.SchemaValidator
-               subresources, err := apiextensionshelpers.GetSubresourcesForVersion(crd, v.Name)
-               if err != nil {
-                       utilruntime.HandleError(err)
-                       return nil, fmt.Errorf("the server could not properly serve the CR subresources")
-               }
+               subresources := versionToSubresources[v.Name]
                if subresources != nil && subresources.Status != nil {
-                       equivalentResourceRegistry.RegisterKindFor(resource, "status", kind)
                        statusSpec = &apiextensionsinternal.CustomResourceSubresourceStatus{}
                        if err := apiextensionsv1.Convert_v1_CustomResourceSubresourceStatus_To_apiextensions_CustomResourceSubresourceStatus(subresources.Status, statusSpec, nil); err != nil {
                                return nil, fmt.Errorf("failed converting CRD status subresource to internal version: %v", err)
@@ -800,7 +834,6 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
 
                var scaleSpec *apiextensionsinternal.CustomResourceSubresourceScale
                if subresources != nil && subresources.Scale != nil {
-                       equivalentResourceRegistry.RegisterKindFor(resource, "scale", autoscalingv1.SchemeGroupVersion.WithKind("Scale"))
                        scaleSpec = &apiextensionsinternal.CustomResourceSubresourceScale{}
                        if err := apiextensionsv1.Convert_v1_CustomResourceSubresourceScale_To_apiextensions_CustomResourceSubresourceScale(subresources.Scale, scaleSpec, nil); err != nil {
                                return nil, fmt.Errorf("failed converting CRD status subresource to internal version: %v", err)
@@ -817,11 +850,7 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
                        klog.V(2).Infof("The CRD for %v has an invalid printer specification, falling back to default printing: %v", kind, err)
                }
 
-               listKind := schema.GroupVersionKind{Group: crd.Spec.Group, Version: v.Name, Kind: crd.Status.AcceptedNames.ListKind}
-               if len(listKind.Kind) == 0 {
-                       utilruntime.HandleError(fmt.Errorf("CustomResourceDefinition %s has unexpected empty status.acceptedNames.listKind", crd.Name))
-                       return nil, fmt.Errorf("the server could not properly serve the list kind")
-               }
+               listKind := versionToListKind[v.Name]
 
                storages[v.Name], err = customresource.NewStorage(
                        resource.GroupResource(),

if !v.Storage && !v.Served {
continue
}

storages[v.Name], err = customresource.NewStorage(
resource.GroupResource(),
singularResource.GroupResource(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,81 @@ func TestWebhookConverterWithoutWatchCache(t *testing.T) {
testWebhookConverter(t, false)
}

// TestWebhookNotCalledForUnusedVersions tests scenario where conversion webhook could be called for
// versions that are nor served or nor stored.
// Described in detail in https://github.com/kubernetes/kubernetes/issues/129979
func TestWebhookNotCalledForUnusedVersions(t *testing.T) {
// KUBE_APISERVER_SERVE_REMOVED_APIS_FOR_ONE_RELEASE allows for APIs pending removal to not block tests
t.Setenv("KUBE_APISERVER_SERVE_REMOVED_APIS_FOR_ONE_RELEASE", "true")
Comment on lines +70 to +71
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't need this set for this test


etcd3watcher.TestOnlySetFatalOnDecodeError(false)
defer etcd3watcher.TestOnlySetFatalOnDecodeError(true)

tearDown, config, options, err := fixtures.StartDefaultServer(t, "--watch-cache=true")
if err != nil {
t.Fatal(err)
}

apiExtensionsClient, err := clientset.NewForConfig(config)
if err != nil {
tearDown()
t.Fatal(err)
}

dynamicClient, err := dynamic.NewForConfig(config)
if err != nil {
tearDown()
t.Fatal(err)
}
defer tearDown()

crd := multiVersionFixture.DeepCopy()

RESTOptionsGetter := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd, nil, nil)
restOptions, err := RESTOptionsGetter.GetRESTOptions(schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Spec.Names.Plural}, nil)
if err != nil {
t.Fatal(err)
}
etcdClient, _, err := storage.GetEtcdClients(restOptions.StorageConfig.Transport)
if err != nil {
t.Fatal(err)
}
// nolint:errcheck
defer etcdClient.Close()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to make sure that existing objects in etcd can still be successfully read and converted from a non-served, non-storage version, and can be updated

  1. create the CRD serving all versions, storing in v1alpha2
  2. server-side apply an object via the v1alpha2 API (will persist in v1alpha2)
  3. update the CRD to switch the storage version to a different version, stop serving v1alpha2, and set a conversion webhook
  4. ensure the conversion webhook is never asked to convert to v1alpha2 (as this test already does)
  5. ensure the previously created object can still be read via a different API version
  6. ensure the previously created object can still be updated via a different API version using server-side apply
  7. ensure the update does not trigger a conversion request to v1alpha2

etcdObjectReader := storage.NewEtcdObjectReader(etcdClient, &restOptions, crd)
ctcTearDown, ctc := newConversionTestContext(t, apiExtensionsClient, dynamicClient, etcdObjectReader, crd)
defer ctcTearDown()

upCh, handler := closeOnCall(NewObjectConverterWebhookHandler(t, getUnexpectedVersionCheckConverter(t, "v1alpha2")))
tearDown, webhookClientConfig, err := StartConversionWebhookServer(handler)
if err != nil {
t.Fatal(err)
}
defer tearDown()

ctc.setConversionWebhook(t, webhookClientConfig, []string{"v1alpha1", "v1beta1"})

// create an object to initiate webhook calls
_, err = ctc.versionedClient("marker", "v1beta1").Create(context.TODO(), newConversionMultiVersionFixture("marker", "marker", "v1beta1"), metav1.CreateOptions{})
if err != nil {
t.Fatal(err)
}

// wait until new webhook is called the first time
if err := wait.PollUntilContextTimeout(context.Background(), time.Second*1, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) {
select {
case <-upCh:
return true, nil
default:
t.Log("Waiting until webhook is effective")
return false, nil
}
}); err != nil {
t.Fatal(err)
}
}

func testWebhookConverter(t *testing.T, watchCache bool) {
tests := []struct {
group string
Expand Down Expand Up @@ -248,6 +323,9 @@ func validateStorageVersion(t *testing.T, ctc *conversionTestContext) {
ns := ctc.namespace

for _, version := range ctc.crd.Spec.Versions {
if !version.Storage && !version.Served {
continue
}
t.Run(version.Name, func(t *testing.T) {
name := "storageversion-" + version.Name
client := ctc.versionedClient(ns, version.Name)
Expand Down Expand Up @@ -315,6 +393,9 @@ func validateServed(t *testing.T, ctc *conversionTestContext) {
ns := ctc.namespace

for _, version := range ctc.crd.Spec.Versions {
if !version.Storage && !version.Served {
continue
}
t.Run(version.Name, func(t *testing.T) {
name := "served-" + version.Name
client := ctc.versionedClient(ns, version.Name)
Expand All @@ -334,6 +415,9 @@ func validateNonTrivialConverted(t *testing.T, ctc *conversionTestContext) {
ns := ctc.namespace

for _, createVersion := range ctc.crd.Spec.Versions {
if !createVersion.Storage && !createVersion.Served {
continue
}
t.Run(fmt.Sprintf("getting objects created as %s", createVersion.Name), func(t *testing.T) {
name := "converted-" + createVersion.Name
client := ctc.versionedClient(ns, createVersion.Name)
Expand All @@ -354,6 +438,9 @@ func validateNonTrivialConverted(t *testing.T, ctc *conversionTestContext) {
verifyMultiVersionObject(t, "v1beta1", obj)

for _, getVersion := range ctc.crd.Spec.Versions {
if !getVersion.Storage && !getVersion.Served {
continue
}
client := ctc.versionedClient(ns, getVersion.Name)
obj, err := client.Get(context.TODO(), name, metav1.GetOptions{})
if err != nil {
Expand Down Expand Up @@ -391,7 +478,15 @@ func validateNonTrivialConvertedList(t *testing.T, ctc *conversionTestContext) {
ns := ctc.namespace + "-list"

names := sets.String{}
for _, createVersion := range ctc.crd.Spec.Versions {
servedVersions := []apiextensionsv1.CustomResourceDefinitionVersion{}
for _, version := range ctc.crd.Spec.Versions {
if !version.Storage && !version.Served {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we should only be checking version.Served when accumulating a list named servedVersions:

Suggested change
if !version.Storage && !version.Served {
if !version.Served {

maybe make this a helper function:

func servedVersions(versions []apiextensionsv1.CustomResourceDefinitionVersion) []apiextensionsv1.CustomResourceDefinitionVersion

so all the places we're iterating over versions can just wrap with a call to the helper instead of adding custom loop logic (noted a couple below, but I think ~all of them could switch to that style)

continue
}
servedVersions = append(servedVersions, version)
}

for _, createVersion := range servedVersions {
name := "converted-" + createVersion.Name
client := ctc.versionedClient(ns, createVersion.Name)
fixture := newConversionMultiVersionFixture(ns, name, createVersion.Name)
Expand All @@ -405,14 +500,14 @@ func validateNonTrivialConvertedList(t *testing.T, ctc *conversionTestContext) {
names.Insert(name)
}

for _, listVersion := range ctc.crd.Spec.Versions {
for _, listVersion := range servedVersions {
t.Run(fmt.Sprintf("listing objects as %s", listVersion.Name), func(t *testing.T) {
client := ctc.versionedClient(ns, listVersion.Name)
obj, err := client.List(context.TODO(), metav1.ListOptions{})
if err != nil {
t.Fatal(err)
}
if len(obj.Items) != len(ctc.crd.Spec.Versions) {
if len(obj.Items) != len(servedVersions) {
t.Fatal("unexpected number of items")
}
foundNames := sets.String{}
Expand All @@ -431,6 +526,9 @@ func validateStoragePruning(t *testing.T, ctc *conversionTestContext) {
ns := ctc.namespace

for _, createVersion := range ctc.crd.Spec.Versions {
if !createVersion.Storage && !createVersion.Served {
continue
}
Comment on lines 528 to +531
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for _, createVersion := range ctc.crd.Spec.Versions {
if !createVersion.Storage && !createVersion.Served {
continue
}
for _, createVersion := range servedVersions(ctc.crd.Spec.Versions) {

t.Run(fmt.Sprintf("getting objects created as %s", createVersion.Name), func(t *testing.T) {
name := "storagepruning-" + createVersion.Name
client := ctc.versionedClient(ns, createVersion.Name)
Expand Down Expand Up @@ -466,6 +564,9 @@ func validateStoragePruning(t *testing.T, ctc *conversionTestContext) {
}

for _, getVersion := range ctc.crd.Spec.Versions {
if !getVersion.Storage && !getVersion.Served {
continue
}
Comment on lines 566 to +569
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for _, getVersion := range ctc.crd.Spec.Versions {
if !getVersion.Storage && !getVersion.Served {
continue
}
for _, getVersion := range servedVersions(ctc.crd.Spec.Versions) {

client := ctc.versionedClient(ns, getVersion.Name)
obj, err := client.Get(context.TODO(), name, metav1.GetOptions{})
if err != nil {
Expand Down Expand Up @@ -593,8 +694,15 @@ func validateDefaulting(t *testing.T, ctc *conversionTestContext) {

ns := ctc.namespace
storageVersion := "v1beta1"
servedVersions := []apiextensionsv1.CustomResourceDefinitionVersion{}
for _, version := range ctc.crd.Spec.Versions {
if !version.Storage && !version.Served {
continue
}
servedVersions = append(servedVersions, version)
}

for _, createVersion := range ctc.crd.Spec.Versions {
for _, createVersion := range servedVersions {
t.Run(fmt.Sprintf("getting objects created as %s", createVersion.Name), func(t *testing.T) {
name := "defaulting-" + createVersion.Name
client := ctc.versionedClient(ns, createVersion.Name)
Expand Down Expand Up @@ -644,7 +752,7 @@ func validateDefaulting(t *testing.T, ctc *conversionTestContext) {
}

// check that when reading any other version, we do not default that version, but only the (non-persisted) storage version default
for _, v := range ctc.crd.Spec.Versions {
for _, v := range servedVersions {
if v.Name == createVersion.Name {
// create version is persisted anyway, nothing to verify
continue
Expand Down Expand Up @@ -925,6 +1033,16 @@ func uidMutatingConverter(desiredAPIVersion string, obj runtime.RawExtension) (r
return runtime.RawExtension{Raw: raw}, nil
}

func getUnexpectedVersionCheckConverter(t *testing.T, version string) ObjectConverterFunc {
return func(desiredAPIVersion string, obj runtime.RawExtension) (runtime.RawExtension, error) {
if desiredAPIVersion == "stable.example.com/"+version {
t.Fatalf("webhook received unexpected version: %s", version)
}

return noopConverter(desiredAPIVersion, obj)
}
}

func newConversionTestContext(t *testing.T, apiExtensionsClient clientset.Interface, dynamicClient dynamic.Interface, etcdObjectReader *storage.EtcdObjectReader, v1CRD *apiextensionsv1.CustomResourceDefinition) (func(), *conversionTestContext) {
v1CRD, err := fixtures.CreateNewV1CustomResourceDefinition(v1CRD, apiExtensionsClient, dynamicClient)
if err != nil {
Expand Down Expand Up @@ -963,6 +1081,9 @@ func (c *conversionTestContext) versionedClient(ns string, version string) dynam
func (c *conversionTestContext) versionedClients(ns string) map[string]dynamic.ResourceInterface {
ret := map[string]dynamic.ResourceInterface{}
for _, v := range c.crd.Spec.Versions {
if !v.Storage && !v.Served {
continue
}
ret[v.Name] = c.versionedClient(ns, v.Name)
}
return ret
Expand Down Expand Up @@ -1134,6 +1255,7 @@ var multiVersionFixture = &apiextensionsv1.CustomResourceDefinition{
Type: "object",
Properties: map[string]apiextensionsv1.JSONSchemaProps{
"v1alpha1": {Type: "boolean"},
"v1alpha2": {Type: "boolean"},
"v1beta1": {Type: "boolean", Default: jsonPtr(true)},
"v1beta2": {Type: "boolean"},
},
Expand Down Expand Up @@ -1175,6 +1297,7 @@ var multiVersionFixture = &apiextensionsv1.CustomResourceDefinition{
Type: "object",
Properties: map[string]apiextensionsv1.JSONSchemaProps{
"v1alpha1": {Type: "boolean", Default: jsonPtr(true)},
"v1alpha2": {Type: "boolean"},
"v1beta1": {Type: "boolean"},
"v1beta2": {Type: "boolean"},
},
Expand Down Expand Up @@ -1216,6 +1339,7 @@ var multiVersionFixture = &apiextensionsv1.CustomResourceDefinition{
Type: "object",
Properties: map[string]apiextensionsv1.JSONSchemaProps{
"v1alpha1": {Type: "boolean"},
"v1alpha2": {Type: "boolean"},
"v1beta1": {Type: "boolean"},
"v1beta2": {Type: "boolean", Default: jsonPtr(true)},
},
Expand All @@ -1224,6 +1348,48 @@ var multiVersionFixture = &apiextensionsv1.CustomResourceDefinition{
},
},
},
{
// same schema as v1beta1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// same schema as v1beta1
// same schema as v1beta1, but not served

Name: "v1alpha2",
Served: false,
Storage: false,
Subresources: &apiextensionsv1.CustomResourceSubresources{
Status: &apiextensionsv1.CustomResourceSubresourceStatus{},
Scale: &apiextensionsv1.CustomResourceSubresourceScale{
SpecReplicasPath: ".spec.num.num1",
StatusReplicasPath: ".status.num.num2",
},
},
Schema: &apiextensionsv1.CustomResourceValidation{
OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextensionsv1.JSONSchemaProps{
"content": {
Type: "object",
Properties: map[string]apiextensionsv1.JSONSchemaProps{
"key": {Type: "string"},
},
},
"num": {
Type: "object",
Properties: map[string]apiextensionsv1.JSONSchemaProps{
"num1": {Type: "integer"},
"num2": {Type: "integer"},
},
},
"defaults": {
Type: "object",
Properties: map[string]apiextensionsv1.JSONSchemaProps{
"v1alpha1": {Type: "boolean"},
"v1alpha2": {Type: "boolean", Default: jsonPtr(true)},
"v1beta1": {Type: "boolean"},
"v1beta2": {Type: "boolean"},
},
},
},
},
},
},
},
},
}
Expand Down