Merge pull request GoogleCloudPlatform#2994 from jasonvigil/sqlinstan…

…ce-defaults-p2

fix: Assign default values for SQLInstance to fix sqlinstance-activationpolicy-direct

pkg/controller/direct/sql/sqlinstance_controller.go

@@ -212,7 +212,7 @@ func (a *sqlInstanceAdapter) cloneInstance(ctx context.Context, u *unstructured.
 }
 
 func (a *sqlInstanceAdapter) insertInstance(ctx context.Context, u *unstructured.Unstructured, log klog.Logger) error {
-	desiredGCP, err := SQLInstanceKRMToGCP(a.desired)
+	desiredGCP, err := SQLInstanceKRMToGCP(a.desired, a.actual)
 	if err != nil {
 		return err
 	}
@@ -381,17 +381,14 @@ func (a *sqlInstanceAdapter) Update(ctx context.Context, updateOp *directbase.Up
 	}
 
 	// Finally, update rest of the fields
-	desiredGCP, err := SQLInstanceKRMToGCP(a.desired)
+	desiredGCP, err := SQLInstanceKRMToGCP(a.desired, a.actual)
 	if err != nil {
 		return err
 	}
 
 	if !InstancesMatch(desiredGCP, a.actual) {
 		updateOp.RecordUpdatingEvent()
 
-		// GCP API requires we set the current settings version, otherwise update will fail.
-		desiredGCP.Settings.SettingsVersion = a.actual.Settings.SettingsVersion
-
 		op, err := a.sqlInstancesClient.Update(a.projectID, desiredGCP.Name, desiredGCP).Context(ctx).Do()
 		if err != nil {
 			return fmt.Errorf("updating SQLInstance %s failed: %w", desiredGCP.Name, err)

pkg/controller/direct/sql/sqlinstance_defaults.go

@@ -20,29 +20,70 @@ import (
 	api "google.golang.org/api/sqladmin/v1beta4"
 )
 
-func ApplySQLInstanceGCPDefaults(in *krm.SQLInstance, out *api.DatabaseInstance) {
+func ApplySQLInstanceGCPDefaults(in *krm.SQLInstance, out *api.DatabaseInstance, actual *api.DatabaseInstance) {
 	if in.Spec.InstanceType == nil {
+		// GCP default InstanceType is CLOUD_SQL_INSTANCE.
 		out.InstanceType = "CLOUD_SQL_INSTANCE"
 	}
+	if in.Spec.MaintenanceVersion == nil && actual != nil {
+		// If desired maintenanceVersion is not specified, assume user wants the actual.
+		out.MaintenanceVersion = actual.MaintenanceVersion
+	}
 	if in.Spec.Settings.ActivationPolicy == nil {
+		// GCP default ActivationPolicy is ALWAYS.
 		out.Settings.ActivationPolicy = "ALWAYS"
 	}
+	if in.Spec.Settings.AuthorizedGaeApplications == nil {
+		// For some reason, GCP API uses empty slice instead of nil.
+		out.Settings.AuthorizedGaeApplications = make([]string, 0)
+	}
 	if in.Spec.Settings.AvailabilityType == nil {
+		// GCP default AvailailbilityType is ZONAL.
 		out.Settings.AvailabilityType = "ZONAL"
 	}
+	if in.Spec.Settings.BackupConfiguration == nil && actual != nil && !actual.Settings.BackupConfiguration.Enabled {
+		// If desired backupConfiguration is not specified and actual is disabled, use the actual.
+		out.Settings.BackupConfiguration = actual.Settings.BackupConfiguration
+	}
 	if in.Spec.Settings.ConnectorEnforcement == nil {
+		// GCP default ConnectorEnforcement is NOT_REQUIRED.
 		out.Settings.ConnectorEnforcement = "NOT_REQUIRED"
 	}
 	if in.Spec.Settings.DiskType == nil {
+		// GCP default DiskType is PD_SSD.
 		out.Settings.DataDiskType = "PD_SSD"
 	}
 	if in.Spec.Settings.Edition == nil {
+		// GCP default Edition is ENTERPRISE.
 		out.Settings.Edition = "ENTERPRISE"
 	}
+	if in.Spec.Settings.IpConfiguration == nil {
+		// GCP default IpConfiguration.
+		out.Settings.IpConfiguration = &api.IpConfiguration{
+			Ipv4Enabled:  true,
+			ServerCaMode: "GOOGLE_MANAGED_INTERNAL_CA",
+			SslMode:      "ALLOW_UNENCRYPTED_AND_ENCRYPTED",
+		}
+	}
 	if in.Spec.Settings.PricingPlan == nil {
+		// GCP default PricingPlan is PER_USE.
 		out.Settings.PricingPlan = "PER_USE"
 	}
+	if in.Spec.Settings.ReplicationType == nil {
+		// GCP default ReplicationType is SYNCHRONOUS.
+		out.Settings.ReplicationType = "SYNCHRONOUS"
+	}
 	if in.Spec.Settings.DiskAutoresize == nil {
+		// GCP default StorageAutoResize is true.
 		out.Settings.StorageAutoResize = direct.PtrTo(true)
 	}
+	if in.Spec.Settings.DiskSize == nil && actual != nil && *out.Settings.StorageAutoResize {
+		// If desired DiskSize is not specified and StorageAutoResize is enabled, use the actual disk size.
+		// Note: This must be set AFTER setting the default value for StorageAutoResize.
+		out.Settings.DataDiskSizeGb = actual.Settings.DataDiskSizeGb
+	}
+	if actual != nil {
+		// GCP API requires we set the current settings version, otherwise update will fail.
+		out.Settings.SettingsVersion = actual.Settings.SettingsVersion
+	}
 }

pkg/controller/direct/sql/sqlinstance_equality.go

@@ -183,11 +183,13 @@ func SettingsMatch(desired *api.Settings, actual *api.Settings) bool {
 	if desired.ReplicationType != actual.ReplicationType {
 		return false
 	}
-	// Ignore SettingsVersion. It is not part of the "desired state".
+	if desired.SettingsVersion != actual.SettingsVersion {
+		return false
+	}
 	if !SqlServerAuditConfigsMatch(desired.SqlServerAuditConfig, actual.SqlServerAuditConfig) {
 		return false
 	}
-	if desired.StorageAutoResize != actual.StorageAutoResize {
+	if !StorageAutoResizesMatch(desired.StorageAutoResize, actual.StorageAutoResize) {
 		return false
 	}
 	if desired.StorageAutoResizeLimit != actual.StorageAutoResizeLimit {
@@ -624,6 +626,19 @@ func SqlServerAuditConfigsMatch(desired *api.SqlServerAuditConfig, actual *api.S
 	return true
 }
 
+func StorageAutoResizesMatch(desired *bool, actual *bool) bool {
+	if desired == nil && actual == nil {
+		return true
+	}
+	if !PointersMatch(desired, actual) {
+		return false
+	}
+	if *desired != *actual {
+		return false
+	}
+	return true
+}
+
 func PointersMatch[T any](desired *T, actual *T) bool {
 	if (desired == nil && actual != nil) || (desired != nil && actual == nil) {
 		// Pointers are not matching if one is nil and the other is not nil.

pkg/controller/direct/sql/sqlinstance_mappings.go

@@ -26,7 +26,7 @@ import (
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/label"
 )
 
-func SQLInstanceKRMToGCP(in *krm.SQLInstance) (*api.DatabaseInstance, error) {
+func SQLInstanceKRMToGCP(in *krm.SQLInstance, actual *api.DatabaseInstance) (*api.DatabaseInstance, error) {
 	if in == nil {
 		return nil, fmt.Errorf("cannot convert nil KRM SQLInstance to GCP DatabaseInstance")
 	}
@@ -52,7 +52,7 @@ func SQLInstanceKRMToGCP(in *krm.SQLInstance) (*api.DatabaseInstance, error) {
 	}
 
 	// Here be dragons.
-	ApplySQLInstanceGCPDefaults(in, out)
+	ApplySQLInstanceGCPDefaults(in, out, actual)
 
 	return out, nil
 }

pkg/test/resourcefixture/testdata/basic/sql/v1beta1/sqlinstance/sqlinstance-activationpolicy-direct/_http.log

@@ -44,12 +44,18 @@ User-Agent: kcc/controller-manager
     "connectorEnforcement": "NOT_REQUIRED",
     "dataDiskType": "PD_SSD",
     "edition": "ENTERPRISE",
+    "ipConfiguration": {
+      "ipv4Enabled": true,
+      "serverCaMode": "GOOGLE_MANAGED_INTERNAL_CA",
+      "sslMode": "ALLOW_UNENCRYPTED_AND_ENCRYPTED"
+    },
     "kind": "sql#settings",
     "locationPreference": {
       "kind": "sql#locationPreference",
       "zone": "us-central1-a"
     },
     "pricingPlan": "PER_USE",
+    "replicationType": "SYNCHRONOUS",
     "storageAutoResize": true,
     "tier": "db-custom-1-3840",
     "userLabels": {
@@ -378,20 +384,38 @@ User-Agent: kcc/controller-manager
   "databaseVersion": "POSTGRES_15",
   "instanceType": "CLOUD_SQL_INSTANCE",
   "kind": "sql#instance",
+  "maintenanceVersion": "POSTGRES_15_7.R20240514.00_12",
   "name": "sqlinstance-activationpolicy-direct-${uniqueId}",
   "region": "us-central1",
   "settings": {
     "activationPolicy": "NEVER",
     "availabilityType": "ZONAL",
+    "backupConfiguration": {
+      "backupRetentionSettings": {
+        "retainedBackups": 7,
+        "retentionUnit": "COUNT"
+      },
+      "kind": "sql#backupConfiguration",
+      "startTime": "12:00",
+      "transactionLogRetentionDays": 7,
+      "transactionalLogStorageState": "TRANSACTIONAL_LOG_STORAGE_STATE_UNSPECIFIED"
+    },
     "connectorEnforcement": "NOT_REQUIRED",
+    "dataDiskSizeGb": "10",
     "dataDiskType": "PD_SSD",
     "edition": "ENTERPRISE",
+    "ipConfiguration": {
+      "ipv4Enabled": true,
+      "serverCaMode": "GOOGLE_MANAGED_INTERNAL_CA",
+      "sslMode": "ALLOW_UNENCRYPTED_AND_ENCRYPTED"
+    },
     "kind": "sql#settings",
     "locationPreference": {
       "kind": "sql#locationPreference",
       "zone": "us-central1-a"
     },
     "pricingPlan": "PER_USE",
+    "replicationType": "SYNCHRONOUS",
     "settingsVersion": "123",
     "storageAutoResize": true,
     "tier": "db-custom-1-3840",
@@ -527,6 +551,7 @@ X-Xss-Protection: 0
       },
       "enabled": false,
       "kind": "sql#backupConfiguration",
+      "replicationLogArchivingEnabled": false,
       "startTime": "12:00",
       "transactionLogRetentionDays": 7,
       "transactionalLogStorageState": "TRANSACTIONAL_LOG_STORAGE_STATE_UNSPECIFIED"

pkg/test/resourcefixture/testdata/basic/sql/v1beta1/sqlinstance/sqlinstance-auditconfig-direct/_http.log

@@ -184,12 +184,18 @@ User-Agent: kcc/controller-manager
     "connectorEnforcement": "NOT_REQUIRED",
     "dataDiskType": "PD_SSD",
     "edition": "ENTERPRISE",
+    "ipConfiguration": {
+      "ipv4Enabled": true,
+      "serverCaMode": "GOOGLE_MANAGED_INTERNAL_CA",
+      "sslMode": "ALLOW_UNENCRYPTED_AND_ENCRYPTED"
+    },
     "kind": "sql#settings",
     "locationPreference": {
       "kind": "sql#locationPreference",
       "zone": "us-central1-a"
     },
     "pricingPlan": "PER_USE",
+    "replicationType": "SYNCHRONOUS",
     "sqlServerAuditConfig": {
       "bucket": "gs://storagebucket-${uniqueId}",
       "kind": "sql#sqlServerAuditConfig",
@@ -561,21 +567,39 @@ User-Agent: kcc/controller-manager
   "databaseVersion": "SQLSERVER_2022_EXPRESS",
   "instanceType": "CLOUD_SQL_INSTANCE",
   "kind": "sql#instance",
+  "maintenanceVersion": "SQLSERVER_2022_EXPRESS_CU12_GDR.R20240501.00_05",
   "name": "sqlinstance-auditconfig-direct-${uniqueId}",
   "region": "us-central1",
   "rootPassword": "1234!@#$asdf",
   "settings": {
     "activationPolicy": "ALWAYS",
     "availabilityType": "ZONAL",
+    "backupConfiguration": {
+      "backupRetentionSettings": {
+        "retainedBackups": 7,
+        "retentionUnit": "COUNT"
+      },
+      "kind": "sql#backupConfiguration",
+      "startTime": "12:00",
+      "transactionLogRetentionDays": 7,
+      "transactionalLogStorageState": "TRANSACTIONAL_LOG_STORAGE_STATE_UNSPECIFIED"
+    },
     "connectorEnforcement": "NOT_REQUIRED",
+    "dataDiskSizeGb": "10",
     "dataDiskType": "PD_SSD",
     "edition": "ENTERPRISE",
+    "ipConfiguration": {
+      "ipv4Enabled": true,
+      "serverCaMode": "GOOGLE_MANAGED_INTERNAL_CA",
+      "sslMode": "ALLOW_UNENCRYPTED_AND_ENCRYPTED"
+    },
     "kind": "sql#settings",
     "locationPreference": {
       "kind": "sql#locationPreference",
       "zone": "us-central1-a"
     },
     "pricingPlan": "PER_USE",
+    "replicationType": "SYNCHRONOUS",
     "settingsVersion": "123",
     "sqlServerAuditConfig": {
       "bucket": "gs://storagebucket-${uniqueId}",

pkg/test/resourcefixture/testdata/basic/sql/v1beta1/sqlinstance/sqlinstance-authorizednetworks-direct/_http.log

@@ -60,6 +60,7 @@ User-Agent: kcc/controller-manager
       "zone": "us-central1-a"
     },
     "pricingPlan": "PER_USE",
+    "replicationType": "SYNCHRONOUS",
     "storageAutoResize": true,
     "tier": "db-custom-1-3840",
     "userLabels": {
@@ -374,12 +375,24 @@ User-Agent: kcc/controller-manager
   "databaseVersion": "POSTGRES_16",
   "instanceType": "CLOUD_SQL_INSTANCE",
   "kind": "sql#instance",
+  "maintenanceVersion": "POSTGRES_16_3.R20240527.01_10",
   "name": "sqlinstance-authorizednetworks-direct-${uniqueId}",
   "region": "us-central1",
   "settings": {
     "activationPolicy": "ALWAYS",
     "availabilityType": "ZONAL",
+    "backupConfiguration": {
+      "backupRetentionSettings": {
+        "retainedBackups": 7,
+        "retentionUnit": "COUNT"
+      },
+      "kind": "sql#backupConfiguration",
+      "startTime": "12:00",
+      "transactionLogRetentionDays": 7,
+      "transactionalLogStorageState": "TRANSACTIONAL_LOG_STORAGE_STATE_UNSPECIFIED"
+    },
     "connectorEnforcement": "NOT_REQUIRED",
+    "dataDiskSizeGb": "10",
     "dataDiskType": "PD_SSD",
     "edition": "ENTERPRISE",
     "ipConfiguration": {
@@ -404,6 +417,7 @@ User-Agent: kcc/controller-manager
       "zone": "us-central1-a"
     },
     "pricingPlan": "PER_USE",
+    "replicationType": "SYNCHRONOUS",
     "settingsVersion": "123",
     "storageAutoResize": true,
     "tier": "db-custom-1-3840",
@@ -539,6 +553,7 @@ X-Xss-Protection: 0
       },
       "enabled": false,
       "kind": "sql#backupConfiguration",
+      "replicationLogArchivingEnabled": false,
       "startTime": "12:00",
       "transactionLogRetentionDays": 7,
       "transactionalLogStorageState": "TRANSACTIONAL_LOG_STORAGE_STATE_UNSPECIFIED"

pkg/test/resourcefixture/testdata/basic/sql/v1beta1/sqlinstance/sqlinstance-backupconfiguration-binarylog-direct/_http.log

@@ -57,12 +57,18 @@ User-Agent: kcc/controller-manager
     "connectorEnforcement": "NOT_REQUIRED",
     "dataDiskType": "PD_SSD",
     "edition": "ENTERPRISE",
+    "ipConfiguration": {
+      "ipv4Enabled": true,
+      "serverCaMode": "GOOGLE_MANAGED_INTERNAL_CA",
+      "sslMode": "ALLOW_UNENCRYPTED_AND_ENCRYPTED"
+    },
     "kind": "sql#settings",
     "locationPreference": {
       "kind": "sql#locationPreference",
       "zone": "us-central1-a"
     },
     "pricingPlan": "PER_USE",
+    "replicationType": "SYNCHRONOUS",
     "storageAutoResize": true,
     "tier": "db-custom-1-3840",
     "userLabels": {
@@ -573,6 +579,7 @@ User-Agent: kcc/controller-manager
   "databaseVersion": "MYSQL_5_7",
   "instanceType": "CLOUD_SQL_INSTANCE",
   "kind": "sql#instance",
+  "maintenanceVersion": "MYSQL_5_7_44.R20231105.01_03",
   "name": "sqlinstance-backupconfiguration-binarylog-direct-${uniqueId}",
   "region": "us-central1",
   "settings": {
@@ -592,14 +599,21 @@ User-Agent: kcc/controller-manager
       "transactionLogRetentionDays": 3
     },
     "connectorEnforcement": "NOT_REQUIRED",
+    "dataDiskSizeGb": "10",
     "dataDiskType": "PD_SSD",
     "edition": "ENTERPRISE",
+    "ipConfiguration": {
+      "ipv4Enabled": true,
+      "serverCaMode": "GOOGLE_MANAGED_INTERNAL_CA",
+      "sslMode": "ALLOW_UNENCRYPTED_AND_ENCRYPTED"
+    },
     "kind": "sql#settings",
     "locationPreference": {
       "kind": "sql#locationPreference",
       "zone": "us-central1-a"
     },
     "pricingPlan": "PER_USE",
+    "replicationType": "SYNCHRONOUS",
     "settingsVersion": "123",
     "storageAutoResize": true,
     "tier": "db-custom-1-3840",