From f209def693ddac4144e63a0d42074c178ea38d0f Mon Sep 17 00:00:00 2001 From: Matthew Christopher Date: Wed, 9 Oct 2024 17:00:10 -0700 Subject: [PATCH] Fix bug where some ConfigMap and Secret references could be missed The ASO controller uses reflection to look through the resource and find the ConfigMapReference and SecretReference fields. This reflection could miss SecretReference and ConfigMapReference fields that had been serialized into the "PropertyBag" property on the storage type. This was most common when working with preview APIs that had added new ConfigMapReference or SecretReference fields that don't exist in the GA api version. The fix is to use the converted version for reflection-based discovery. Fixes #4298. Fixes #4316. --- .../azure_generic_arm_reconciler_instance.go | 62 ++------- v2/internal/testcommon/scheme.go | 27 +++- .../azure_generic_arm_reconciler_test.go | 126 ++++++++++++++++-- v2/pkg/genruntime/convertible.go | 105 +++++++++++++++ v2/pkg/genruntime/convertible_spec.go | 1 + v2/pkg/tests/convertible_test.go | 34 +++++ 6 files changed, 290 insertions(+), 65 deletions(-) create mode 100644 v2/pkg/genruntime/convertible.go create mode 100644 v2/pkg/tests/convertible_test.go diff --git a/v2/internal/reconcilers/arm/azure_generic_arm_reconciler_instance.go b/v2/internal/reconcilers/arm/azure_generic_arm_reconciler_instance.go index 18d6ab9dd98..4e993be5ca7 100644 --- a/v2/internal/reconcilers/arm/azure_generic_arm_reconciler_instance.go +++ b/v2/internal/reconcilers/arm/azure_generic_arm_reconciler_instance.go @@ -15,11 +15,9 @@ import ( "github.com/go-logr/logr" "github.com/pkg/errors" v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/conversion" "github.com/Azure/azure-service-operator/v2/internal/genericarmclient" . "github.com/Azure/azure-service-operator/v2/internal/logging" @@ -671,7 +669,12 @@ func (r *azureDeploymentReconcilerInstance) saveAssociatedKubernetesResources(ct } // Also check if the resource itself implements KubernetesExporter - exporter, ok := r.ObjAsKubernetesExporter() + versionedMeta, err := genruntime.ObjAsOriginalVersion(r.Obj, r.ResourceResolver.Scheme()) + if err != nil { + return err + } + + exporter, ok := versionedMeta.(genruntime.KubernetesExporter) if ok { var additionalResources []client.Object additionalResources, err = exporter.ExportKubernetesResources(ctx, r.Obj, r.ARMConnection.Client(), r.Log) @@ -713,52 +716,12 @@ func (r *azureDeploymentReconcilerInstance) saveAssociatedKubernetesResources(ct return nil } -// ObjAsKubernetesExporter returns r.Obj as a genruntime.KubernetesExporter if it supports that interface, respecting -// the original API version used to create the resource. -func (r *azureDeploymentReconcilerInstance) ObjAsKubernetesExporter() (genruntime.KubernetesExporter, bool) { - resource := r.Obj - resourceGVK := resource.GetObjectKind().GroupVersionKind() - - desiredGVK := genruntime.GetOriginalGVK(resource) - if resourceGVK != desiredGVK { - // Need to convert the hub resource we have to the original version used - scheme := r.ResourceResolver.Scheme() - versionedResource, err := genruntime.NewEmptyVersionedResourceFromGVK(scheme, desiredGVK) - if err != nil { - r.Log.V(Status).Info( - "Unable to create expected resource version", - "have", resourceGVK, - "desired", desiredGVK) - return nil, false - } - - if convertible, ok := versionedResource.(conversion.Convertible); ok { - hub := resource.(conversion.Hub) - err := convertible.ConvertFrom(hub) - if err != nil { - r.Log.V(Status).Info( - "Unable to convert resource to expected version", - "original", resourceGVK, - "destination", desiredGVK) - return nil, false - } - } - - resource = versionedResource - } - - // Now test whether we support the interface - result, ok := resource.(genruntime.KubernetesExporter) - return result, ok -} - // ConvertResourceToARMResource converts a genruntime.ARMMetaObject (a Kubernetes representation of a resource) into // a genruntime.ARMResourceSpec - a specification which can be submitted to Azure for deployment func (r *azureDeploymentReconcilerInstance) ConvertResourceToARMResource(ctx context.Context) (genruntime.ARMResource, error) { metaObject := r.Obj - scheme := r.ResourceResolver.Scheme() - result, err := ConvertToARMResourceImpl(ctx, metaObject, scheme, r.ResourceResolver, r.ARMConnection.SubscriptionID()) + result, err := ConvertToARMResourceImpl(ctx, metaObject, r.ResourceResolver, r.ARMConnection.SubscriptionID()) if err != nil { return nil, err } @@ -772,21 +735,24 @@ func (r *azureDeploymentReconcilerInstance) ConvertResourceToARMResource(ctx con func ConvertToARMResourceImpl( ctx context.Context, metaObject genruntime.ARMMetaObject, - scheme *runtime.Scheme, resolver *resolver.Resolver, subscriptionID string, ) (genruntime.ARMResource, error) { - spec, err := genruntime.GetVersionedSpec(metaObject, scheme) + // This calls ObjAsOriginalVersion which technically is doing more work than strictly needed, as it converts the spec, + // metadata, and status. We need the spec and metadata but don't strictly need the status converted at this point. + // We could look to do some future optimizations here to avoid conversion of status if it ever becomes an issue. + versionedMeta, err := genruntime.ObjAsOriginalVersion(metaObject, resolver.Scheme()) if err != nil { - return nil, errors.Wrapf(err, "unable to get spec from %s", metaObject.GetObjectKind().GroupVersionKind()) + return nil, err } + spec := versionedMeta.GetSpec() armTransformer, ok := spec.(genruntime.ARMTransformer) if !ok { return nil, errors.Errorf("spec was of type %T which doesn't implement genruntime.ArmTransformer", spec) } - resourceHierarchy, resolvedDetails, err := resolver.ResolveAll(ctx, metaObject) + resourceHierarchy, resolvedDetails, err := resolver.ResolveAll(ctx, versionedMeta) if err != nil { return nil, reconcilers.ClassifyResolverError(err) } diff --git a/v2/internal/testcommon/scheme.go b/v2/internal/testcommon/scheme.go index 7ea213ee9aa..86118c9d2c7 100644 --- a/v2/internal/testcommon/scheme.go +++ b/v2/internal/testcommon/scheme.go @@ -6,22 +6,37 @@ Licensed under the MIT license. package testcommon import ( + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" batch "github.com/Azure/azure-service-operator/v2/api/batch/v1api20210101" + batchstorage "github.com/Azure/azure-service-operator/v2/api/batch/v1api20210101/storage" + dbforpostgresqlstorage "github.com/Azure/azure-service-operator/v2/api/dbforpostgresql/v1api20221201/storage" + dbforpostgresql "github.com/Azure/azure-service-operator/v2/api/dbforpostgresql/v1api20230601preview" resources "github.com/Azure/azure-service-operator/v2/api/resources/v1api20200601" + resourcesstorage "github.com/Azure/azure-service-operator/v2/api/resources/v1api20200601/storage" ) func CreateScheme() (*runtime.Scheme, error) { scheme := runtime.NewScheme() - err := batch.AddToScheme(scheme) - if err != nil { - return nil, err + + addToScheme := []func(scheme *runtime.Scheme) error{ + batch.AddToScheme, + batchstorage.AddToScheme, + resources.AddToScheme, + resourcesstorage.AddToScheme, + // These dbforpostgresql resources are an example that illustrates + // https://github.com/Azure/azure-service-operator/issues/4316 + dbforpostgresql.AddToScheme, + dbforpostgresqlstorage.AddToScheme, + v1.AddToScheme, } - err = resources.AddToScheme(scheme) - if err != nil { - return nil, err + for _, f := range addToScheme { + err := f(scheme) + if err != nil { + return nil, err + } } return scheme, nil diff --git a/v2/internal/tests/azure_generic_arm_reconciler_test.go b/v2/internal/tests/azure_generic_arm_reconciler_test.go index f714d923951..b298d45f729 100644 --- a/v2/internal/tests/azure_generic_arm_reconciler_test.go +++ b/v2/internal/tests/azure_generic_arm_reconciler_test.go @@ -10,16 +10,30 @@ import ( "testing" . "github.com/onsi/gomega" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/conversion" + dbforpostgresqlstorage "github.com/Azure/azure-service-operator/v2/api/dbforpostgresql/v1api20221201/storage" + dbforpostgresql "github.com/Azure/azure-service-operator/v2/api/dbforpostgresql/v1api20230601preview" "github.com/Azure/azure-service-operator/v2/internal/reconcilers/arm" + "github.com/Azure/azure-service-operator/v2/internal/resolver" "github.com/Azure/azure-service-operator/v2/internal/testcommon" + "github.com/Azure/azure-service-operator/v2/internal/util/to" + "github.com/Azure/azure-service-operator/v2/pkg/genruntime" ) -func Test_ConvertResourceToARMResource(t *testing.T) { - t.Parallel() - g := NewGomegaWithT(t) - ctx := context.Background() +type testData struct { + scheme *runtime.Scheme + client client.Client + resolver *resolver.Resolver + subscriptionID string +} +func testSetup(g *WithT) *testData { scheme, err := testcommon.CreateScheme() g.Expect(err).ToNot(HaveOccurred()) @@ -28,18 +42,108 @@ func Test_ConvertResourceToARMResource(t *testing.T) { resolver, err := testcommon.CreateResolver(scheme, testClient) g.Expect(err).ToNot(HaveOccurred()) + subscriptionID := "00000000-0000-0000-0000-000000000000" + + return &testData{ + scheme: scheme, + client: testClient, + resolver: resolver, + subscriptionID: subscriptionID, + } +} + +func Test_ConvertResourceToARMResource(t *testing.T) { + t.Parallel() + g := NewGomegaWithT(t) + ctx := context.Background() + + testData := testSetup(g) + rg := testcommon.CreateResourceGroup() - g.Expect(testClient.Create(ctx, rg)).To(Succeed()) + g.Expect(testData.client.Create(ctx, rg)).To(Succeed()) account := testcommon.CreateDummyResource() - g.Expect(testClient.Create(ctx, account)).To(Succeed()) + g.Expect(testData.client.Create(ctx, account)).To(Succeed()) - subscriptionID := "00000000-0000-0000-0000-000000000000" + resource, err := arm.ConvertToARMResourceImpl(ctx, account, testData.resolver, testData.subscriptionID) + g.Expect(err).ToNot(HaveOccurred()) - resource, err := arm.ConvertToARMResourceImpl(ctx, account, scheme, resolver, subscriptionID) + g.Expect(resource.Spec().GetName()).To(Equal("azureName")) + g.Expect(resource.Spec().GetAPIVersion()).To(Equal("2021-01-01")) + g.Expect(resource.Spec().GetType()).To(Equal("Microsoft.Batch/batchAccounts")) +} + +func Test_Conversion_DiscoversConfigMapsNotOnStorageVersion(t *testing.T) { + t.Parallel() + g := NewGomegaWithT(t) + ctx := context.Background() + + // ensure that the storage type is the hub type + var _ conversion.Hub = &dbforpostgresqlstorage.FlexibleServer{} + + testData := testSetup(g) + + rg := testcommon.CreateResourceGroup() + g.Expect(testData.client.Create(ctx, rg)).To(Succeed()) + + configMap := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myconfig", + Namespace: rg.Namespace, + }, + Data: map[string]string{ + "breakfast": "bagel", + }, + } + g.Expect(testData.client.Create(ctx, configMap)).To(Succeed()) + + db2 := &dbforpostgresql.FlexibleServer{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: rg.Namespace, + Name: "mydb", + }, + Spec: dbforpostgresql.FlexibleServer_Spec{ + AzureName: "mydb", + Location: to.Ptr("westus"), + Owner: &genruntime.KnownResourceReference{ + Name: "myrg", + }, + DataEncryption: &dbforpostgresql.DataEncryption{ + GeoBackupKeyURIFromConfig: &genruntime.ConfigMapReference{ + Name: "myconfig", + Key: "breakfast", + }, + }, + }, + } + + storageVersion, err := genruntime.ObjAsVersion( + db2, + testData.scheme, + schema.GroupVersionKind{ + Group: dbforpostgresqlstorage.GroupVersion.Group, + Version: dbforpostgresqlstorage.GroupVersion.Version, + Kind: "FlexibleServer", + }) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(genruntime.GetOriginalGVK(storageVersion)).To(Equal(schema.GroupVersionKind{ + Group: dbforpostgresql.GroupVersion.Group, + Version: dbforpostgresql.GroupVersion.Version, + Kind: "FlexibleServer", + })) + + // Convert db to the storage type to simulate what would happen during reconciliation + resource, err := arm.ConvertToARMResourceImpl(ctx, storageVersion, testData.resolver, testData.subscriptionID) g.Expect(err).ToNot(HaveOccurred()) - g.Expect("azureName").To(Equal(resource.Spec().GetName())) - g.Expect("2021-01-01").To(Equal(resource.Spec().GetAPIVersion())) - g.Expect("Microsoft.Batch/batchAccounts").To(Equal(resource.Spec().GetType())) + armType, ok := resource.Spec().(*dbforpostgresql.FlexibleServer_Spec_ARM) + g.Expect(ok).To(BeTrue(), "Expected resource.Spec() to be of type FlexibleServer_Spec_ARM") + + g.Expect(resource.Spec().GetName()).To(Equal("mydb")) + g.Expect(resource.Spec().GetAPIVersion()).To(Equal("2023-06-01-preview")) + g.Expect(resource.Spec().GetType()).To(Equal("Microsoft.DBforPostgreSQL/flexibleServers")) + + // Ensure that the property that exists only in the preview API but not in the storage version was found + g.Expect(armType.Properties.DataEncryption.GeoBackupKeyURI).To(Equal(to.Ptr("bagel"))) } diff --git a/v2/pkg/genruntime/convertible.go b/v2/pkg/genruntime/convertible.go new file mode 100644 index 00000000000..635022fa8d4 --- /dev/null +++ b/v2/pkg/genruntime/convertible.go @@ -0,0 +1,105 @@ +/* + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT license. + */ + +package genruntime + +import ( + "reflect" + + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/conversion" +) + +func findStorageGVK(scheme *runtime.Scheme, gk schema.GroupKind) (schema.GroupVersionKind, error) { + interfaceType := reflect.TypeOf((*conversion.Hub)(nil)).Elem() + + for gvk, rt := range scheme.AllKnownTypes() { + if gvk.Group != gk.Group || gvk.Kind != gk.Kind { + continue + } + + if reflect.PointerTo(rt).Implements(interfaceType) { + return gvk, nil + } + } + + return schema.GroupVersionKind{}, errors.Errorf("couldn't find hub type for group kind %s", gk.String()) +} + +// ObjAsOriginalVersion returns the obj as the original API version used to create it. +func ObjAsOriginalVersion(obj ARMMetaObject, scheme *runtime.Scheme) (ARMMetaObject, error) { + return ObjAsVersion(obj, scheme, GetOriginalGVK(obj)) +} + +// ObjAsVersion returns the object as the specified version, or an error if it cannot be converted to the +// requested version. +func ObjAsVersion(obj ARMMetaObject, scheme *runtime.Scheme, gvk schema.GroupVersionKind) (ARMMetaObject, error) { + objGVK := obj.GetObjectKind().GroupVersionKind() + + versionedResource, err := NewEmptyVersionedResourceFromGVK(scheme, gvk) + if err != nil { + return nil, errors.Wrap(err, "getting empty versioned resource") + } + if objGVK == gvk { + // No conversion needed, resource GVK is the same as what we want + return obj, nil + } + + // if the obj isn't the storage version (doesn't implement conversion.Hub) then we find the version that is the storage + // version, construct an empty one and convert obj to the storage version. + hub, ok := obj.(conversion.Hub) + if !ok { + // Note that if this function is called by a control-loop, the expectation is that the control-loop + // is operating on the storage/hub version already, so it shouldn't be necessarily to take this code path. + gk := schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind} + var storageGVK schema.GroupVersionKind + storageGVK, err = findStorageGVK(scheme, gk) + if err != nil { + return nil, errors.Wrapf(err, "couldn't find storage GVK for %s", gk) + } + + var storageObj ARMMetaObject + storageObj, err = NewEmptyVersionedResourceFromGVK(scheme, storageGVK) + if err != nil { + return nil, errors.Wrap(err, "getting empty hub versioned resource") + } + + hub, ok = storageObj.(conversion.Hub) + if !ok { + return nil, errors.Errorf("storage object %T with GVK %s is not a Hub object", storageObj, storageGVK) + } + + if convertible, ok := obj.(conversion.Convertible); ok { + err = convertible.ConvertTo(hub) + if err != nil { + return nil, errors.Wrapf(err, "couldn't convert %s to hub", objGVK) + } + } else { + // This is unexpected/a bug + return nil, errors.Errorf("obj %T was not convertible", obj) + } + } + + // Special case, if the destination is the hub we can just return here because we've already got it + if _, ok := versionedResource.(conversion.Hub); ok { + return hub.(ARMMetaObject), nil + } + + // Convert from the storage version to the destination version. + if convertible, ok := versionedResource.(conversion.Convertible); ok { + err = convertible.ConvertFrom(hub) + if err != nil { + return nil, errors.Wrapf(err, "unable to convert resource to expected version. have: %s, want: %s", objGVK, gvk) + } + } else { + return nil, errors.Errorf("obj %T was not convertible", versionedResource) + } + + obj = versionedResource + + return obj, nil +} diff --git a/v2/pkg/genruntime/convertible_spec.go b/v2/pkg/genruntime/convertible_spec.go index 49267206a98..1d5ef49aabb 100644 --- a/v2/pkg/genruntime/convertible_spec.go +++ b/v2/pkg/genruntime/convertible_spec.go @@ -46,6 +46,7 @@ type ConvertibleSpec interface { // GetVersionedSpec returns a versioned spec for the provided resource; the original API version used when the // resource was first created is used to identify the version to return +// TODO: This is currently unused func GetVersionedSpec(metaObject ARMMetaObject, scheme *runtime.Scheme) (ConvertibleSpec, error) { return GetVersionedSpecFromGVK(metaObject, scheme, GetOriginalGVK(metaObject)) } diff --git a/v2/pkg/tests/convertible_test.go b/v2/pkg/tests/convertible_test.go new file mode 100644 index 00000000000..b9d38b9e342 --- /dev/null +++ b/v2/pkg/tests/convertible_test.go @@ -0,0 +1,34 @@ +/* + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT license. + */ + +package tests + +import ( + "testing" + + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/runtime" + + batch "github.com/Azure/azure-service-operator/v2/api/batch/v1api20210101" + batchstorage "github.com/Azure/azure-service-operator/v2/api/batch/v1api20210101/storage" + "github.com/Azure/azure-service-operator/v2/pkg/genruntime" +) + +func TestObjAsOriginalVersion_WorksWhenNoPivotNeeded(t *testing.T) { + t.Parallel() + g := NewGomegaWithT(t) + + scheme := runtime.NewScheme() + err := batch.AddToScheme(scheme) + g.Expect(err).To(Succeed()) + err = batchstorage.AddToScheme(scheme) + g.Expect(err).To(Succeed()) + + account := &batch.BatchAccount{} + + rsrc, err := genruntime.ObjAsOriginalVersion(account, scheme) + g.Expect(err).To(Succeed()) + g.Expect(rsrc).NotTo(BeNil()) +}