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

Explain why and how to isolate the cert-manager workloads #1331

Merged
merged 7 commits into from
Oct 25, 2023

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Oct 20, 2023

Preview: https://deploy-preview-1331--cert-manager-website.netlify.app/docs/installation/best-practice/#isolate-cert-manager-on-dedicated-node-pools

Followup to #1330
Fixes: cert-manager/cert-manager#5211

In this PR I want to give an example of how to use the affinity and toleration Helm values
and I propose running the cert-manager Pods on dedicated "platform" nodes,
for security reasons, but there may be other good use cases.

@erikgb Please take a look

@jetstack-bot jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Oct 20, 2023
@wallrj wallrj requested a review from hawksight October 20, 2023 16:27
@jetstack-bot jetstack-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 20, 2023
@wallrj wallrj requested a review from erikgb October 20, 2023 16:27
@netlify
Copy link

netlify bot commented Oct 20, 2023

Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit 7b22cba
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/6538dd7f64c9fc00080f21e5
😎 Deploy Preview https://deploy-preview-1331--cert-manager-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj wallrj force-pushed the node-placement-recommendations branch from 1f0abd9 to faeaf0e Compare October 20, 2023 16:31
content/docs/installation/best-practice.md Outdated Show resolved Hide resolved
content/docs/installation/best-practice.md Outdated Show resolved Hide resolved
content/docs/installation/best-practice.md Outdated Show resolved Hide resolved
content/docs/installation/best-practice.md Outdated Show resolved Hide resolved
Copy link
Contributor

@erikgb erikgb 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 working on this, @wallrj!

We simply use https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#nodeselector to schedule cert-manager workloads to dedicated platform nodes. I think that should be included as the simplest way of achieving the desired goal. The examples you have put up are simpler to express with nodeselector, I think?

Signed-off-by: Richard Wall <richard.wall@venafi.com>
…ages

Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj
Copy link
Member Author

wallrj commented Oct 23, 2023

We simply use https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#nodeselector to schedule cert-manager workloads to dedicated platform nodes. I think that should be included as the simplest way of achieving the desired goal. The examples you have put up are simpler to express with nodeselector, I think?

Done. I agree that nodeSelector is much simpler and works the same as nodeAffinity so I've changed it.
I had to explain that there is a default OS nodeSelector which you must explicitly add to your values.

@jetstack-bot jetstack-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 23, 2023
@wallrj wallrj requested review from erikgb and jsoref October 23, 2023 11:58
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Richard Wall <wallrj@users.noreply.github.com>
Copy link
Contributor

@erikgb erikgb left a comment

Choose a reason for hiding this comment

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

I am not an expert in this area, but why do we need tolerations in addition to the nodeSelector? Looks like a duplication to me, and according to https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#nodeselector, just the nodeSelector should be sufficient. And this setup works well for us.

Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj
Copy link
Member Author

wallrj commented Oct 24, 2023

I am not an expert in this area, but why do we need tolerations in addition to the nodeSelector? Looks like a duplication to me, and according to https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#nodeselector, just the nodeSelector should be sufficient. And this setup works well for us.

I'm not an expert either, but I guess one reason is:

  • in a multi-tenant cluster,
  • non-platform Pods might not have nodeSelector or affinity.nodeAffinity settings,
  • so those could be scheduled to any Nodes, including those that you reserved for your platform's Pods.
  • You can prevent this by adding a taint to the platform Nodes.

How do you prevent this happening in your cluster?
Do you have an admission webhook that overrides nodeSelector of the Pods of your tenants?

I will make it clearer that there are various solutions to this problem and that this is only one suggestion.

@wallrj wallrj requested a review from erikgb October 24, 2023 08:51
@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 24, 2023
Copy link
Member

@hawksight hawksight left a comment

Choose a reason for hiding this comment

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

A really useful addition. Had a quick read and looks good to me and seems better to have this live now and correct anything later if needed.

@gintautassulskus
Copy link

Great contribution, @wallrj! The documentation sounds correct. A minor remark, I do not recognise cainjection and startupapicheck used in the example, but it's been a long time since I used this package.

Copy link
Contributor

@erikgb erikgb left a comment

Choose a reason for hiding this comment

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

/lgtm

Very nice addition to the docs, @wallrj

@jetstack-bot
Copy link
Contributor

@erikgb: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

Very nice addition to the docs, @wallrj

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@erikgb
Copy link
Contributor

erikgb commented Oct 25, 2023

How do you prevent this happening in your cluster? Do you have an admission webhook that overrides nodeSelector of the Pods of your tenants?

No idea, but I can check. 😉 We run on Openshift, and it's usually pretty "secure by default".

@erikgb
Copy link
Contributor

erikgb commented Oct 25, 2023

How do you prevent this happening in your cluster? Do you have an admission webhook that overrides nodeSelector of the Pods of your tenants?

No idea, but I can check. 😉 We run on Openshift, and it's usually pretty "secure by default".

I think this is handled by some Openshift "magic" described here. When a normal user, without write access to namespace resources, schedules a workload, the pod will always get a "worker" label added to the pod nodeSelector. Since none of our nodes matches worker + something else, it means that all end-user workloads will be scheduled on worker nodes. Or not scheduled at all - if a user tries to set nodeSelector. Our platform team can override this default behavior by annotating system namespaces - like cert-manager.

Signed-off-by: Richard Wall <richard.wall@venafi.com>
Thanks @schelv for the suggestion.

Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj
Copy link
Member Author

wallrj commented Oct 25, 2023

I think this is handled by some Openshift "magic" described here.

@erikgb Thanks so much for digging into that! I've added a link to that doc.

@wallrj wallrj requested a review from SgtCoDFish October 25, 2023 08:45
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

There's a reasonable looking comment just been added, so I've added a hold if you want to incorporate that suggestion. Ping me for a re-review if needed!

@jetstack-bot jetstack-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Oct 25, 2023
@wallrj wallrj requested a review from SgtCoDFish October 25, 2023 09:19
@wallrj
Copy link
Member Author

wallrj commented Oct 25, 2023

/hold cancel

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2023
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2023
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erikgb, hawksight, SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@SgtCoDFish SgtCoDFish merged commit 8bc7e89 into cert-manager:master Oct 25, 2023
4 checks passed
@wallrj wallrj deleted the node-placement-recommendations branch October 25, 2023 13:14
wallrj added a commit to wallrj/website that referenced this pull request Nov 15, 2023
Thanks for all your documentation reviews Peter, for example:
 * cert-manager#1344
 * cert-manager#1338
 * cert-manager#1331

We'd like you to be able to `lgtm` future PRs,
so we're adding you to the reviewers list.

Signed-off-by: Richard Wall <richard.wall@venafi.com>
SgtCoDFish pushed a commit to SgtCoDFish/cert-manager-website that referenced this pull request Jan 18, 2024
Thanks for all your documentation reviews Peter, for example:
 * cert-manager#1344
 * cert-manager#1338
 * cert-manager#1331

We'd like you to be able to `lgtm` future PRs,
so we're adding you to the reviewers list.

Signed-off-by: Richard Wall <richard.wall@venafi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question about tolerations
8 participants