Make excluded namespace configurable #1444

yiqigao217 · 2021-03-23T17:57:43Z

Fixes #374, #1023.
Part of #1443.
See design https://bit.ly/hnc-excluded-namespaces.

Add excluded-namespace container args in the
config/manager/manager.yaml to allow user defined excluded namespaces.
The default excluded namespaces are kube-system, kube-public, hnc-system
and kube-node-lease. Add excluded-namespace label to hnc-system by
default to avoid deadlock (between cert-rotator writing secret vs object
webhook failing CLOSE) when installing HNC. Let users to add
excluded-namespaces to other namespaces.

Replace the hard-coded EX excluded namespaces.

Add webhook patch only to the object validator with namespaceSelector to
filter excluded namespaces. Add comments on why not patching other
validators. Make object validator fail CLOSE now.

Patch object webhook to only apply on "namespaced" scope objects, so
that cert-rotator can insert certs to VWHConfiguration and namespaced
objects are indeed what we care in HNC.

Add webhook rules:

Do not allow creating/updating non-excluded namespace with excluded
label;
Do not allow excluded namespaces to be created as a subnamespace;
Do not allow setting an excluded namespace as a child.

Make hierarchyConfiguration reconciler remove excluded-namespace label
on non-excluded namespaces.

Add excluded-namespace label to user-guide/concepts.md doc.

Tested by make test and make test-e2e.

yiqigao217 · 2021-03-23T17:59:33Z

/assign @adrianludwin
/assign @rjbez17

(See commit message) I was not able to think of a good e2e test case for this, so I did a manual test. Please let me know if you have any sugguestions, thanks!

incubator/hnc/config/webhook/webhook_patch.yaml

incubator/hnc/internal/reconcilers/anchor.go

incubator/hnc/cmd/manager/main.go

incubator/hnc/config/webhook/webhook_patch.yaml

incubator/hnc/internal/reconcilers/suite_test.go

incubator/hnc/internal/validators/namespace.go

yiqigao217 · 2021-03-30T00:19:01Z

Please note in this PR, I added the hnc.x-k8s.io/excluded-namespace label to hnc-system namespace by default, so that HNC installation could work, because otherwise there will be a deadlock between cert-rotator writing secret VS object webhook failing CLOSE to block it. See PR message.

I will update the design doc to reflect it if it SGTY @adrianludwin @rjbez17 .

Additionally the e2e tests always pass on a clean cluster but may be flaky on problematic cluster (with resource stuck on terminating status). I will look further into it tomorrow.

incubator/hnc/config/webhook/webhook_patch.yaml

incubator/hnc/cmd/manager/main.go

incubator/hnc/config/webhook/webhook_patch.yaml

incubator/hnc/docs/user-guide/concepts.md

incubator/hnc/internal/reconcilers/hierarchy_config.go

incubator/hnc/internal/validators/anchor.go

incubator/hnc/internal/validators/hierarchy.go

incubator/hnc/internal/validators/namespace.go

adrianludwin · 2021-03-31T00:15:04Z

incubator/hnc/internal/validators/namespace.go

@@ -99,6 +106,16 @@ func (v *Namespace) handle(req *nsRequest) admission.Response {
 	return allow("")
 }

+func (v *Namespace) illegalExcludedNamespaceLabel(req *nsRequest) admission.Response {
+	for l := range req.ns.Labels {
+		if l == api.LabelExcludedNamespace && !config.ExcludedNamespaces[req.ns.Name] {


What about if that namespace already has the label? Should we block further changes to that namespace until the label is deleted?

Yes, I updated the message to say "Please remove the %q label. See http://... for detail".

I'm not a huge fan of this. I feel like webhooks should only validate the changes in a resource, not what's already there. For example, imagine an automated process that's trying to make some unrelated change; we shouldn't block them. And we shouldn't have to ask the user to remove the label given that we've got a reconciler that can do it automatically.

sg. Changed the message back to "You cannot exclude this namespace using the %q label. See..for detail"
and commented here with:

// Note: this only blocks the request if it has a newly added illegal // excluded-namespace label because existing illegal excluded-namespace // label should have already been removed by our reconciler. For example, // even when the VWHConfiguration is removed, adding the label to a non- // excluded namespace would pass but the label is immediately removed; when // the VWHConfiguration is there but the reconcilers are down, any request // gets denied anyway.

adrianludwin

lgtm after these last few minor fixes

/cc @rjbez17

incubator/hnc/docs/user-guide/concepts.md

incubator/hnc/internal/reconcilers/hierarchy_config.go

incubator/hnc/internal/validators/anchor.go

Fixes 374, 1023 Part of 1443. See design https://bit.ly/hnc-excluded-namespaces. Add `excluded-namespace` container args in the config/manager/manager.yaml to allow user defined excluded namespaces. The default excluded namespaces are kube-system, kube-public, hnc-system and kube-node-lease. Add excluded-namespace label to hnc-system by default to avoid deadlock (between cert-rotator writing secret vs object webhook failing CLOSE) when installing HNC. Let users to add excluded-namespaces to other namespaces. Replace the hard-coded `EX` excluded namespaces. Add webhook patch only to the object validator with namespaceSelector to filter excluded namespaces. Add comments on why not patching other validators. Make object validator fail CLOSE now. Patch object webhook to only apply on "namespaced" scope objects, so that cert-rotator can insert certs to VWHConfiguration and namespaced objects are indeed what we care in HNC. Add webhook rules: 1) Do not allow creating/updating non-excluded namespace with excluded label; 2) Do not allow excluded namespaces to be created as a subnamespace; 3) Do not allow setting an excluded namespace as a child. Make hierarchyConfiguration reconciler remove excluded-namespace label on non-excluded namespaces. Add excluded-namespace label to user-guide/concepts.md doc. Tested by make test and make test-e2e.

adrianludwin · 2021-04-01T14:49:29Z

/lgtm
/approve
/hold
/assign @rjbez17

k8s-ci-robot · 2021-04-01T14:49:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin, yiqigao217

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:

~~OWNERS~~ [adrianludwin,yiqigao217]

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

yiqigao217 · 2021-04-01T15:12:27Z

FYI the doc is updated in the "Detailed Design" section. PTAL thanks!
https://bit.ly/hnc-excluded-namespaces

rjbez17

lgtm after the one question

rjbez17 · 2021-04-01T21:44:14Z

incubator/hnc/docs/user-guide/concepts.md

+Setting this label on namespaces that are not
+listed in the HNC deployment as an `excluded-namespace` is not allowed.
+
+As of March 2021, the default excluded namespaces listed in [config/manager/manager.yaml](https://github.com/kubernetes-sigs/multi-tenancy/blob/master/incubator/hnc/config/manager/manager.yaml)


I kind of feel this should be called out in the installation section or linked from their. As it's a pre/post installation action that is somewhat hidden away but also highly recommended for users to do.

Yeah, it should probably be mentioned here, but it will be more important to call it out in the release notes, which is where the latest installation instructions are.

Filed #1466 to add excluded namespace configuration to v0.8 release notes and the installation instruction.

yiqigao217 · 2021-04-06T18:01:19Z

/hold cancel
Asked Ryan offline.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 23, 2021

k8s-ci-robot requested review from Fei-Guo and srampal March 23, 2021 17:58

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2021

yiqigao217 requested review from adrianludwin and removed request for srampal and Fei-Guo March 23, 2021 17:58

k8s-ci-robot assigned adrianludwin and rjbez17 Mar 23, 2021

adrianludwin reviewed Mar 23, 2021

View reviewed changes

yiqigao217 force-pushed the v1 branch from 257645e to 8b56b3c Compare March 29, 2021 23:54

adrianludwin reviewed Mar 31, 2021

View reviewed changes

yiqigao217 force-pushed the v1 branch from 8b56b3c to 2b3cd3c Compare April 1, 2021 01:53

adrianludwin reviewed Apr 1, 2021

View reviewed changes

k8s-ci-robot requested a review from rjbez17 April 1, 2021 02:12

yiqigao217 force-pushed the v1 branch from 2b3cd3c to f468192 Compare April 1, 2021 14:30

k8s-ci-robot 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. labels Apr 1, 2021

rjbez17 reviewed Apr 1, 2021

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2021

k8s-ci-robot merged commit 7683941 into kubernetes-retired:master Apr 6, 2021

yiqigao217 mentioned this pull request Apr 12, 2021

HNC: ValidatingWebhook should have a way to avoiding operation on specific namespaces and/or object kinds #1023

Closed

yiqigao217 mentioned this pull request Apr 16, 2021

HNC: e2e tests intermittent failure due to no excluded-namespace label on kube-system ns, etc. #1480

Closed

yiqigao217 mentioned this pull request Apr 29, 2021

HNC: Add label to excluded namespaces and enforce it by reconcilers #1386

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make excluded namespace configurable #1444

Make excluded namespace configurable #1444

yiqigao217 commented Mar 23, 2021 •

edited

Loading

yiqigao217 commented Mar 23, 2021

yiqigao217 commented Mar 30, 2021

adrianludwin Mar 31, 2021

yiqigao217 Apr 1, 2021

adrianludwin Apr 1, 2021

yiqigao217 Apr 1, 2021

adrianludwin left a comment

adrianludwin commented Apr 1, 2021

k8s-ci-robot commented Apr 1, 2021

yiqigao217 commented Apr 1, 2021

rjbez17 left a comment

rjbez17 Apr 1, 2021

adrianludwin Apr 2, 2021

yiqigao217 Apr 6, 2021

yiqigao217 commented Apr 6, 2021

Make excluded namespace configurable #1444

Make excluded namespace configurable #1444

Conversation

yiqigao217 commented Mar 23, 2021 • edited Loading

yiqigao217 commented Mar 23, 2021

yiqigao217 commented Mar 30, 2021

adrianludwin Mar 31, 2021

Choose a reason for hiding this comment

yiqigao217 Apr 1, 2021

Choose a reason for hiding this comment

adrianludwin Apr 1, 2021

Choose a reason for hiding this comment

yiqigao217 Apr 1, 2021

Choose a reason for hiding this comment

adrianludwin left a comment

Choose a reason for hiding this comment

adrianludwin commented Apr 1, 2021

k8s-ci-robot commented Apr 1, 2021

yiqigao217 commented Apr 1, 2021

rjbez17 left a comment

Choose a reason for hiding this comment

rjbez17 Apr 1, 2021

Choose a reason for hiding this comment

adrianludwin Apr 2, 2021

Choose a reason for hiding this comment

yiqigao217 Apr 6, 2021

Choose a reason for hiding this comment

yiqigao217 commented Apr 6, 2021

yiqigao217 commented Mar 23, 2021 •

edited

Loading