Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add option to include CloudSQL proxy as a sidecar container (WIP) #232

Closed
wants to merge 6 commits into from

Conversation

Oscaarjs
Copy link

@Oscaarjs Oscaarjs commented Aug 1, 2024

Adds the option of including a sidecar cloud sql proxy container to the deployment, init- and setupjob. Suitable for deploying Zitadel in a cluster with a managed gcp sql db instead of a self-hosted db.

Related to;

Definition of Ready

  • I am happy with the code
  • Short description of the feature/issue is added in the pr description
  • PR is linked to the corresponding user story
  • Acceptance criteria are met
  • All open todos and follow ups are defined in a new ticket and justified
  • Deviations from the acceptance criteria and design are agreed with the PO and documented.
  • No debug or dead code
  • My code has no repetitions
  • Documentation/examples are up-to-date
  • All non-functional requirements are met
  • If possible, the test configuration is adjusted so acceptance tests cover my changes

@hifabienne hifabienne added the os-contribution This is a contribution from our open-source community label Aug 1, 2024
Copy link
Member

@eliobischof eliobischof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @Oscaarjs

I like the new example 8 describing how to use Google Cloud SQL.
However, I think the "cloudSqlProxy" template is a bit edge case specific.

By the way, Is it possible that you mixed up init containers and sidecars? The proxy should run alongside ZITADEL (containers array), not before ZITADEL or a ZITADEL job starts (initContainers array), right?

I think the only change the chart needs is to add an extraContainers value that is used in the deployment.yaml. Similar to the already existing initJob.extraContainers and setupJob.extraContainers. I'd love to add the example 8 using these generic extraContainers values.

If we implement it this way, any sidecar is configurable which I guess covers more use cases and we have to maintain less template config. Does that make sense?

@eliobischof
Copy link
Member

I will close the PR for now and reopen it once there is activity again.

@xXluki98Xx
Copy link

xXluki98Xx commented Dec 9, 2024

@eliobischof Hello,
I'm trying the same thing right now (Zitadel on GKE with managed postgres).

I'm not sure why Oscaarjs isn't responding, but the extraContainers and an example would be just what I need, to get Zitadel up and running.

@eliobischof eliobischof reopened this Dec 12, 2024
@PurseChicken
Copy link
Contributor

PurseChicken commented Dec 12, 2024

I deploy Zitadel in GKE with cloud-sql-proxy.

If you are wanting to use a sidecar approach, then in my opinion the chart should be modified to include a global ".extraContainers" list in which you can apply your own containers to each required pod. This likely will include the Zitadel Deployment, init job and setup job. I do not think this should be specific to cloud-sql-proxy as there might be other use cases for sidecar deployments that the chart does not support. By having it configurable, then you can deploy whatever you want as a sidecar using .extraContainers. I can create a PR for this if its truly desired.

Since I deploy using a subchart, I just use my own cloudsqlproxy manifest which gets deployed along with the rest of Zitadel via the Zitadel dependency.

Folder Structure:

zitadel
-->charts (is populated with the chart tgz after running helm dep update)
-->templates (this is where my custom manifest lives along with many others like external secrets, gateway-api etc)
chart.yaml
my.values.yaml (my own values that feed into my templates in the templates folder + zitadel values)

chart.yaml:

apiVersion: v2
name: zitadel
description: A Helm chart for ZITADEL v2
type: application
version: 0.0.1
dependencies:
  - name: zitadel
    version: 8.7.1
    repository: "https://charts.zitadel.com"

@eliobischof
Copy link
Member

I also think we should add an extraContrainers option for init, setup and deployment and an example with cloud sql proxy.

@PurseChicken
Copy link
Contributor

I also think we should add an extraContrainers option for init, setup and deployment and an example with cloud sql proxy.

Ok, I will work on a PR and link it here. Then this one can be closed again.

@Oscaarjs
Copy link
Author

Oscaarjs commented Dec 13, 2024

Sorry for no responses on my side here, not currently working with this at all. @eliobischof As for your question regarding sidecar containers/init containers, the seemingly recommended way in current kubernetes versions is to do it the way I did with an initContainer with the restart policy "always" (https://kubernetes.io/docs/concepts/workloads/pods/sidecar-containers/)

@PurseChicken
Copy link
Contributor

PurseChicken commented Dec 13, 2024

Don't use an Init container for Cloud SQL Proxy. An Init container is meant to run, do a job, and then exit and move on. Cloud SQL proxy will never exit. Its meant to run and stay running.

It either needs to be a sidecar container or a separate deployment.

@Oscaarjs
Copy link
Author

@PurseChicken please check the above url, an initContainer with "restart: Always” is a sidecar container

@PurseChicken
Copy link
Contributor

PurseChicken commented Dec 14, 2024

@Oscaarjs Thanks for forcing me to read that. It does appear that there is "sidecar-style init container" functionality to be able to keep an init container running and not prevent the regular containers from running.

That being said, its definitely a confusing subject depending on where you are gathering your information. In the traditional sense (and even in the documentation for init containers specifically at https://kubernetes.io/docs/concepts/workloads/pods/init-containers/) init containers are meant to run to completion during Pod initialization. (In Kubernetes, a sidecar container is a container that starts before the main application container and continues to run. This document is about init containers: containers that run to completion during Pod initialization..)

As I write the new PR to add extraContainers (supporting cloud-sql-proxy use), it will be in the containers section and NOT an init container. This can always be adjusted in the future as "sidecar-style init containers" become more well known and not feature gated.

@eliobischof
Copy link
Member

@Oscaarjs thanks for clarifying, I wasn't aware of that feature.

Shouldn't we just add the option to configure both initContainers and extraContainers?

@PurseChicken
Copy link
Contributor

@eliobischof Certainly doable, but order of init containers can be important. When I start on the PR I will look to see what currently exists and make some decisions from there. I did see an issue about switching from hooks to init containers so I think we should also be cognizant of that work as well.

#280

@eliobischof
Copy link
Member

@eliobischof Certainly doable, but order of init containers can be important. When I start on the PR I will look to see what currently exists and make some decisions from there. I did see an issue about switching from hooks to init containers so I think we should also be cognizant of that work as well.

#280

Sounds great, thank you @PurseChicken

@PurseChicken
Copy link
Contributor

FYI here is the new PR for extraContainers:

#287

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os-contribution This is a contribution from our open-source community waiting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants