Add --include-namespaces-regex features #89

santinoncs · 2021-10-07T08:27:17Z

In order to just specify few namespaces to be managed by HNC we implemented this logic. So in the case you want to exclude everything except specific namespaces, you can set this parameter

--included-namespace-regex

Check this out for more info

#87

k8s-ci-robot · 2021-10-07T08:27:25Z

Welcome @santinoncs!

It looks like this is your first PR to kubernetes-sigs/hierarchical-namespaces 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/hierarchical-namespaces has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-10-07T08:27:25Z

Hi @santinoncs. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

adrianludwin

Thanks, this looks like a good change! I'd forgotten that we'd already switched to automatically managing the labels, that's really nice since it makes this change much easier.

I've left some detailed comments in the relevant lines of code, but once you've made those changes, can you please:

Squash all your commits into one commit and then force-push them to your branch? That'll keep our history cleaner.
In your commit message, include a paragraph or two about how you tested this change, especially since you're not adding end-to-end tests. A good example of such a comment is here.

Thanks!

Dockerfile

config/manager/manager.yaml

internal/mutators/namespace.go

adrianludwin · 2021-10-07T14:11:52Z

internal/reconcilers/hierarchy_config.go

-	if config.ExcludedNamespaces[ns] {
+	// Early exit if it's an excluded namespace
+
+	if !utils.IsNamespaceIncluded(ns, config.IncludedNamespacesRegex, config.ExcludedNamespaces) {


Note all the repetition here - another hint that IsNamespaceIncluded should only take one parameter (the one that changes each time it's called), not three (two of which never change).

I think config.ExcludeNamespaces changes depending on the point in code you call IsNamespaceIncluded.
So this would be

if !utils.IsNamespaceIncluded(ns, config.ExcludedNamespaces) {

and as you wrote down in the comment below, use

SetIncludedNamespaces()

to set the

config.IncludedNamespacesRegex

What do you think ?

I had another look and config.ExcludedNamespaces is only set by the command-line flags, and never changes once HNC starts. So it should be a constant throughout. The only exception is for unit tests, but you can just call config.SetNamespaces(regex, excluded) in unit tests too.

internal/reconcilers/hierarchy_config.go

pkg/utils/utils.go

internal/config/default_config.go

adrianludwin · 2021-10-08T13:28:02Z

Makefile

@@ -76,7 +76,7 @@ GOBIN ?= $(shell go env GOPATH)/bin
 HNC_KREW_TAR_SHA256=$(shell sha256sum bin/kubectl-hns.tar.gz | cut -d " " -f 1)

 # The directories to run Go command on (excludes /vendor)
-DIRS=./api/... ./cmd/... ./internal/...
+DIRS=./api/... ./cmd/... ./internal/... ./pkg/...


adrianludwin · 2021-10-08T13:31:43Z

internal/config/namespace.go

+		return false
+	}
+
+	match, err := regexp.MatchString(includedNamespacesRegex, name)


Are we changing the semantics to include an implied ^...$? E.g. if I say --included-namespace-regex=test-.*, will I match prod-test-service or not?

it seems good idea to add this implied ^..$
in this case this won't match

prod-test-service

adrianludwin · 2021-10-08T13:32:36Z

internal/config/namespace_test.go

+		excludeNamespaces map[string]bool
+		expect            bool
+	}{
+		{name: "foobar", regex: "foo*", excludeNamespaces: map[string]bool{"bar": true}, expect: true},


This will also match fo. Is that what you intended? Or did you mean foo.*?

adrianludwin · 2021-10-08T13:33:14Z

internal/mutators/namespace.go

@@ -65,6 +65,7 @@ func (m *Namespace) handle(log logr.Logger, ns *corev1.Namespace) {
 		log.Info("Not an excluded namespace; set included-namespace label.")
 		ns.Labels[api.LabelIncludedNamespace] = "true"
 	}
+


Remove extra newlines please?

internal/reconcilers/anchor.go

internal/reconcilers/hierarchy_config.go

internal/validators/anchor.go

internal/validators/namespace.go

internal/validators/object.go

adrianludwin

Let's make ExcludedNamespaces private, and then squash all the commits and I think we're good to go. Thanks.

internal/config/namespace.go

adrianludwin

lgtm with a few final nits in the details and one or two last questions about testing.

Can you please give the commit a nice, short title? e.g. it turns up strangely if you look at it here, as well as in the git log command. E.g. the title could be something like:

Add --include-namespaces-regex features

Finally, did you test this manually, e.g. by using a regex like test-.*? If so, can you please add that to the "Testing" paragraph in your commit message? Right now, it doesn't say that you've done any end-to-end testing and you didn't add any end-to-end tests (which are really tricky for this kind of change), so let's just record what you did do in the commit message.

/cc @rjbez17
Ryan, do you want to check this out?

adrianludwin · 2021-10-08T20:46:11Z

internal/reconcilers/anchor.go

@@ -75,7 +74,8 @@ func (r *AnchorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr

 	// Always delete anchor (and any other HNC CRs) in the excluded namespaces and
 	// early exit.
-	if config.ExcludedNamespaces[pnm] {
+


internal/reconcilers/hierarchy_config_test.go

cmd/manager/main.go

rjbez17 · 2021-10-09T14:27:51Z

internal/config/namespace.go

+	}
+
+	match, err := regexp.MatchString("^"+includedNamespacesRegex+"$", name)
+	if err != nil {


If err exists here is usually means some sort of regex compiling error, it seems like that may be helpful to log so admins can see their regex is bad.

I missed that, good catch!

It would be unfortunate to pass a logger into each call of this function, so maybe in SetNamespaces, rather than saving the string version of the regex, Compile it into a Regexp object and save that? Then you can call Regexp.Match which doesn't return an error.

internal/config/namespace.go

adrianludwin

Thanks, the code looks good. Just two things left:

Can you please squash all the commits into one?
Your "testing" comment in the first commit message just says that you ran the automated tests, which isn't necessary (the CI system will run that), so you can take that out. But it doesn't say how you manually tested it. E.g. did you install HNC on your system with a regex and verify that everything still works? If so, please document that, and if not, can you please try that, and then update the commit message?

Other than that, lgtm! Thanks for this change, I'll approve it.

This PR allows us to select which namespaces we want HNC to manage. By adding a new parameter called "--included-namespaces-regex" we will select them. By choosing this way we are including the namespaces that match this regex. If this parameter is not included, we assume that all namespaces except excluded ones are included, as usual. Testing: the function isIncluded is the one that is called and tells you if this namespace is included or not. There is a test for this function in the config package. Testing process: * I manually deploy via kind ( make deploy ) with this startup parameters - --excluded-namespace=kube-system - --excluded-namespace=kube-public - --excluded-namespace=hnc-system - --excluded-namespace=kube-node-lease - --included-namespace-regex=test-.* - --excluded-namespace=test-1 this resulted in the following output $ kubectl get ns --show-labels NAME STATUS AGE LABELS default Active 89m default.tree.hnc.x-k8s.io/depth=0 hnc-system Active 71s control-plane=controller-manager kube-node-lease Active 89m <none> kube-public Active 89m <none> kube-system Active 89m <none> local-path-storage Active 89m local-path-storage.tree.hnc.x-k8s.io/depth=0 test-1 Active 5s <none> test-2 Active 6s hnc.x-k8s.io/included-namespace=true,test-2.tree.hnc.x-k8s.io/depth=0

adrianludwin · 2021-10-10T14:08:18Z

This is a great change, thanks so much!

/lgtm
/approve

k8s-ci-robot · 2021-10-10T14:08:29Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

santinoncs · 2021-10-10T14:56:04Z

This is a great change, thanks so much!

/lgtm

/approve

Thanks for your help

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 7, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 7, 2021

k8s-ci-robot requested review from adrianludwin and tashimi October 7, 2021 08:27

adrianludwin reviewed Oct 7, 2021

View reviewed changes

adrianludwin reviewed Oct 8, 2021

View reviewed changes

internal/config/namespace.go Outdated Show resolved Hide resolved

internal/config/namespace.go Outdated Show resolved Hide resolved

santinoncs force-pushed the included-namespaces branch 2 times, most recently from 273b7df to 1bf897d Compare October 8, 2021 16:21

adrianludwin reviewed Oct 8, 2021

View reviewed changes

k8s-ci-robot requested a review from rjbez17 October 8, 2021 20:54

santinoncs force-pushed the included-namespaces branch from 6f552f3 to 76bb724 Compare October 9, 2021 08:45

rjbez17 reviewed Oct 9, 2021

View reviewed changes

adrianludwin reviewed Oct 9, 2021

View reviewed changes

internal/config/namespace.go Outdated Show resolved Hide resolved

adrianludwin reviewed Oct 9, 2021

View reviewed changes

santinoncs force-pushed the included-namespaces branch from 80d4f7e to 77c3d14 Compare October 9, 2021 19:25

adrianludwin changed the title ~~Included namespaces logic~~ Add --include-namespaces-regex features Oct 10, 2021

k8s-ci-robot assigned adrianludwin Oct 10, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2021

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

k8s-ci-robot merged commit e297652 into kubernetes-sigs:master Oct 10, 2021

adrianludwin added this to the release-v0.9 milestone Oct 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --include-namespaces-regex features #89

Add --include-namespaces-regex features #89

santinoncs commented Oct 7, 2021

k8s-ci-robot commented Oct 7, 2021

k8s-ci-robot commented Oct 7, 2021

adrianludwin left a comment

adrianludwin Oct 7, 2021

santinoncs Oct 8, 2021 •

edited

Loading

adrianludwin Oct 8, 2021

adrianludwin Oct 8, 2021

adrianludwin Oct 8, 2021

santinoncs Oct 8, 2021

adrianludwin Oct 8, 2021

adrianludwin Oct 8, 2021

adrianludwin left a comment

adrianludwin left a comment

adrianludwin Oct 8, 2021

rjbez17 Oct 9, 2021 •

edited

Loading

adrianludwin Oct 9, 2021

adrianludwin left a comment

adrianludwin commented Oct 10, 2021

k8s-ci-robot commented Oct 10, 2021

santinoncs commented Oct 10, 2021

Add --include-namespaces-regex features #89

Add --include-namespaces-regex features #89

Conversation

santinoncs commented Oct 7, 2021

k8s-ci-robot commented Oct 7, 2021

k8s-ci-robot commented Oct 7, 2021

adrianludwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santinoncs Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianludwin left a comment

Choose a reason for hiding this comment

adrianludwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbez17 Oct 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianludwin left a comment

Choose a reason for hiding this comment

adrianludwin commented Oct 10, 2021

k8s-ci-robot commented Oct 10, 2021

santinoncs commented Oct 10, 2021

santinoncs Oct 8, 2021 •

edited

Loading

rjbez17 Oct 9, 2021 •

edited

Loading