tryretool / retool-helm

MIT License
47 stars 59 forks source link

Improve support for sidecar containers #58

Open jawnsy opened 2 years ago

jawnsy commented 2 years ago

Overview

Support configuring sidecars as YAML rather than a YAML string.

Current behavior

The extraManifests setting allows us to define a list of manifests to deploy, in list format:

https://github.com/tryretool/retool-helm/blob/1e06cee2744e41197b66eb501c467cddd253c783/values.yaml#L282-L290

For example, my extraManifests looks like this:

extraManifests:
  - apiVersion: networking.gke.io/v1beta1
    kind: FrontendConfig
    metadata:
      name: "frontend"
    spec:
      sslPolicy: "restricted"
  - apiVersion: cloud.google.com/v1
    kind: BackendConfig
    metadata:
      name: "backend"
  - apiVersion: networking.gke.io/v1
    kind: ManagedCertificate
    metadata:
      name: managed-cert
    spec:
      domains:
        - retool.contoso.com

Given that extraContainers is defined as a list [] by default, a reasonable assumption would be that we can add sidecar containers in the same way:

https://github.com/tryretool/retool-helm/blob/1e06cee2744e41197b66eb501c467cddd253c783/values.yaml#L191

However, the following configuration results in an error message:

extraContainers:
  - name: cloud-sql-proxy
    image: gcr.io/cloudsql-docker/gce-proxy:1.28.0
    command:
      - "/cloud_sql_proxy"
      - "-enable_iam_login"
      - "-log_debug_stdout"
      - "-instances=contoso-retool:us-east1:retool=tcp:5432"
    securityContext:
      runAsNonRoot: true
    resources:
      requests:
        memory: "256Mi"
        cpu: "250m"
      limits:
        memory: "1Gi"
        cpu: "1000m"

This results in an error:

$ helm upgrade retool retool/retool --install --namespace=retool --values=values.yaml
Error: UPGRADE FAILED: template: retool/templates/deployment_backend.yaml:195:7: executing "retool/templates/deployment_backend.yaml" at <.>: wrong type for value; expected string; got []interface {}

We can fix this by changing the extraContainers to be a string, by adding a |, like this:

extraContainers: |
  - name: cloud-sql-proxy
    image: gcr.io/cloudsql-docker/gce-proxy:1.28.0
    command:
      - "/cloud_sql_proxy"
      - "-enable_iam_login"
      - "-log_debug_stdout"
      - "-instances=prefect-retool:us-east1:retool-us-east1=tcp:5432"
    securityContext:
      runAsNonRoot: true
    resources:
      requests:
        memory: "256Mi"
        cpu: "250m"
      limits:
        memory: "1Gi"
        cpu: "1000m"

Unfortunately, this causes us to lose syntax highlighting support from IDEs, since it is interpreted as a simple string.

Desired behavior

Ideally, we should be able to specify containers using YAML rather than as a string. Compare this to how Bitnami charts allow users to configure sidecars: https://docs.bitnami.com/kubernetes/apps/wordpress/configuration/configure-sidecar-init-containers/

In the template, it is implemented with code like this: https://github.com/bitnami/charts/blob/0c32e83bd5ac05cd9f2f17e60b0e3f3aaa6d0c8f/bitnami/postgresql/templates/primary/statefulset.yaml#L561-L563

Notes

A benefit of the existing approach is that the extra containers can refer to other template values, which we would lose if we made it behave similarly to extraManifests.

jawnsy commented 2 years ago

Related: https://github.com/tryretool/retool-helm/pull/40

djMax commented 11 months ago

Also a benefit that value overrides in environments would work instead of having to cut/paste/modify