Remove enabled field from cloudSqlProxy (#517)

* Remove enabled field from cloudSqlProxy * fix attempting to patch immutable fields on jobs * increase timeouts, parallel * revert status check for skipjob * make ingress test not concurrent
kartverket · Aug 21, 2024 · e2b7cff · e2b7cff
1 parent 6972d7a
commit e2b7cff
Show file tree

Hide file tree

Showing 10 changed files with 62 additions and 43 deletions.
diff --git a/api/v1alpha1/podtypes/gcp.go b/api/v1alpha1/podtypes/gcp.go
@@ -34,12 +34,6 @@ type Auth struct {
 }
 
 type CloudSQLProxySettings struct {
-	// Enables the sidecar for CloudSQL proxy
-
-	//+kubebuilder:validation:Optional
-	//+kubebuilder:default:=false
-	Enabled bool `json:"enabled"`
-
 	// Connection name for the CloudSQL instance. Found in the Google Cloud Console under your CloudSQL resource.
 	// The format is "projectName:region:instanceName" E.g. "skip-prod-bda1:europe-north1:my-db".
 	//+kubebuilder:validation:Required

diff --git a/api/v1alpha1/skipjob_types.go b/api/v1alpha1/skipjob_types.go
@@ -36,10 +36,6 @@ type SKIPJobStatus struct {
 // +kubebuilder:object:generate=true
 // +kubebuilder:printcolumn:name="Status",type=string,JSONPath=`.status.summary.status`
 //
-// A SKIPJob is either defined as a one-off or a scheduled job. If the Cron field is set for SKIPJob, it may not be removed. If the Cron field is unset, it may not be added.
-// The Container field of a SKIPJob is only mutable if the Cron field is set. If unset, you must delete your SKIPJob to change container settings.
-// +kubebuilder:validation:XValidation:rule="(has(oldSelf.spec.cron) && has(self.spec.cron)) || (!has(oldSelf.spec.cron) && !has(self.spec.cron))", message="After creation of a SKIPJob you may not remove the Cron field if it was previously present, or add it if it was previously omitted. Please delete the SKIPJob to change its nature from a one-off/scheduled job."
-// +kubebuilder:validation:XValidation:rule="(size(self.status.subresources) == 0|| ((!has(self.spec.cron) && (oldSelf.spec.container == self.spec.container)) || has(self.spec.cron)))", message="The field Container is immutable for one-off jobs. Please delete your SKIPJob to change the containers settings."
 // SKIPJob is the Schema for the skipjobs API
 type SKIPJob struct {
 	metav1.TypeMeta   `json:",inline"`
@@ -64,6 +60,10 @@ type SKIPJobList struct {
 // SKIPJobSpec defines the desired state of SKIPJob
 //
 // +kubebuilder:object:generate=true
+// A SKIPJob is either defined as a one-off or a scheduled job. If the Cron field is set for SKIPJob, it may not be removed. If the Cron field is unset, it may not be added.
+// The Container field of a SKIPJob is only mutable if the Cron field is set. If unset, you must delete your SKIPJob to change container settings.
+// +kubebuilder:validation:XValidation:rule="(has(oldSelf.cron) && has(self.cron)) || (!has(oldSelf.cron) && !has(self.cron))", message="After creation of a SKIPJob you may not remove the Cron field if it was previously present, or add it if it was previously omitted. Please delete the SKIPJob to change its nature from a one-off/scheduled job."
+// +kubebuilder:validation:XValidation:rule="((!has(self.cron) && (oldSelf.container == self.container)) || has(self.cron))", message="The field Container is immutable for one-off jobs. Please delete your SKIPJob to change the containers settings."
 type SKIPJobSpec struct {
 	// Settings for the actual Job. If you use a scheduled job, the settings in here will also specify the template of the job.
 	//

diff --git a/config/crd/skiperator.kartverket.no_applications.yaml b/config/crd/skiperator.kartverket.no_applications.yaml
@@ -520,9 +520,6 @@ spec:
                           The format is "projectName:region:instanceName" E.g. "skip-prod-bda1:europe-north1:my-db".
                         pattern: ^[^:]+:[^:]+:[^:]+$
                         type: string
-                      enabled:
-                        default: false
-                        type: boolean
                       ip:
                         description: The IP address of the CloudSQL instance. This
                           is used to create a serviceentry for the CloudSQL proxy.

diff --git a/config/crd/skiperator.kartverket.no_skipjobs.yaml b/config/crd/skiperator.kartverket.no_skipjobs.yaml
@@ -21,10 +21,7 @@ spec:
     name: v1alpha1
     schema:
       openAPIV3Schema:
-        description: |-
-          A SKIPJob is either defined as a one-off or a scheduled job. If the Cron field is set for SKIPJob, it may not be removed. If the Cron field is unset, it may not be added.
-          The Container field of a SKIPJob is only mutable if the Cron field is set. If unset, you must delete your SKIPJob to change container settings.
-          SKIPJob is the Schema for the skipjobs API
+        description: SKIPJob is the Schema for the skipjobs API
         properties:
           apiVersion:
             description: |-
@@ -44,7 +41,11 @@ spec:
           metadata:
             type: object
           spec:
-            description: SKIPJobSpec defines the desired state of SKIPJob
+            description: |-
+              SKIPJobSpec defines the desired state of SKIPJob
+
+              A SKIPJob is either defined as a one-off or a scheduled job. If the Cron field is set for SKIPJob, it may not be removed. If the Cron field is unset, it may not be added.
+              The Container field of a SKIPJob is only mutable if the Cron field is set. If unset, you must delete your SKIPJob to change container settings.
             properties:
               container:
                 description: |-
@@ -484,9 +485,6 @@ spec:
                               The format is "projectName:region:instanceName" E.g. "skip-prod-bda1:europe-north1:my-db".
                             pattern: ^[^:]+:[^:]+:[^:]+$
                             type: string
-                          enabled:
-                            default: false
-                            type: boolean
                           ip:
                             description: The IP address of the CloudSQL instance.
                               This is used to create a serviceentry for the CloudSQL
@@ -838,6 +836,17 @@ spec:
             required:
             - container
             type: object
+            x-kubernetes-validations:
+            - message: After creation of a SKIPJob you may not remove the Cron field
+                if it was previously present, or add it if it was previously omitted.
+                Please delete the SKIPJob to change its nature from a one-off/scheduled
+                job.
+              rule: (has(oldSelf.cron) && has(self.cron)) || (!has(oldSelf.cron) &&
+                !has(self.cron))
+            - message: The field Container is immutable for one-off jobs. Please delete
+                your SKIPJob to change the containers settings.
+              rule: ((!has(self.cron) && (oldSelf.container == self.container)) ||
+                has(self.cron))
           status:
             description: |-
               SkiperatorStatus
@@ -944,16 +953,6 @@ spec:
         required:
         - spec
         type: object
-        x-kubernetes-validations:
-        - message: After creation of a SKIPJob you may not remove the Cron field if
-            it was previously present, or add it if it was previously omitted. Please
-            delete the SKIPJob to change its nature from a one-off/scheduled job.
-          rule: (has(oldSelf.spec.cron) && has(self.spec.cron)) || (!has(oldSelf.spec.cron)
-            && !has(self.spec.cron))
-        - message: The field Container is immutable for one-off jobs. Please delete
-            your SKIPJob to change the containers settings.
-          rule: (size(self.status.subresources) == 0|| ((!has(self.spec.cron) && (oldSelf.spec.container
-            == self.spec.container)) || has(self.spec.cron)))
     served: true
     storage: true
     subresources:

diff --git a/pkg/resourceprocessor/resource.go b/pkg/resourceprocessor/resource.go
@@ -45,8 +45,10 @@ func preparePatch(new client.Object, old client.Object) {
 	case *batchv1.Job:
 		job := old.(*batchv1.Job)
 		definition := new.(*batchv1.Job)
-		maps.Copy(definition.Spec.Template.Labels, job.Spec.Template.Labels) //kubernetes adds labels on creation
-		definition.Spec.Selector = job.Spec.Selector                         //is set on creation
+		maps.Copy(definition.Spec.Template.Labels, job.Spec.Template.Labels) // kubernetes adds labels on creation
+		definition.Spec.Selector = job.Spec.Selector                         // immutable
+		definition.Spec.Template = job.Spec.Template                         // immutable
+		definition.Spec.Completions = job.Spec.Completions                   // immutable
 	}
 }
 

diff --git a/pkg/util/helperfunctions.go b/pkg/util/helperfunctions.go
@@ -149,7 +149,7 @@ func EnsurePrefix(s string, prefix string) string {
 }
 
 func IsCloudSqlProxyEnabled(gcp *podtypes.GCP) bool {
-	return gcp != nil && gcp.CloudSQLProxy != nil && gcp.CloudSQLProxy.Enabled
+	return gcp != nil && gcp.CloudSQLProxy != nil
 }
 
 func IsGCPAuthEnabled(gcp *podtypes.GCP) bool {

diff --git a/tests/application/cloudsql-auth-proxy/set-version.yaml b/tests/application/cloudsql-auth-proxy/set-version.yaml
@@ -7,7 +7,6 @@ spec:
   port: 8080
   gcp:
     cloudSqlProxy:
-      enabled: true
       connectionName: test-project-bda1:europe-north1:pg-01-test
       version: 2.8.12
       serviceAccount: [email protected]

diff --git a/tests/application/ingress/chainsaw-test.yaml b/tests/application/ingress/chainsaw-test.yaml
@@ -4,7 +4,7 @@ metadata:
   name: ingress
 spec:
   skip: false
-  concurrent: true
+  concurrent: false
   skipDelete: false
   namespace: ingress
   steps:

diff --git a/tests/config.yaml b/tests/config.yaml
@@ -3,11 +3,11 @@ kind: Configuration
 metadata:
   name: configuration
 spec:
-  parallel: 5
+  parallel: 40
   timeouts:
-    delete: 60s
-    assert: 60s
-    apply: 60s
-    cleanup: 60s
-    error: 60s
-    exec: 60s
+    delete: 180s
+    assert: 180s
+    apply: 180s
+    cleanup: 180s
+    error: 180s
+    exec: 180s
diff --git a/tests/skipjob/immutable-container/skipjob-assert.yaml b/tests/skipjob/immutable-container/skipjob-assert.yaml
@@ -13,3 +13,31 @@ spec:
 status:
   summary:
     status: Synced
+  subresources:
+    Job[minimal-job]:
+      message: Job has finished synchronizing
+      status: Synced
+    ServiceAccount[minimal-job-skipjob]:
+      message: ServiceAccount has finished synchronizing
+      status: Synced
+  conditions:
+    - message: Job failed previous run
+      observedGeneration: 2
+      reason: JobFailed
+      status: 'False'
+      type: Failed
+    - message: Job has been created and is now running
+      observedGeneration: 2
+      reason: JobRunning
+      status: 'False'
+      type: Running
+    - message: Job has finished
+      observedGeneration: 2
+      reason: JobFinished
+      status: 'True'
+      type: Finished
+    - message: Internal rules are valid
+      observedGeneration: 2
+      reason: ApplicationReconciled
+      status: 'True'
+      type: InternalRulesValid