Merge pull request GoogleCloudPlatform#2808 from jasonvigil/fix-sqlinstance-labels

Fix sqlinstance user labels to filter out KRM-style labels

Author: google-oss-prow[bot]

pkg/cli/krmtohcl/krmtohcl.go

@@ -40,7 +40,7 @@ func UnstructuredToHCL(ctx context.Context, u *unstructured.Unstructured, smLoad
 	if err != nil {
 		return "", fmt.Errorf("could not parse resource %s: %w", u.GetName(), err)
 	}
-	config, _, err := krmtotf.KRMResourceToTFResourceConfigFull(krmResource, k8s.NewErroringClient(), smLoader, nil, nil, true, map[string]string{})
+	config, _, err := krmtotf.KRMResourceToTFResourceConfigFull(krmResource, k8s.NewErroringClient(), smLoader, nil, nil, true)
 	if err != nil {
 		return "", fmt.Errorf("error expanding resource configuration: %w", err)
 	}

pkg/cli/krmtohcl/testdata/computeinstance.golden.tf

@@ -32,6 +32,7 @@ resource "google_compute_instance" "computetargetpool_dep4" {
   labels = {
     cnrm-lease-expiration = "1603985453"
     cnrm-lease-holder-id  = "btpp498colih6qs1pe5g"
+    managed-by-cnrm       = "true"
   }
 
   machine_type            = "n1-standard-1"

pkg/cli/krmtohcl/testdata/containercluster.golden.tf

@@ -87,6 +87,10 @@ resource "google_container_cluster" "twenty_namespaces" {
     channel = "UNSPECIFIED"
   }
 
+  resource_labels = {
+    managed-by-cnrm = "true"
+  }
+
   subnetwork = "projects/my-project/regions/us-central1/subnetworks/default"
 
   workload_identity_config {

pkg/cli/krmtohcl/testdata/pubsubsubscription.golden.tf

@@ -25,6 +25,7 @@ resource "google_pubsub_subscription" "pubsubsubscription_sample" {
     cnrm-lease-expiration = "1603984859"
     cnrm-lease-holder-id  = "btpp498colih6qs1pe5g"
     label-one             = "value-one"
+    managed-by-cnrm       = "true"
   }
 
   message_retention_duration = "86400s"

pkg/cli/krmtohcl/testdata/storagebucket.golden.tf

@@ -15,7 +15,12 @@
  */
 
 resource "google_storage_bucket" "cc_cli" {
-  force_destroy               = false
+  force_destroy = false
+
+  labels = {
+    managed-by-cnrm = "true"
+  }
+
   location                    = "US"
   name                        = "cc-cli"
   project                     = "my-project"

pkg/cli/stream/testdata/expected-hcl-stream.golden.yaml

@@ -15,13 +15,23 @@
 resource "google_bigquery_dataset" "bigquerydatasetsamplec99k2khjhfu8dfrt53mr" {
   dataset_id                 = "bigquerydatasetsamplec99k2khjhfu8dfrt53mr"
   delete_contents_on_destroy = false
-  project                    = "cli-test-project"
+
+  labels = {
+    managed-by-cnrm = "true"
+  }
+
+  project = "cli-test-project"
 }
 # terraform import google_bigquery_dataset.bigquerydatasetsamplec99k2khjhfu8dfrt53mr projects/cli-test-project/datasets/bigquerydatasetsamplec99k2khjhfu8dfrt53mr
 resource "google_bigquery_dataset" "bigquerydatasetivs17e1s13s9ro2k9n1j" {
   dataset_id                 = "bigquerydatasetivs17e1s13s9ro2k9n1j"
   delete_contents_on_destroy = false
-  project                    = "cli-test-project"
+
+  labels = {
+    managed-by-cnrm = "true"
+  }
+
+  project = "cli-test-project"
 }
 # terraform import google_bigquery_dataset.bigquerydatasetivs17e1s13s9ro2k9n1j projects/cli-test-project/datasets/bigquerydatasetivs17e1s13s9ro2k9n1j
 resource "google_kms_key_ring" "key_ring_4dln90vis0b1c6f1jja9" {
@@ -31,18 +41,31 @@ resource "google_kms_key_ring" "key_ring_4dln90vis0b1c6f1jja9" {
 }
 # terraform import google_kms_key_ring.key_ring_4dln90vis0b1c6f1jja9 projects/cli-test-project/locations/us-central1/keyRings/key-ring-4dln90vis0b1c6f1jja9
 resource "google_compute_address" "computeaddress_1j26msq4eebfv63n6sil" {
+  labels = {
+    managed-by-cnrm = "true"
+  }
+
   name    = "computeaddress-1j26msq4eebfv63n6sil"
   project = "cli-test-project"
   region  = "us-central1"
 }
 # terraform import google_compute_address.computeaddress_1j26msq4eebfv63n6sil projects/cli-test-project/regions/us-central1/addresses/computeaddress-1j26msq4eebfv63n6sil
 resource "google_sql_database_instance" "sqluser_dep_7olc12tp9tdancsggr6s" {
-  name     = "sqluser-dep-7olc12tp9tdancsggr6s"
-  project  = "cli-test-project"
-  settings = {}
+  name    = "sqluser-dep-7olc12tp9tdancsggr6s"
+  project = "cli-test-project"
+
+  settings {
+    user_labels = {
+      managed-by-cnrm = "true"
+    }
+  }
 }
 # terraform import google_sql_database_instance.sqluser_dep_7olc12tp9tdancsggr6s projects/cli-test-project/instances/sqluser-dep-7olc12tp9tdancsggr6s
 resource "google_compute_address" "computeaddress_asdbgmb8o9mo6s075qp3" {
+  labels = {
+    managed-by-cnrm = "true"
+  }
+
   name    = "computeaddress-asdbgmb8o9mo6s075qp3"
   project = "cli-test-project"
   region  = "us-central1"
@@ -54,30 +77,51 @@ resource "google_compute_backend_service" "computebackendservice_8034p172pirpg3b
 }
 # terraform import google_compute_backend_service.computebackendservice_8034p172pirpg3bg31te projects/cli-test-project/global/backendServices/computebackendservice-8034p172pirpg3bg31te
 resource "google_sql_database_instance" "sqlinstance_sample_iocjd2jpebp9brf46rob" {
-  name     = "sqlinstance-sample-iocjd2jpebp9brf46rob"
-  project  = "cli-test-project"
-  settings = {}
+  name    = "sqlinstance-sample-iocjd2jpebp9brf46rob"
+  project = "cli-test-project"
+
+  settings {
+    user_labels = {
+      managed-by-cnrm = "true"
+    }
+  }
 }
 # terraform import google_sql_database_instance.sqlinstance_sample_iocjd2jpebp9brf46rob projects/cli-test-project/instances/sqlinstance-sample-iocjd2jpebp9brf46rob
 resource "google_compute_disk" "computedisk_39kl1g908j6npdp7pa9f" {
+  labels = {
+    managed-by-cnrm = "true"
+  }
+
   name    = "computedisk-39kl1g908j6npdp7pa9f"
   project = "cli-test-project"
   zone    = "us-central1-a"
 }
 # terraform import google_compute_disk.computedisk_39kl1g908j6npdp7pa9f projects/cli-test-project/zones/us-central1-a/disks/computedisk-39kl1g908j6npdp7pa9f
 resource "google_compute_disk" "computedisk_hiiupq1q4ujch81bj4g9" {
+  labels = {
+    managed-by-cnrm = "true"
+  }
+
   name    = "computedisk-hiiupq1q4ujch81bj4g9"
   project = "cli-test-project"
   zone    = "us-central1-a"
 }
 # terraform import google_compute_disk.computedisk_hiiupq1q4ujch81bj4g9 projects/cli-test-project/zones/us-central1-a/disks/computedisk-hiiupq1q4ujch81bj4g9
 resource "google_compute_forwarding_rule" "computeforwardingrule1_1j26msq4eebfv63n6sil" {
+  labels = {
+    managed-by-cnrm = "true"
+  }
+
   name    = "computeforwardingrule1-1j26msq4eebfv63n6sil"
   project = "cli-test-project"
   region  = "us-central1"
 }
 # terraform import google_compute_forwarding_rule.computeforwardingrule1_1j26msq4eebfv63n6sil projects/cli-test-project/regions/us-central1/forwardingRules/computeforwardingrule1-1j26msq4eebfv63n6sil
 resource "google_compute_global_forwarding_rule" "computeglobalforwardingrule_8034p172pirpg3bg31te" {
+  labels = {
+    managed-by-cnrm = "true"
+  }
+
   name    = "computeglobalforwardingrule-8034p172pirpg3bg31te"
   project = "cli-test-project"
 }
@@ -118,7 +162,12 @@ resource "google_compute_subnetwork" "default" {
 # terraform import google_compute_subnetwork.default projects/cli-test-project/regions/northamerica-northeast1/subnetworks/default
 resource "google_storage_bucket" "storagebucket_sample_696uso65hngc4js48ic9" {
   force_destroy = false
-  name          = "storagebucket-sample-696uso65hngc4js48ic9"
-  project       = "project-id-1"
+
+  labels = {
+    managed-by-cnrm = "true"
+  }
+
+  name    = "storagebucket-sample-696uso65hngc4js48ic9"
+  project = "project-id-1"
 }
 # terraform import google_storage_bucket.storagebucket_sample_696uso65hngc4js48ic9 storagebucket-sample-696uso65hngc4js48ic9

pkg/controller/direct/sql/mapping.go pkg/controller/direct/sql/sqlinstance_mappings.go

@@ -105,13 +105,7 @@ func SQLInstanceKRMToGCP(in *krm.SQLInstance, refs *SQLInstanceInternalRefs) (*a
 	out.Settings.SqlServerAuditConfig = InstanceSqlServerAuditConfigKRMToGCP(in.Spec.Settings.SqlServerAuditConfig, refs)
 	out.Settings.Tier = in.Spec.Settings.Tier
 	out.Settings.TimeZone = direct.ValueOf(in.Spec.Settings.TimeZone)
-
-	if in.Labels != nil {
-		out.Settings.UserLabels = make(map[string]string)
-		for k, v := range in.Labels {
-			out.Settings.UserLabels[k] = v
-		}
-	}
+	out.Settings.UserLabels = in.Labels
 
 	// TODO: Move to InstanceSettingsKRMToGCP
 	if in.Spec.Settings.DeletionProtectionEnabled != nil {

pkg/controller/direct/sql/merge.go pkg/controller/direct/sql/sqlinstance_merge.go

@@ -24,6 +24,7 @@ import (
 	storagev1beta1 "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/clients/generated/apis/storage/v1beta1"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/k8s"
+	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/label"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -70,9 +71,9 @@ func NormalizeSQLInstance(ctx context.Context, kube client.Reader, obj *krm.SQLI
 	if err != nil {
 		return nil, err
 	}
-	if err := normalizeLabels(obj); err != nil {
-		return nil, err
-	}
+	// Normalize labels.
+	obj.Labels = label.NewGCPLabelsFromK8sLabels(obj.Labels)
+	// Apply defaults.
 	if err := normalizeTFDefaults(obj); err != nil {
 		return nil, err
 	}
@@ -338,14 +339,6 @@ func normalizeSourceSQLInstanceRef(ctx context.Context, kube client.Reader, obj
 	}
 }
 
-func normalizeLabels(obj *krm.SQLInstance) error {
-	if obj.Labels == nil {
-		obj.Labels = make(map[string]string)
-	}
-	obj.Labels["managed-by-cnrm"] = "true"
-	return nil
-}
-
 func normalizeTFDefaults(obj *krm.SQLInstance) error {
 	if obj.Spec.Settings.ActivationPolicy == nil {
 		obj.Spec.Settings.ActivationPolicy = direct.PtrTo("ALWAYS")

pkg/controller/direct/sql/normalize.go pkg/controller/direct/sql/sqlinstance_normalize.go

@@ -34,7 +34,6 @@ import (
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/execution"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/k8s"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/krmtotf"
-	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/label"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/lease/leaser"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/resourceoverrides"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/resourceoverrides/operations"
@@ -336,7 +335,7 @@ func (r *Reconciler) sync(ctx context.Context, krmResource *krmtotf.Resource) (r
 			fmt.Errorf("underlying resource no longer exists and can't be recreated without creating a brand new resource"))
 	}
 	config, secretVersions, err := krmtotf.KRMResourceToTFResourceConfigFull(
-		krmResource, r, r.smLoader, liveState, r.schemaRef.JSONSchema, true, label.GetDefaultLabels(),
+		krmResource, r, r.smLoader, liveState, r.schemaRef.JSONSchema, true,
 	)
 	if err != nil {
 		if unwrappedErr, ok := lifecyclehandler.CausedByUnresolvableDeps(err); ok {

pkg/controller/tf/controller.go

@@ -793,7 +793,7 @@ func convertToDCLLabelsField(obj *unstructured.Unstructured, r *dclunstruct.Reso
 	if _, ok := schema.Properties[labelsField]; !ok {
 		return nil
 	}
-	labels := label.NewGCPLabelsFromK8SLabels(obj.GetLabels(), label.GetDefaultLabels())
+	labels := label.ToJSONCompatibleFormat(label.NewGCPLabelsFromK8sLabels(obj.GetLabels()))
 	if len(labels) != 0 {
 		r.Object[labelsField] = labels
 	}

pkg/dcl/conversion/converter.go

@@ -23,7 +23,6 @@ import (
 	corekccv1alpha1 "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/apis/core/v1alpha1"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/deepcopy"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/k8s"
-	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/label"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/servicemapping/servicemappingloader"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/text"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/util"
@@ -204,7 +203,7 @@ func WithFieldsPresetForRead(imported map[string]interface{}, r *Resource, kubeC
 	importedAsInstanceState := MapToInstanceState(r.TFResource, imported)
 	var jsonSchema *apiextensions.JSONSchemaProps
 	config, secretVersions, err = KRMResourceToTFResourceConfigFull(
-		r, kubeClient, smLoader, importedAsInstanceState, jsonSchema, mustResolveSensitiveFields, label.GetDefaultLabels(),
+		r, kubeClient, smLoader, importedAsInstanceState, jsonSchema, mustResolveSensitiveFields,
 	)
 	if err != nil {
 		return nil, fmt.Errorf("error converting resource config: %w", err)

pkg/krmtotf/fetchlivestate.go

@@ -44,7 +44,7 @@ import (
 // validation of either the input KRM or output TF is left as the
 // responsibility of other layers (e.g. webhooks, CRD schemas, GCP API, etc.)
 func KRMResourceToTFResourceConfig(r *Resource, c client.Client, smLoader *servicemappingloader.ServiceMappingLoader) (tfConfig *terraform.ResourceConfig, secretVersions map[string]string, err error) {
-	return KRMResourceToTFResourceConfigFull(r, c, smLoader, nil, nil, true, label.GetDefaultLabels())
+	return KRMResourceToTFResourceConfigFull(r, c, smLoader, nil, nil, true)
 }
 
 // KRMResourceToTFResourceConfigFull is a more flexible version of KRMResourceToTFResourceConfig,
@@ -55,7 +55,7 @@ func KRMResourceToTFResourceConfig(r *Resource, c client.Client, smLoader *servi
 //   - mustResolveSensitiveFields: if set, sensitive fields will be resolved.
 //   - defaultLabels: if set, these labels will be added to tfConfig.
 func KRMResourceToTFResourceConfigFull(r *Resource, c client.Client, smLoader *servicemappingloader.ServiceMappingLoader,
-	liveState *terraform.InstanceState, jsonSchema *apiextensions.JSONSchemaProps, mustResolveSensitiveFields bool, defaultLabels map[string]string) (tfConfig *terraform.ResourceConfig, secretVersions map[string]string, err error) {
+	liveState *terraform.InstanceState, jsonSchema *apiextensions.JSONSchemaProps, mustResolveSensitiveFields bool) (tfConfig *terraform.ResourceConfig, secretVersions map[string]string, err error) {
 	config := deepcopy.MapStringInterface(r.Spec)
 	if config == nil {
 		config = make(map[string]interface{})
@@ -74,7 +74,7 @@ func KRMResourceToTFResourceConfigFull(r *Resource, c client.Client, smLoader *s
 	}
 	if r.ResourceConfig.MetadataMapping.Labels != "" {
 		path := text.SnakeCaseToLowerCamelCase(r.ResourceConfig.MetadataMapping.Labels)
-		labels := label.NewGCPLabelsFromK8SLabels(r.GetLabels(), defaultLabels)
+		labels := label.ToJSONCompatibleFormat(label.NewGCPLabelsFromK8sLabels(r.GetLabels()))
 		if err := setValue(config, path, labels); err != nil {
 			return nil, nil, fmt.Errorf("error mapping 'metadata.labels': %w", err)
 		}

pkg/krmtotf/krmtotf.go

@@ -18,13 +18,14 @@ import (
 	"strings"
 )
 
-func NewGcpFromK8sLabels(labels map[string]string) map[string]string {
-	res := RemoveLabelsWithKRMPrefix(labels)
+func NewGCPLabelsFromK8sLabels(labels map[string]string) map[string]string {
+	res := removeLabelsWithKRMPrefix(labels)
+	// Apply default label.
 	res[CnrmManagedKey] = "true"
 	return res
 }
 
-func RemoveLabelsWithKRMPrefix(labels map[string]string) map[string]string {
+func removeLabelsWithKRMPrefix(labels map[string]string) map[string]string {
 	res := make(map[string]string)
 	for k, v := range labels {
 		if len(strings.Split(k, "/")) == 2 {
@@ -38,26 +39,11 @@ func RemoveLabelsWithKRMPrefix(labels map[string]string) map[string]string {
 	return res
 }
 
-func NewGCPLabelsFromK8SLabels(labelMaps ...map[string]string) map[string]interface{} {
+// Reformat labels map into a JSON-compatible map[string]interface{} type.
+func ToJSONCompatibleFormat(labels map[string]string) map[string]interface{} {
 	res := make(map[string]interface{})
-	for _, labels := range labelMaps {
-		if labels != nil {
-			for k, v := range labels {
-				if len(strings.Split(k, "/")) == 2 {
-					// Do not include any KRM-style labels (labels that include a prefix
-					// denoted with a '/').
-					// TODO(b/137755194): Determine long-term solution.
-					continue
-				}
-				res[k] = v
-			}
-		}
+	for k, v := range labels {
+		res[k] = v
 	}
 	return res
 }
-
-func GetDefaultLabels() map[string]string {
-	return map[string]string{
-		CnrmManagedKey: "true",
-	}
-}