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()) +}