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

support cert-manager for generating tls and ca #555

Merged
merged 2 commits into from
May 23, 2022

Conversation

MaxFedotov
Copy link
Collaborator

closes #554

Added new flag to Capsule enable-secret-controller, which enables or disables secretcontroller reconcilers.
added 2 new options to Capsule helm chart:

  • manager.options.enableSecretController - configures enable-secret-controller flag
  • certManager.generateCertificates - enables cert-manager certificates yaml and other stuff

@MaxFedotov MaxFedotov requested a review from prometherion May 6, 2022 10:24
@netlify
Copy link

netlify bot commented May 6, 2022

Deploy Preview for capsule-documentation canceled.

Name Link
🔨 Latest commit 92ba2b4
🔍 Latest deploy log https://app.netlify.com/sites/capsule-documentation/deploys/6274f74a09054e0009eb7f29


var metricsAddr, namespace, configurationName string

var goFlagSet goflag.FlagSet

flag.StringVar(&metricsAddr, "metrics-addr", ":8080", "The address the metric endpoint binds to.")
flag.BoolVar(&enableSecretController, "enable-secret-controller", true,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this flag?

If we're missing the Secret since non-existing, the Pod will not be started until those are available for the mount.

Copy link
Collaborator Author

@MaxFedotov MaxFedotov May 6, 2022

Choose a reason for hiding this comment

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

we need to disable build-in reconciler to be able to generate secret using cert-manager. By default it is set to true, so nothing will change

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'm getting the point, I was more pointing to another issue.

If I install capsule with helm upgrade --install --set certManager.generateCertificates=true, I'll end up with Deployment, ServiceAccount, Service, Webhooks, and all the required resources except the Secret, with the additional ones managed by cert-manager, such as Certificate, Issue and other custom resources.

Since we're mounting the Capsule TLS certificate in the Deployment, if this is missing, the Pod won't start due to missing Volume.

With that said, I'm wondering if we truly need that flag, since Kubernetes already waits for the Secret to be created by cert-manager.

Copy link
Collaborator Author

@MaxFedotov MaxFedotov May 6, 2022

Choose a reason for hiding this comment

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

if you want to use cert-manager to generate tls\ca secrets, you will need to install chart using helm upgrade --instal --set manager.options.enableSecretController=false --set certManager.generateCertificates=true. This will disable build-in secretController and enable cert-manager certificate. Secret with tls\ca will be generated before the start of the Capsule pod and will be mounted inside the deployment as before, webhooks will be patched with generated caData. I've verified this locally and everything is working fine :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't have any doubts about it, pretty sure it works. I'm just wondering why we should disable the secret reconciler since the CA and TLS would be created by cert-manager.

Capsule would just check if the Secret CA and TLS are there, and read their value. Pretty sure I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

@MaxFedotov can't we use the cert-manager's CA Injector?

Copy link
Collaborator Author

@MaxFedotov MaxFedotov May 6, 2022

Choose a reason for hiding this comment

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

We can, but we need to template the name of the certificate in cert-manager annotation:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ include "capsule.fullname" . }}-webhook-cert

And templates and variables can't be used in crds :(

Copy link
Member

Choose a reason for hiding this comment

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

@MaxFedotov WDYT on moving CRD definition from crds folder to templates? Do you see any drawbacks to this?

We're already minor bumping upon any new CRD update, since we're informing users I don't see any issue on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only problem will be that resources in crds folder are not deleted with helm uninstall. If we move them to templates folder, they will be removed (and with the removing of crds all tenants and namespaces will be deleted as well). And one more thing to check is the ordering when applying resources. We have a capsuleConfiguration crd and capsuleConfiguration resource with default configuration, so we need to ensure that crd will be applied first

Copy link
Collaborator Author

@MaxFedotov MaxFedotov May 10, 2022

Choose a reason for hiding this comment

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

@prometherion let's do the same way as in cert-manager helm chart - put all crds in templates/crds.yaml

@prometherion
Copy link
Member

Besides the ongoing discussion, I think we should change the logic here:

https://github.com/clastix/capsule/blob/9f6883d309368a9dc0dda2c79a436837dc924c55/controllers/secret/ca.go#L178-L182

Actually, the code is not aware we have to ignore the secret since managed externally, maybe I could address this in a different issue: WDYT @MaxFedotov ?

@MaxFedotov
Copy link
Collaborator Author

Besides the ongoing discussion, I think we should change the logic here:

https://github.com/clastix/capsule/blob/9f6883d309368a9dc0dda2c79a436837dc924c55/controllers/secret/ca.go#L178-L182

Actually, the code is not aware we have to ignore the secret since managed externally, maybe I could address this in a different issue: WDYT @MaxFedotov ?

Completely ok :) you make a separate PR or add a commit to this?

@prometherion
Copy link
Member

Completely ok :) you make a separate PR or add a commit to this?

@MaxFedotov we can fix it later, let's get this merged.

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

Successfully merging this pull request may close these issues.

Use cert-manager to issue certificate for the capsule webhooks
2 participants