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

fix(helm): fix the namespaceSelector that prevents the webhook from running in the release namespace #90

Merged

Conversation

acobaugh
Copy link
Contributor

I noticed my cluster would get wedged any time the gmsa webhook pods weren't running. I've determined the namespaceSelectors for the webhook configurations are not correct.

This PR fixes this, and also allows an additional label to be used on namespaces (windows.k8s.io/disabled: true) to allow the webhooks to be skipped in other namespaces. I just picked that name, but I'm open to using something else. At least in my cluster, there are other namespaces besides kube-system that are critical (karpenter, istio-system, etc) which will never have windows pods in them, but should still be able to function if gmsa is down.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 12, 2022
@k8s-ci-robot k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 12, 2022
@jsturtevant
Copy link
Contributor

Thanks, this looks like it was indeed a bug in regards to the namespace name. I just ran into this myself on another project. Dropping the link to the comment we found that explained this: kubernetes/kubernetes#92157 (comment).

The other option would be to opt-in to running it on specific namespaces which might be more appropriate?

@acobaugh
Copy link
Contributor Author

The other option would be to opt-in to running it on specific namespaces which might be more appropriate?

I think I actually like this more, but it will take a bit more work in the chart to make this possible. So basically we'd have a xx/disabled: true OR xx/enabled: true label for namespaces. But, I don't know how other people are using this project. It would make sense that the number of namespaces using gmsa would be less than those that aren't, but I don't know.

Open to ideas here. My priority is at least fixing what's already there ahead of my windows workloads hitting production.

@jsturtevant
Copy link
Contributor

I guess it would be a breaking change to require opt in, but could manage that through the helm option maybe?

Opting out seems like a fine addition and could potentially be used together I think.

@acobaugh
Copy link
Contributor Author

Yeah, helm value could just default to opt-out, which wouldn't break anyone.

@jsturtevant
Copy link
Contributor

sounds good, could be a follow up PR if you are interested

/lgtm
/assign @marosset

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2022
@marosset
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acobaugh, marosset

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2022
@k8s-ci-robot k8s-ci-robot merged commit 9f8c15f into kubernetes-sigs:master Oct 13, 2022
@jsturtevant jsturtevant mentioned this pull request Jan 3, 2023
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants