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

Proposal: Add sidecar to Druid deployments #293

Open
cintoSunny opened this issue Jun 21, 2022 · 7 comments
Open

Proposal: Add sidecar to Druid deployments #293

cintoSunny opened this issue Jun 21, 2022 · 7 comments

Comments

@cintoSunny
Copy link
Contributor

MOTIVATION

Currently, the Druid operator does not allow the addition of a sidecar container to Druid deployments. The only way is to inject sidecars by mutation webhooks like istio. But many orgs may not allow setting up istio or it could be an additional effort to add them.

PROPOSAL
To have the ability to add sidecars via druid operator based on a set of configs. The configs can look something like this:

sidecarContainer:
- configs:
    imageName: docker.com/ns/samplesimage:latest
    containerName: sidecar
    command: /bin/start.sh
    pullPolicy: Always
...

I have a working code for my org that I used for logging. And can push the change here, but wanted to check with the community first. I saw a couple of issues regarding this earlier, so thought it would be useful. cc: @AdheipSingh

@AdheipSingh
Copy link
Contributor

IMHO mutation webhook is the right way to inject, security reasons are something specific to orgs. Is there a druid specific reason to add a sidecar ?
Regardless i would be curios to see the code changes as this might require changes in the core handler and makeDeployment and makeStatefulset functions.

@AdheipSingh
Copy link
Contributor

cc @nishantmonu51

@cintoSunny
Copy link
Contributor Author

Thanks. I agree that mutation webhook may be a better solution, but may not be an option for everyone. I believe the operator should have easy config-driven settings to enable sidecars as well. I agree with @himanshug here:
#59 (comment)

I see a couple of issues open related to this as well. These were also my motivation to enable sidecars:
#59
#275

For us, we had to send logs to Splunk which needed a sidecar to be run.

I will send the PR across in a while.

@AdheipSingh
Copy link
Contributor

AdheipSingh commented Jun 28, 2022

Init containers make sense to pull in dependency , adding sidecars in the CR doesn't make sense to me.
Sidecars are something which are injected for external use cases such as logging, security. I would only add a sidecar config inside the CR if its a use case for druid specifically.

Mutation Webhooks is the standard practice to add sidecars.

You can add mutation webhook in the source code, ie is the standard practice as per kubebuilder and controller runtime docs.
Adding in the CR will add in anti-practice and confusion

@cintoSunny
Copy link
Contributor Author

I get that Mutation Webhooks is the standard practice, but thought it will be good to have an option for the sidecar in the config itself. Let me know if you still want to take a look at the PR. I can send it up within a few days.

@AdheipSingh
Copy link
Contributor

@cintoSunny feel free to send PR :)

@cintoSunny
Copy link
Contributor Author

@AdheipSingh : Here is the PR: #296

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants