feat: support generic sidecars and passwordless DB access #629
Conversation
|
Thanks for the work @ed-marks ! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| service: postgres | ||
| spec: | ||
| containers: | ||
| - name: postgres |
Check warning
Code scanning / SonarCloud
CPU limits should be enforced Medium
| service: postgres | ||
| spec: | ||
| containers: | ||
| - name: postgres |
Check warning
Code scanning / SonarCloud
Memory limits should be enforced Medium
| metadata: | ||
| labels: | ||
| service: postgres | ||
| spec: |
Check warning
Code scanning / SonarCloud
Service account tokens should not be mounted in pods Medium
| service: postgres | ||
| spec: | ||
| containers: | ||
| - name: postgres |
Check warning
Code scanning / SonarCloud
Storage limits should be enforced Medium
controllers/reconcile_persistence.go
Outdated
| @@ -117,6 +118,13 @@ func (r *TemporalClusterReconciler) reconcilePersistence(ctx context.Context, cl | |||
| return 0, fmt.Errorf("can't reconcile schema script configmap: %w", err) | |||
| } | |||
|
|
|||
| // Ensure the serviceaccount used by jobs is up-to-date | |||
| serviceAccountBuilder := base.NewServiceAccountBuilder(persistence.ServiceNameSuffix, cluster, r.Scheme, &v1beta1.ServiceSpec{}) | |||
There was a problem hiding this comment.
This feels a little hacky, passing in the null ServiceSpec - I'll take a look at making the SA builder a little more generic
There was a problem hiding this comment.
The SA builder wasn't actually using the ServiceSpec at all so I've just removed it
| } | ||
| annotations["eks.amazonaws.com/role-arn"] = *b.instance.Spec.Archival.Provider.S3.RoleName | ||
| } | ||
| if b.instance.Spec.Persistence.DefaultStore.SQL != nil && |
There was a problem hiding this comment.
this also feels like it could be neater
|
thanks @alexandrevilain - I think i've covered most bases here. Tricky to add e2e tests to as i couldn't find a kind_with_registry version later than 1.27 though I'll run some more manual tests next week against some of our dev clusters, but as of cd889 everything functioned with sidecars and GCP CloudSQL as it did locally |
| type: boolean | ||
| perUnitHistogramBoundaries: | ||
| additionalProperties: | ||
| jobInitContainers: |
There was a problem hiding this comment.
Could you please add entries here: https://github.com/alexandrevilain/temporal-operator/blob/main/Makefile#L49 ?
They are used to remove pod specs in the CRDs and set the x-kubernetes-preserve-unknown-fields = true field.
This makes the operator's CRD much tinner.
| @@ -987,6 +994,9 @@ type TemporalClusterSpec struct { | |||
| // JobResources allows set resources for setup/update jobs. | |||
| // +optional | |||
| JobResources corev1.ResourceRequirements `json:"jobResources,omitempty"` | |||
| // JobInitContainers adds a list of init containers to the setup's jobs. | |||
| // +optional | |||
| JobInitContainers []corev1.Container `json:"jobInitContainers,omitempty"` | |||
There was a problem hiding this comment.
Note for myself: when releasing v1 APIversion, jobs will need their own key.
Co-authored-by: Alexandre Vilain <[email protected]>
There was a problem hiding this comment.
LGTM @ed-marks ! Thanks for the work!
Lint and e2e tests are failing, feel free to ping me if you need some help!
|
perfect thanks for the approve @alexandrevilain - that last commit has fixed all the e2e tests except the cass persistence which afaik isn't affected by the changes in this PR but for some reason i can't seem to make happy. Should have a min to take a proper look on Friday |
|
Ok @alexandrevilain the e2es are all reliably passing locally now so i suspect we should be good to retry those e2e tests again. The problem with them locally was the namespaces being left over was eating up resources on my laptop and by the time we got to the last one - usually cassandra - the db pod would just get OOMKilled all day. I hope this is whats also happening in that 1.27 test but naturally I can't see into whats happening on the kind cluster to be sure. I'm not sure if there was a reason the NS deletion stuff was removed before, i couldn't find any context around it - if there is I can delete it again but you might need to run the pipeline on a bigger machine |
|
|
Thanks @ed-marks ! |
Primarily to enable GCP's CloudSQLProxy to run as a sidecar
✅ Tested basic sidecar functionality
✅ Tested omitting secretRefs
✅ Tested ServiceAccount annotations
✅ Tested CloudSQLProxy Explicitly
✅ Tested passwordless DB access
Addresses #600 - however only for K8s 1.28+ with the SidecarContainers Feature Gate enabled.
Since this primarily affects GKE, that means a 1.29 cluster in reality which they've just released on the Regular release channel in most(all?) regions now