diff --git a/pkg/crd-installer/installer.go b/pkg/crd-installer/installer.go index b207b734..ffc16d01 100644 --- a/pkg/crd-installer/installer.go +++ b/pkg/crd-installer/installer.go @@ -15,6 +15,8 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" apimachineryv1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" apimachineryYaml "k8s.io/apimachinery/pkg/util/yaml" "k8s.io/client-go/dynamic" @@ -190,32 +192,29 @@ func (cp *CRDsInstaller) processCRD(ctx context.Context, crdFilePath string) err } func (cp *CRDsInstaller) putCRDToCluster(ctx context.Context, crdReader io.Reader, bufferSize int) error { - var crd *apiextensionsv1.CustomResourceDefinition - - err := apimachineryYaml.NewYAMLOrJSONDecoder(crdReader, bufferSize).Decode(&crd) + // Decode into unstructured so vendor schema extensions (x-kubernetes-*, x-ui-*, ...) + // survive verbatim; the typed struct below is used only to read metadata. + desired := &unstructured.Unstructured{} + err := apimachineryYaml.NewYAMLOrJSONDecoder(crdReader, bufferSize).Decode(&desired) if err != nil { return err } - // it could be a comment or some other peace of yaml file, skip it - if crd == nil { + // a comment or other non-object yaml document decodes to a nil pointer / empty object, skip it + if desired == nil || len(desired.Object) == 0 { return nil } - if crd.APIVersion != apiextensionsv1.SchemeGroupVersion.String() && crd.Kind != "CustomResourceDefinition" { - return fmt.Errorf("invalid CRD document apiversion/kind: '%s/%s'", crd.APIVersion, crd.Kind) - } - - if len(crd.ObjectMeta.Labels) == 0 { - crd.ObjectMeta.Labels = make(map[string]string, 1) + crd := &apiextensionsv1.CustomResourceDefinition{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(desired.Object, crd); err != nil { + return err } - - for crdExtraLabel := range cp.crdExtraLabels { - crd.ObjectMeta.Labels[crdExtraLabel] = cp.crdExtraLabels[crdExtraLabel] + if crd.APIVersion != apiextensionsv1.SchemeGroupVersion.String() || crd.Kind != "CustomResourceDefinition" { + return fmt.Errorf("invalid CRD document apiversion/kind: '%s/%s'", crd.APIVersion, crd.Kind) } cp.k8sTasks.Go(func() error { - err := cp.updateOrInsertCRD(ctx, crd) + err := cp.updateOrInsertCRD(ctx, crd, desired) if err == nil { var crdGroup, crdKind string @@ -255,16 +254,13 @@ func (cp *CRDsInstaller) putCRDToCluster(ctx context.Context, crdReader io.Reade return nil } -func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensionsv1.CustomResourceDefinition) error { +func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensionsv1.CustomResourceDefinition, desired *unstructured.Unstructured) error { return retry.RetryOnConflict(retry.DefaultRetry, func() error { - existCRD, err := cp.GetCRDFromCluster(ctx, crd.GetName()) + existing, err := cp.k8sClient.Resource(crdGVR).Get(ctx, crd.GetName(), apimachineryv1.GetOptions{}) if apierrors.IsNotFound(err) { - ucrd, err := utils.ToUnstructured(crd) - if err != nil { - return err - } + mergeLabels(desired, cp.crdExtraLabels) - _, err = cp.k8sClient.Resource(crdGVR).Create(ctx, ucrd, apimachineryv1.CreateOptions{}) + _, err = cp.k8sClient.Resource(crdGVR).Create(ctx, desired, apimachineryv1.CreateOptions{}) if err != nil { return fmt.Errorf("create crd: %w", err) } @@ -276,6 +272,13 @@ func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensio return fmt.Errorf("get crd from cluster: %w", err) } + // typed view of the existing object is used only to reconcile storedVersions; + // it is never written back as the CRD body (that would drop vendor extensions). + existCRD := &apiextensionsv1.CustomResourceDefinition{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(existing.Object, existCRD); err != nil { + return fmt.Errorf("existing crd from unstructured: %w", err) + } + versionsFromNewSpec := make(map[string]struct{}, len(crd.Spec.Versions)) for _, version := range crd.Spec.Versions { versionsFromNewSpec[version.Name] = struct{}{} @@ -288,8 +291,10 @@ func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensio } } + resourceVersion := existing.GetResourceVersion() if !slices.Equal(newStoredVersions, existCRD.Status.StoredVersions) { existCRD.Status.StoredVersions = newStoredVersions + // the status subresource update ignores .spec, so writing the typed (lossy) body here is safe ucrd, err := utils.ToUnstructured(existCRD) if err != nil { return fmt.Errorf("crd to unstructured: %w", err) @@ -301,38 +306,48 @@ func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensio } if resp != nil { - existCRD.ObjectMeta.ResourceVersion = resp.GetResourceVersion() + resourceVersion = resp.GetResourceVersion() } } - if existCRD.Spec.Conversion != nil { - crd.Spec.Conversion = existCRD.Spec.Conversion + // keep the in-cluster conversion webhook config (it is not present in the CRD file) + if conv, found, err := unstructured.NestedFieldCopy(existing.Object, "spec", "conversion"); err == nil && found { + if err := unstructured.SetNestedField(desired.Object, conv, "spec", "conversion"); err != nil { + return fmt.Errorf("preserve conversion: %w", err) + } } - if cmp.Equal(existCRD.Spec, crd.Spec) && - cmp.Equal(existCRD.GetLabels(), crd.GetLabels()) && - cmp.Equal(existCRD.GetAnnotations(), crd.GetAnnotations()) { - return nil - } + mergeLabels(desired, cp.crdExtraLabels) - existCRD.Spec = crd.Spec - existCRD.Labels = crd.Labels - existCRD.Annotations = crd.Annotations + desiredSpec, _, err := unstructured.NestedMap(desired.Object, "spec") + if err != nil { + return fmt.Errorf("read desired spec: %w", err) + } - if len(existCRD.ObjectMeta.Labels) == 0 { - existCRD.ObjectMeta.Labels = make(map[string]string, 1) + existingSpec, _, err := unstructured.NestedMap(existing.Object, "spec") + if err != nil { + return fmt.Errorf("read existing spec: %w", err) } - for crdExtraLabel := range cp.crdExtraLabels { - existCRD.ObjectMeta.Labels[crdExtraLabel] = cp.crdExtraLabels[crdExtraLabel] + // diff on lossless unstructured specs so vendor extensions are not silently dropped + // ponytail: apiserver-defaulted .spec fields may differ from the manifest and cause + // reconcile churn; the update stays idempotent, tighten the diff here if it ever churns. + if cmp.Equal(existingSpec, desiredSpec) && + cmp.Equal(existing.GetLabels(), desired.GetLabels()) && + cmp.Equal(existing.GetAnnotations(), desired.GetAnnotations()) { + return nil } - ucrd, err := utils.ToUnstructured(existCRD) - if err != nil { - return err + // write back the fetched object with only spec/labels/annotations overlaid, so + // server-managed metadata (finalizers, ownerReferences, uid, ...) is preserved. + if err := unstructured.SetNestedMap(existing.Object, desiredSpec, "spec"); err != nil { + return fmt.Errorf("set spec: %w", err) } + existing.SetLabels(desired.GetLabels()) + existing.SetAnnotations(desired.GetAnnotations()) + existing.SetResourceVersion(resourceVersion) - _, err = cp.k8sClient.Resource(crdGVR).Update(ctx, ucrd, apimachineryv1.UpdateOptions{}) + _, err = cp.k8sClient.Resource(crdGVR).Update(ctx, existing, apimachineryv1.UpdateOptions{}) if err != nil { return fmt.Errorf("update crd: %w", err) } @@ -341,6 +356,19 @@ func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensio }) } +func mergeLabels(u *unstructured.Unstructured, extra map[string]string) { + labels := u.GetLabels() + if labels == nil { + labels = make(map[string]string, len(extra)) + } + + for k, v := range extra { + labels[k] = v + } + + u.SetLabels(labels) +} + func (cp *CRDsInstaller) GetCRDFromCluster(ctx context.Context, crdName string) (*apiextensionsv1.CustomResourceDefinition, error) { crd := &apiextensionsv1.CustomResourceDefinition{} diff --git a/pkg/crd-installer/installer_test.go b/pkg/crd-installer/installer_test.go index e201eaf8..227fc537 100644 --- a/pkg/crd-installer/installer_test.go +++ b/pkg/crd-installer/installer_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apimachineryv1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic/fake" @@ -66,4 +67,94 @@ func TestCRDInstaller(t *testing.T) { f4 := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties["field4"].Type assert.Equal(t, "boolean", f4) }) + + // Regression: vendor schema extensions (x-kubernetes-sensitive-data and friends) must + // survive the install path. Decoding CRDs into the typed struct drops them, so the + // installer must apply the object as unstructured. + t.Run("preserves vendor schema extensions", func(t *testing.T) { + inst := NewCRDsInstaller(fc, []string{"testdata/3_sensitive.yaml"}) + require.NoError(t, inst.Run(context.Background())) + + un, err := fc.Resource(gvr).Get(context.Background(), "secrets.example.com", apimachineryv1.GetOptions{}) + require.NoError(t, err) + + versions, found, err := unstructured.NestedSlice(un.Object, "spec", "versions") + require.NoError(t, err) + require.True(t, found, "spec.versions must be present") + + token, found, err := unstructured.NestedMap(versions[0].(map[string]any), + "schema", "openAPIV3Schema", "properties", "spec", "properties", "token") + require.NoError(t, err) + require.True(t, found, "token property must be present") + assert.Equal(t, true, token["x-kubernetes-sensitive-data"], + "vendor extension must survive the install path") + }) + + // Regression: updating an existing CRD must apply the manifest spec (with vendor + // extensions) while preserving server-managed metadata (finalizers) and the in-cluster + // conversion config, which is never present in the manifest. + t.Run("update preserves finalizers and in-cluster conversion", func(t *testing.T) { + seed := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": map[string]any{ + "name": "things.example.com", + "finalizers": []any{"customresourcecleanup.apiextensions.k8s.io"}, + }, + "spec": map[string]any{ + "group": "example.com", + "names": map[string]any{ + "kind": "Thing", "listKind": "ThingList", "plural": "things", "singular": "thing", + }, + "scope": "Namespaced", + "conversion": map[string]any{ + "strategy": "Webhook", + "webhook": map[string]any{ + "conversionReviewVersions": []any{"v1"}, + "clientConfig": map[string]any{ + "service": map[string]any{"name": "conv", "namespace": "default"}, + }, + }, + }, + "versions": []any{ + map[string]any{"name": "v1", "served": true, "storage": true}, + }, + }, + }} + _, err := fc.Resource(gvr).Create(context.Background(), seed, apimachineryv1.CreateOptions{}) + require.NoError(t, err) + + inst := NewCRDsInstaller(fc, []string{"testdata/4_conversion.yaml"}) + require.NoError(t, inst.Run(context.Background())) + + un, err := fc.Resource(gvr).Get(context.Background(), "things.example.com", apimachineryv1.GetOptions{}) + require.NoError(t, err) + + finalizers, found, err := unstructured.NestedStringSlice(un.Object, "metadata", "finalizers") + require.NoError(t, err) + require.True(t, found, "finalizers must be preserved") + assert.Contains(t, finalizers, "customresourcecleanup.apiextensions.k8s.io") + + _, found, err = unstructured.NestedMap(un.Object, "spec", "conversion") + require.NoError(t, err) + assert.True(t, found, "in-cluster conversion must be preserved") + + versions, _, err := unstructured.NestedSlice(un.Object, "spec", "versions") + require.NoError(t, err) + token, found, err := unstructured.NestedMap(versions[0].(map[string]any), + "schema", "openAPIV3Schema", "properties", "spec", "properties", "token") + require.NoError(t, err) + require.True(t, found, "manifest schema must be applied") + assert.Equal(t, true, token["x-kubernetes-sensitive-data"]) + }) + + // Regression: a comment-only yaml document decodes to a nil object and must be skipped, + // not panic on the nil dereference. + t.Run("skips comment-only documents", func(t *testing.T) { + inst := NewCRDsInstaller(fc, []string{"testdata/5_comment.yaml"}) + require.NoError(t, inst.Run(context.Background())) + + _, err := fc.Resource(gvr).Get(context.Background(), "comments.example.com", apimachineryv1.GetOptions{}) + require.NoError(t, err) + }) } diff --git a/pkg/crd-installer/testdata/3_sensitive.yaml b/pkg/crd-installer/testdata/3_sensitive.yaml new file mode 100644 index 00000000..b23af4de --- /dev/null +++ b/pkg/crd-installer/testdata/3_sensitive.yaml @@ -0,0 +1,26 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: secrets.example.com +spec: + group: example.com + names: + kind: Secret + listKind: SecretList + plural: secrets + singular: secret + scope: Namespaced + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + token: + type: string + x-kubernetes-sensitive-data: true diff --git a/pkg/crd-installer/testdata/4_conversion.yaml b/pkg/crd-installer/testdata/4_conversion.yaml new file mode 100644 index 00000000..a28e21cc --- /dev/null +++ b/pkg/crd-installer/testdata/4_conversion.yaml @@ -0,0 +1,26 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: things.example.com +spec: + group: example.com + names: + kind: Thing + listKind: ThingList + plural: things + singular: thing + scope: Namespaced + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + token: + type: string + x-kubernetes-sensitive-data: true diff --git a/pkg/crd-installer/testdata/5_comment.yaml b/pkg/crd-installer/testdata/5_comment.yaml new file mode 100644 index 00000000..cd1aeca7 --- /dev/null +++ b/pkg/crd-installer/testdata/5_comment.yaml @@ -0,0 +1,21 @@ +# leading comment document — decodes to null and must be skipped, not crash +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: comments.example.com +spec: + group: example.com + names: + kind: Comment + listKind: CommentList + plural: comments + singular: comment + scope: Namespaced + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object