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 Azure#4298.
Fixes Azure#4316.

v2/internal/controllers/controller_resources.go

@@ -275,7 +275,6 @@ func CreateScheme() *runtime.Scheme {
 	_ = mysqlv1.AddToScheme(scheme)
 	_ = postgresqlv1.AddToScheme(scheme)
 	_ = azuresqlv1.AddToScheme(scheme)
-	scheme.AllKnownTypes()
 	return scheme
 }
 

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

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

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")))
 }