Merge pull request #1768 from GoogleCloudPlatform/acpana/test-reconci…

…le-direct

tests:LoggingLogMetric: make dynamic reconciler work for direct resources

apis/resources/logging/v1beta1/logmetric_types.go

@@ -15,8 +15,6 @@
 package v1beta1
 
 import (
-	"reflect"
-
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/clients/generated/apis/k8s/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -33,7 +31,7 @@ var (
 	LoggingLogMetricGVK = schema.GroupVersionKind{
 		Group:   SchemeGroupVersion.Group,
 		Version: SchemeGroupVersion.Version,
-		Kind:    reflect.TypeOf(LoggingLogMetric{}).Name(),
+		Kind:    "LoggingLogMetric",
 	}
 )
 

pkg/controller/direct/apikeys/apikeyskey_controller.go

@@ -278,6 +278,10 @@ func (a *adapter) Update(ctx context.Context, u *unstructured.Unstructured) erro
 	return nil
 }
 
+func (a *adapter) Export(ctx context.Context) (*unstructured.Unstructured, error) {
+	return nil, nil
+}
+
 func (a *adapter) fullyQualifiedName() string {
 	return fmt.Sprintf("projects/%s/locations/%s/keys/%s", a.projectID, a.location, a.keyID)
 }

pkg/controller/direct/directbase/interfaces.go

@@ -51,4 +51,9 @@ type Adapter interface {
 	// This should only be called when Find has previously returned true.
 	// The implementation should write the updated status into `u`.
 	Update(ctx context.Context, u *unstructured.Unstructured) error
+
+	// Export fetches the cloud provider's representation of the object
+	// as an unstructured.Unstructured.
+	// Assumes Find has previously returned true.
+	Export(ctx context.Context) (*unstructured.Unstructured, error)
 }

pkg/controller/direct/logging/logmetric_controller.go

@@ -50,6 +50,14 @@ func AddLogMetricController(mgr manager.Manager, config *controller.Config, opts
 	return directbase.Add(mgr, gvk, m, opts)
 }
 
+func GetModel(ctx context.Context, config *controller.Config) (directbase.Model, error) {
+	gcpClient, err := newGCPClient(ctx, config)
+	if err != nil {
+		return nil, err
+	}
+	return &logMetricModel{gcpClient: gcpClient}, nil
+}
+
 type logMetricModel struct {
 	*gcpClient
 }
@@ -344,6 +352,49 @@ func convertToMicrotime(s string) (*metav1.MicroTime, error) {
 	return &v, nil
 }
 
+func (a *logMetricAdapter) Export(ctx context.Context) (*unstructured.Unstructured, error) {
+	if a.actual == nil {
+		return nil, fmt.Errorf("logMetric %q not found", a.fullyQualifiedName())
+	}
+
+	un, err := convertAPItoKRM_LoggingLogMetric(a.actual)
+	if err != nil {
+		return nil, fmt.Errorf("error converting logMetric to unstructured %w", err)
+	}
+
+	// TODO(acpana): revisit if we want to include mutable but unreadable fields in our export
+	if a.desired != nil {
+		if a.desired.Spec.MetricDescriptor != nil && a.desired.Spec.MetricDescriptor.LaunchStage != nil {
+			if err := unstructured.SetNestedField(un.Object,
+				*a.desired.Spec.MetricDescriptor.LaunchStage,
+				"spec", "metricDescriptor", "launchStage",
+			); err != nil {
+				return nil, fmt.Errorf("could not set metricDescriptor.launchStage mutable but unreadable field %w", err)
+			}
+		}
+		if a.desired.Spec.MetricDescriptor != nil && a.desired.Spec.MetricDescriptor.Metadata != nil {
+			if a.desired.Spec.MetricDescriptor.Metadata.IngestDelay != nil {
+				if err := unstructured.SetNestedField(un.Object,
+					*a.desired.Spec.MetricDescriptor.Metadata.IngestDelay,
+					"spec", "metricDescriptor", "metadata", "ingestDelay",
+				); err != nil {
+					return nil, fmt.Errorf("could not set metricDescriptor.metadata.ingestDelay mutable but unreadable field %w", err)
+				}
+			}
+			if a.desired.Spec.MetricDescriptor.Metadata.SamplePeriod != nil {
+				if err := unstructured.SetNestedField(un.Object,
+					*a.desired.Spec.MetricDescriptor.Metadata.SamplePeriod,
+					"spec", "metricDescriptor", "metadata", "samplePeriod",
+				); err != nil {
+					return nil, fmt.Errorf("could not set metricDescriptor.metadata.samplePeriod mutable but unreadable field %w", err)
+				}
+			}
+		}
+	}
+
+	return un, nil
+}
+
 func (a *logMetricAdapter) fullyQualifiedName() string {
 	return MakeFQN(a.projectID, a.resourceID)
 }

pkg/controller/direct/logging/utils.go

@@ -18,8 +18,11 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"sort"
 
 	krm "github.com/GoogleCloudPlatform/k8s-config-connector/apis/resources/logging/v1beta1"
+	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/clients/generated/apis/k8s/v1alpha1"
+	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/util"
 	"github.com/googleapis/gax-go/v2/apierror"
 	api "google.golang.org/api/logging/v2"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -111,6 +114,65 @@ func validateImmutableFieldsUpdated(kccObj *krm.LogmetricMetricDescriptor, apiOb
 	return nil
 }
 
+func convertAPItoKRM_LoggingLogMetric(in *api.LogMetric) (*unstructured.Unstructured, error) {
+	if in == nil {
+		return nil, fmt.Errorf("api logMetric is nil")
+	}
+
+	lm := &krm.LoggingLogMetric{}
+	lm.SetGroupVersionKind(krm.LoggingLogMetricGVK)
+	lm.SetName(in.Name)
+	// lm.SetNamespace(in.Namespace) // todo acpana figure out namespace setting
+
+	lm.Spec.Description = &in.Description
+	lm.Spec.Disabled = &in.Disabled
+	lm.Spec.Filter = in.Filter
+	lm.Spec.MetricDescriptor = convertAPItoKRM_MetricDescriptor(in.MetricDescriptor)
+	lm.Spec.LabelExtractors = in.LabelExtractors
+	lm.Spec.BucketOptions = convertAPItoKRM_BucketOptions(in.BucketOptions)
+	lm.Spec.ValueExtractor = &in.ValueExtractor
+	lm.Spec.LoggingLogBucketRef = &v1alpha1.ResourceRef{
+		External: in.BucketName,
+	}
+
+	u := &unstructured.Unstructured{}
+	if err := util.Marshal(lm, u); err != nil {
+		return nil, fmt.Errorf("error marshing logMetric to unstructured %w", err)
+	}
+
+	return u, nil
+}
+
+func convertAPItoKRM_BucketOptions(in *api.BucketOptions) *krm.LogmetricBucketOptions {
+	if in == nil {
+		return nil
+	}
+
+	options := &krm.LogmetricBucketOptions{}
+
+	if in.ExplicitBuckets != nil {
+		options.ExplicitBuckets = &krm.LogmetricExplicitBuckets{
+			Bounds: in.ExplicitBuckets.Bounds,
+		}
+	}
+	if in.ExponentialBuckets != nil {
+		options.ExponentialBuckets = &krm.LogmetricExponentialBuckets{
+			GrowthFactor:     &in.ExponentialBuckets.GrowthFactor,
+			NumFiniteBuckets: &in.ExponentialBuckets.NumFiniteBuckets,
+			Scale:            &in.ExponentialBuckets.Scale,
+		}
+	}
+	if in.LinearBuckets != nil {
+		options.LinearBuckets = &krm.LogmetricLinearBuckets{
+			NumFiniteBuckets: &in.LinearBuckets.NumFiniteBuckets,
+			Offset:           &in.LinearBuckets.Offset,
+			Width:            &in.LinearBuckets.Width,
+		}
+	}
+
+	return options
+}
+
 func convertAPItoKRM_MetricDescriptorStatus(apiObj *api.MetricDescriptor) *krm.LogmetricMetricDescriptorStatus {
 	if apiObj == nil {
 		return nil
@@ -157,7 +219,17 @@ func convertAPItoKRM_LogMetricLabels(apiLabels []*api.LabelDescriptor) []krm.Log
 			Key:         &apiLabel.Key,         // immutable
 			ValueType:   &apiLabel.ValueType,   // immutable
 		}
+
+		// this is a quirk of the API where the "STRING" default value gets returned as "".
+		if ValueOf(kccLabels[i].ValueType) == "" {
+			*kccLabels[i].ValueType = "STRING" // "" defaults to "STRING"
+		}
 	}
+
+	sort.Slice(kccLabels, func(i, j int) bool {
+		return *kccLabels[i].Key < *kccLabels[j].Key
+	})
+
 	return kccLabels
 }
 

pkg/controller/direct/resourcemanager/tagkey_controller.go

@@ -248,6 +248,10 @@ func (a *tagKeyAdapter) Update(ctx context.Context, u *unstructured.Unstructured
 	return nil
 }
 
+func (a *tagKeyAdapter) Export(ctx context.Context) (*unstructured.Unstructured, error) {
+	return nil, nil
+}
+
 func (a *tagKeyAdapter) fullyQualifiedName() string {
 	return fmt.Sprintf("tagKeys/%s", a.resourceID)
 }

pkg/controller/dynamic/dynamic_controller_integration_test.go

@@ -47,6 +47,8 @@ import (
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/test/resourcefixture/contexts"
 	testrunner "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/test/runner"
 	testservicemapping "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/test/servicemapping"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 
 	"github.com/cenkalti/backoff"
 	"github.com/ghodss/yaml"
@@ -238,9 +240,9 @@ func validateCreate(ctx context.Context, t *testing.T, testContext testrunner.Te
 	if err := kubeClient.Get(ctx, testContext.NamespacedName, reconciledUnstruct); err != nil {
 		t.Fatalf("unexpected error getting k8s resource: %v", err)
 	}
-	gcpUnstruct, err := resourceContext.Get(ctx, t, reconciledUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter)
+	gcpUnstruct, err := resourceContext.Get(ctx, t, reconciledUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter, systemContext.HttpClient)
 	if err != nil {
-		t.Fatalf("unexpected error when GETting '%v': %v", initialUnstruct.GetName(), err)
+		t.Fatalf("[validateCreate] unexpected error when GET-ing '%v': %v", initialUnstruct.GetName(), err)
 	}
 	t.Logf("created resource is %v\r", gcpUnstruct)
 	if resourceContext.SupportsLabels(systemContext.SMLoader) {
@@ -249,7 +251,10 @@ func validateCreate(ctx context.Context, t *testing.T, testContext testrunner.Te
 
 	// Check that an "Updating" event was recorded, indicating that the
 	// controller tried to update the resource at all.
-	testcontroller.AssertEventRecordedforUnstruct(t, kubeClient, reconciledUnstruct, k8s.Updating)
+	// TODO(acpana): figure out if we want to expose Updating event for direct resources
+	if !resourceContext.IsDirectResource() {
+		testcontroller.AssertEventRecordedforUnstruct(t, kubeClient, reconciledUnstruct, k8s.Updating)
+	}
 
 	// Check that condition is ready and "UpToDate" event was recorded
 	// TODO: (eventually) check default fields are propagated correctly
@@ -376,9 +381,9 @@ func testUpdate(ctx context.Context, t *testing.T, testContext testrunner.TestCo
 	}
 
 	// Check labels match on update
-	gcpUnstruct, err := resourceContext.Get(ctx, t, reconciledUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter)
+	gcpUnstruct, err := resourceContext.Get(ctx, t, reconciledUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter, nil)
 	if err != nil {
-		t.Fatalf("unexpected error when GETting '%v': %v", updateUnstruct.GetName(), err)
+		t.Fatalf("[testUpdate] unexpected error when GET-ing '%v': %v", updateUnstruct.GetName(), err)
 	}
 	if resourceContext.SupportsLabels(systemContext.SMLoader) {
 		testcontroller.AssertLabelsMatchAndHaveManagedLabel(t, gcpUnstruct.GetLabels(), testContext.UpdateUnstruct.GetLabels())
@@ -396,8 +401,10 @@ func testUpdate(ctx context.Context, t *testing.T, testContext testrunner.TestCo
 
 	// Check that an "Updating" event was recorded, indicating that the
 	// controller tried to update the resource at all.
-	testcontroller.AssertEventRecordedforUnstruct(t, kubeClient, reconciledUnstruct, k8s.Updating)
-
+	// TODO(acpana): figure out if we want to expose Updating event for direct resources
+	if !resourceContext.IsDirectResource() {
+		testcontroller.AssertEventRecordedforUnstruct(t, kubeClient, reconciledUnstruct, k8s.Updating)
+	}
 	// Check if condition is ready and update event was recorded
 	testcontroller.AssertReadyCondition(t, reconciledUnstruct, preReconcileGeneration)
 	testcontroller.AssertEventRecordedforUnstruct(t, kubeClient, reconciledUnstruct, k8s.UpToDate)
@@ -427,7 +434,7 @@ func testDriftCorrection(ctx context.Context, t *testing.T, testContext testrunn
 	testcontroller.DeleteAllEventsForUnstruct(t, kubeClient, testUnstruct)
 
 	t.Logf("testDriftCorrection: deleting kube object %v", testUnstruct)
-	if err := resourceContext.Delete(ctx, t, testUnstruct, systemContext.TFProvider, systemContext.Manager.GetClient(), systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter); err != nil {
+	if err := resourceContext.Delete(ctx, t, testUnstruct, systemContext.TFProvider, systemContext.Manager.GetClient(), systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter, systemContext.HttpClient); err != nil {
 		t.Fatalf("error deleting: %v", err)
 	}
 	// Underlying APIs may not have strongly-consistent reads due to caching. Sleep before attempting a re-reconcile, to
@@ -491,7 +498,7 @@ func testDelete(ctx context.Context, t *testing.T, testContext testrunner.TestCo
 	if err := kubeClient.Get(ctx, testContext.NamespacedName, reconciledUnstruct); err != nil {
 		t.Fatalf("unexpected error getting k8s resource: %v", err)
 	}
-	if _, err := resourceContext.Get(ctx, t, reconciledUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter); err != nil {
+	if _, err := resourceContext.Get(ctx, t, reconciledUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter, nil); err != nil {
 		t.Errorf("expected resource %s to not be deleted with deletion defender finalizer, but got error: %s",
 			initialUnstruct.GetName(), err)
 	}
@@ -501,14 +508,14 @@ func testDelete(ctx context.Context, t *testing.T, testContext testrunner.TestCo
 	testReconciler.Reconcile(ctx, reconciledUnstruct, testreconciler.ExpectedSuccessfulReconcileResultFor(systemContext.Reconciler, reconciledUnstruct), nil)
 
 	if !testgcp.ResourceSupportsDeletion(testContext.ResourceFixture.GVK.Kind) {
-		_, err := resourceContext.Get(ctx, t, reconciledUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter)
+		_, err := resourceContext.Get(ctx, t, reconciledUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter, nil)
 		if err != nil {
 			t.Errorf("expected resource %s to exist after deletion, but got error: %s", initialUnstruct.GetName(), err)
 		}
 	} else {
 		getFunc := func() error {
 			// for some resources, Get after Delete is eventually consistent, for that reason we retry until an error is returned
-			_, err := resourceContext.Get(ctx, t, reconciledUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter)
+			_, err := resourceContext.Get(ctx, t, reconciledUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter, nil)
 			if err == nil {
 				return fmt.Errorf("expected error, instead got 'nil'")
 			}
@@ -592,10 +599,10 @@ func testReconcileAcquire(ctx context.Context, t *testing.T, testContext testrun
 	}
 	var gcpUnstruct *unstructured.Unstructured
 	var err error
-	gcpUnstruct, err = resourceContext.Get(ctx, t, unstructToCreate, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter)
+	gcpUnstruct, err = resourceContext.Get(ctx, t, unstructToCreate, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter, nil)
 	if err != nil {
 		if !strings.Contains(err.Error(), "not found") {
-			t.Fatalf("unexpected error when GETting '%v': %v", unstructToCreate.GetName(), err)
+			t.Fatalf("[testReconcileAcquire] unexpected error when GET-ing '%v': %v", unstructToCreate.GetName(), err)
 		}
 		if gcpUnstruct, err = resourceContext.Create(ctx, t, unstructToCreate, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter); err != nil {
 			t.Fatalf("unexpected error when creating GCP resource '%v': %v", unstructToCreate.GetName(), err)
@@ -635,9 +642,9 @@ func testReconcileAcquire(ctx context.Context, t *testing.T, testContext testrun
 
 	// Check labels match
 	if resourceContext.SupportsLabels(systemContext.SMLoader) {
-		gcpUnstruct, err := resourceContext.Get(ctx, t, initialUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter)
+		gcpUnstruct, err := resourceContext.Get(ctx, t, initialUnstruct, systemContext.TFProvider, kubeClient, systemContext.SMLoader, systemContext.DCLConfig, systemContext.DCLConverter, nil)
 		if err != nil {
-			t.Fatalf("unexpected error when GETting '%v': %v", initialUnstruct.GetName(), err)
+			t.Fatalf("[testReconcileAcquire 2] unexpected error when GET-ing '%v': %v", initialUnstruct.GetName(), err)
 		}
 		testcontroller.AssertLabelsMatchAndHaveManagedLabel(t, gcpUnstruct.GetLabels(), initialUnstruct.GetLabels())
 	}
@@ -728,8 +735,12 @@ func assertObjectContains(t *testing.T, obj, changedFields map[string]interface{
 			}
 			assertObjectContains(t, objVal.(map[string]interface{}), changedVal.(map[string]interface{}))
 		default:
-			if !reflect.DeepEqual(objVal, changedVal) {
-				t.Fatalf("unexpected value for %v: got %v, want %v", changedKey, objVal, changedVal)
+			if diff := cmp.Diff(objVal, changedVal, cmpopts.SortSlices(
+				func(a, b interface{}) bool {
+					return fmt.Sprintf("%v", a) < fmt.Sprintf("%v", b)
+				}),
+			); diff != "" {
+				t.Fatalf("unexpected diff: %v", diff)
 			}
 		}
 	}

pkg/controller/iam/partialpolicy/iampartialpolicy_controller_integration_test.go

@@ -77,7 +77,7 @@ var resourceLevelIAMPartialPolicyTestFunc = func(ctx context.Context, t *testing
 	serviceMetaLoader := dclmetadata.New()
 	converter := conversion.New(dclSchemaLoader, serviceMetaLoader)
 	iamClient := kcciamclient.New(provider, smLoader, kubeClient, converter, dclConfig)
-	reconciler := testreconciler.NewForDCLAndTFTestReconciler(t, mgr, provider, dclConfig)
+	reconciler := testreconciler.NewTestReconciler(t, mgr, provider, dclConfig, nil)
 
 	// Create two service accounts to construct different update cases
 	serviceAccountName1 := fmt.Sprintf("%v-%v", "sa1", rand.Intn(1000000))
@@ -269,7 +269,7 @@ func TestReconcileIAMPartialPolicyResourceLevelCreateNoChangesUpdateDeleteWithSI
 		serviceMetaLoader := dclmetadata.New()
 		converter := conversion.New(dclSchemaLoader, serviceMetaLoader)
 		iamClient := kcciamclient.New(provider, smLoader, kubeClient, converter, dclConfig)
-		reconciler := testreconciler.NewForDCLAndTFTestReconciler(t, mgr, provider, dclConfig)
+		reconciler := testreconciler.NewTestReconciler(t, mgr, provider, dclConfig, nil)
 		testMembers := []iamv1beta1.IAMPartialPolicyMember{
 			{
 				Member: iamv1beta1.Member("group:[email protected]"),

pkg/controller/iam/policy/iampolicy_controller_integration_test.go

@@ -86,7 +86,7 @@ var resourceLevelIAMPolicyTestFunc = func(ctx context.Context, t *testing.T, _ s
 	serviceMetaLoader := dclmetadata.New()
 	converter := conversion.New(dclSchemaLoader, serviceMetaLoader)
 	iamClient := kcciamclient.New(provider, smLoader, kubeClient, converter, dclConfig)
-	reconciler := testreconciler.NewForDCLAndTFTestReconciler(t, mgr, provider, dclConfig)
+	reconciler := testreconciler.NewTestReconciler(t, mgr, provider, dclConfig, nil)
 
 	testReconcileResourceLevelCreateNoChangesUpdateDelete(ctx, t, kubeClient, k8sPolicy, newK8sPolicy, iamClient, reconciler)
 }
@@ -136,7 +136,7 @@ func TestReconcileIAMPolicyResourceLevelCreateNoChangesUpdateDeleteWithSISMerge(
 		serviceMetaLoader := dclmetadata.New()
 		converter := conversion.New(dclSchemaLoader, serviceMetaLoader)
 		iamClient := kcciamclient.New(provider, smLoader, kubeClient, converter, dclConfig)
-		reconciler := testreconciler.NewForDCLAndTFTestReconciler(t, mgr, provider, dclConfig)
+		reconciler := testreconciler.NewTestReconciler(t, mgr, provider, dclConfig, nil)
 
 		testReconcileResourceLevelCreateNoChangesUpdateDelete(ctx, t, kubeClient, k8sPolicy, newK8sPolicy, iamClient, reconciler)
 	}

pkg/controller/mocktests/secretmanager_secret_test.go

@@ -131,7 +131,7 @@ func TestSecretManagerSecretVersion(t *testing.T) {
 	}
 
 	t.Logf("creating testreconciler")
-	testhelper := testreconciler.NewForDCLAndTFTestReconciler(t, mgr, tfProvider, dclConfig)
+	testhelper := testreconciler.NewTestReconciler(t, mgr, tfProvider, dclConfig, nil)
 
 	for _, object := range objects {
 		gvk := object.GetObjectKind().GroupVersionKind()