Add support for restricting the secrets watch list in cainjector#5174
Add support for restricting the secrets watch list in cainjector#5174aubm wants to merge 1 commit intocert-manager:masterfrom
Conversation
|
Hi @aubm. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aubm The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @aubm thank you for the PR! Agree that cainjector (and also cert-manager controller) watching and caching all We've been thinking about fixing this by making them cache metadata only if possible. I'm not opposed to also having a flag for cainjector for this if it seems like the metadata approach alone is not enough of an improvement or if it is not feasible for some reason. The main issue with this PR is that setting this flag will by default break cert-manager's webhook because cainjector is used to sync CA cert from the webhook's self-signed CA cert stored in an (unlabelled) |
|
Hello @irbekrm, thank you for your feedback!
Let me know if there is something I can do to help with that :) In the meantime, what would you think if I reversed the logic in this PR? We could have the flag ignore secrets with a given set of labels instead. |
|
I'll take a look this week at whether the metadata caching is something that can be implemented in relatively straightforward way and then we can have a discussion about what approach can be realistically taken to solve this soon-ish. If possible I would rather not have the reverse flag as it would require folks to have to set specific labels on cert-manager unrelated resources for cert-manager to not process them - that doesn't seem like a good pattern. But similarly also am aware that this issue needs to be solved. |
|
Hello @irbekrm 👋 Did you have time to look into this? Cheers, |
|
Hey there, Just wanted to see if this is still in the pipeline. It's quite a problem on our side as well. Thanks! |
The --secrets-field-selector flag is introduced and accepts a field selector expression The following example makes cainjector ignore all secrets whose type is "helm.sh/release.v1". ``` --secrets-field-selector type!=helm.sh/release.v1 ``` Signed-off-by: Aurélien Baumann <aur.baumann@gmail.com>
|
Hey @irbekrm, just to let you know that I just updated the PR and changed the new option to restrict the secrets watch list based on a field selector instead of a label selector. I think that should address the issue your raised here:
Let me know what you think! |
|
/kind feature |
|
@aubm: PR needs rebase. DetailsInstructions 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. |
|
Issues go stale after 90d of inactivity. |
|
@aubm: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
|
Stale issues rot after 30d of inactivity. |
|
Rotten issues close after 30d of inactivity. |
|
@jetstack-bot: Closed this PR. DetailsIn response to this:
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. |
The --secrets-field-selector flag is introduced and accepts a field selector expression
The following example makes cainjector ignore all secrets whose type is "helm.sh/release.v1".
Pull Request Motivation
Some clusters have lots of creations/updates of secrets that can contain a significant amount of data, namely when heavily relying on helm 3 to deploy lots of releases.
In these clusters, the cainjector can see its memory consumption occasionaly peaks and eventually OOM crash when the allocated limit is exceeded.
To avoid that behavior, one could levarage the
--secrets-label-selectorflags introduced in the PR in order to have cainjector ignore secrets that are known to never be referenced.Kind
feature
Release Note