Copy labels and annotations from SubnamespaceAnchor to child namespace

[mishamo]

No description provided.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ mishamo (74d0e40)

[k8s-ci-robot]

Welcome @mishamo!

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]

Hi @mishamo. 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 for this change! Can you please explain a bit about why you want this feature?

But this isn't the UX I'd like to support this feature. The anchor should be able to have labels and annotations on it (e.g. "created-by: some-tool") that are not copied to the child namespaces. This is how labels and annotations work elsewhere in K8s - for example, the labels on a Deployment are not copied to its pods; rather, the podspec in the Deployment includes the labels that should be created on the pod.

In addition, HNC recently added a new feature (not yet released) called managed labels (link) and this feature needs to integrate with that. For example, this change will not propagate the labels/annotations to the descendants of the subnamespace, which we should definitely do.

So here's how I think we need to change this feature:

• In the SubnamespaceAnchor CRD, add a new .spec.labels and .spec.annotations field, mirroring the ones in HierarchyConfiguration.
• In the HierarchyConfiguration reconciler, modify the syncSubnamespaceParent method to copy the labels and annotations from the anchor to the HierarchyConfiguration. Replace anything that's there (e.g. don't attempt to add or merge). The rest of the reconciler will actually put the labels/annotations on the namespace and its descendants.
• Modify the validators to prevent adding illegal labels or annotations, at least as much as the current validators do (e.g. here). This may require some refactoring to avoid duplicating code. It's also ok to do this in a followup PR but please file an issue to do so.

Thanks!

/hold

[mishamo]

Hi. Thanks for your feedback. It all sounds quite reasonable. Will look at updating this.

Regarding the use case for this feature, we would simply like to create child namespaces with labels and annotations. We can already do this by creating the namespace with a plain Namespace resource, then using kubectl hns set to set the parent. It feels like it would be much more elegant to be able to apply a single resource in a declarative manner, that could already contain the namespaces and annotations; in this case a SubnamespaceAnchor.

[adrianludwin]

The other option is to create the NS resource, and then create the HierarchyConfiguration resource inside it, setting both the parent and labels/annotations simultaneously. It's still a two-step process, but at least it doesn't have an imperative command as a part of it. But I'm happy to make the changes to SubnamespaceAnchor as well if you feel it's important.

…

On Thu, Feb 24, 2022 at 11:13 AM mishamo ***@***.***> wrote: Hi. Thanks for your feedback. It all sounds quite reasonable. Will look at updating this. Regarding the use case for this feature, we would simply like to create child namespaces with labels and annotations. We can already do this by creating the namespace with a plain Namespace resource, then using kubectl hns set to set the parent. It feels like it would be much more elegant to be able to apply a single resource in a declarative manner, that could already contain the namespaces and annotations; in this case a SubnamespaceAnchor. — Reply to this email directly, view it on GitHub <#149 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE43PZGKITCDQKLJKL4AWZ3U4ZKKJANCNFSM5PG3LKAA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[mishamo]

Hi,

We've taken a look at the proposed implementation/UX and think there might be a slight mismatch in our usecase/goal vs the proposed approach.

We're looking to provide a mechanism for creating labels and annotations on namespaces that are created as a direct result of creating a SubnamespaceAnchor. This is a separate requirement to being able to propagate those annotations and labels. A possible approach could be to add UnmanagedLabels and UnmanagedAnnotations to SubnamespaceAnchor.Spec. These would then be copied across to the namespace that's created, but not propagated or managed by HNC. We could also add validation to make sure there are no clashes between managed and unmanaged labels and annotations.

Does that make sense? Or would there be a preferred approach to achieve this?

[erikgb]

My main concern with allowing SubnamespaceAnchor to control labels/annotations on namespaces is the security aspect. AFAIK SubnamespaceAnchor exists to allow unprivileged users to manage namespaces, which would normally be forbidden in a multi-tenant environment. With the new managed labels/annotations feature, the cluster-admin can delegate permission to the namespace-admin to set a limited set of namespace labels/annotations on the HierarchyConfiguration resource. IMO allowing unprivileged users to set namespace labels/annotations is not correct.

[adrianludwin]

I think I'm ok to have anchors include managed labels/annotations, since the admin of the parent namespace can always override what's allowed (even if we don't have a great UX for that yet). I'm much less comfortable including unmanaged labels/annotations on the subnamespaces for the reason that @erikgb says.

Can you give us a very concrete idea of why you need this feature? E.g.:

• Who is the cluster admin (or, at least, the admin of the namespace in which the anchors are being added)?
• Who is trying to create an anchor and why? What are they planning on doing with the resulting namespaces?
• What are the labels/annotations you're adding, and what do they mean? Why can't they be managed labels/annots?

This should help us either find another way for you to solve your problem or give us more clarity on how to implement this feature.

[mishamo]

Hi. I've discussed this with my colleagues and we feel that you're right in terms of security.

We are the admins of the cluster and we have ~100 tenants. We would like generate parent namespaces for each tenant (with their individual roles and rolebindings to propagate to their children). Following that, tenants would be able to create their own children by applying SubnamespaceAnchor resources to their respective parent namespace.

We would like tenants (and ourselves, when generating the parents) to be able to add managed labels and annotations in one step, by creating the SubnamespaceAnchor. The proposal of adding a Spec to the SubnamespaceAnchor CRD and copying the labels and annotations across to the HierarchyConfiguration appears reasonable to us at face value. However, we appear to have stumbled upon some issues when implementing.

We've made some changes here: https://github.com/mishamo/hierarchical-namespaces/tree/changes

We have added a test to check for label propagation (which is very similar to the test in the hierarchyconfig/reconciler_test) here: https://github.com/mishamo/hierarchical-namespaces/blob/824376bcbdcd692af00e8a76134753c32bb2faeb/internal/anchor/reconciler_test.go#L136

The issue we're having is that when the labels in the anchor are updated for the parent namespace (foo), the reconcilers for the child namespace (bar) are not triggered, because nothing has changed in those namespaces. Thus the hierarchyconfig/reconciler only reconciles for foo, not bar. We could add logic to the anchor reconciler to update the hierachyconfiguration in the child, but that feels like crossing a responsibility boundary. We could also potentially delegate to reconcile children from parents (i.e. a direct call to Reconcile with the child namespace as an arg), but that feels quite intrusive and could have cascading side-effects.

We're scratching our heads a little as to the best approach. Any advice would be welcome.

[adrianludwin]

If you change a label in the Hierarchy Config, it should trigger a reconciliation of all descendants (e.g. here). Is this not working for you?

[adrianludwin]

Hmm, what exactly is the problem? E.g. imagine you have a namespace hierarchy like this:

foo
|-bar

Is the problem that you're changing the labels/annotations on the bar anchor in foo, and then the changes are not getting propagated to the bar namespace after the initial creation? If so, I think that would match this logic, which says that when an anchor is changed, we re-reconcile the namespace the anchor is in, not the namespace created by the anchor. This is because of the finalization rules.

If that's the problem, the fix is pretty simple - just update the listening rules:

	anchorMapFn := func(obj client.Object) []reconcile.Request {
		return []reconcile.Request{
			{NamespacedName: types.NamespacedName{
				Name:      api.Singleton,
				Namespace: obj.GetNamespace(),
			}},
>>>			{NamespacedName: types.NamespacedName{
>>>				Name:      api.Singleton,
>>>				Namespace: obj.GetName(),
>>>			}},
		}
	}

This means that whenever an anchor is changed, we'll re-reconcile both the namespace that the anchor is in, as well as the subnamespace represented by that anchor.

[mishamo]

Hi. Yes that sounds exactly like the issue. I'll take a look at your suggestion on Monday. Thanks.

[adrianludwin]

Great, let me know!

…

On Sat, Mar 5, 2022 at 5:39 PM mishamo ***@***.***> wrote: Hi. Yes that sounds exactly like the issue. I'll take a look at your suggestion on Monday. Thanks. — Reply to this email directly, view it on GitHub <#149 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE43PZEHHQXVGG2ZDEV53OLU6PPA3ANCNFSM5PG3LKAA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: mishamo (a925804)

[mishamo]

@adrianludwin How does this look to you? Is it likely that this would fit into the 1.0 release?

[adrianludwin]

Will review today, thanks!

…

On Tue, Mar 15, 2022 at 10:24 AM mishamo ***@***.***> wrote: @adrianludwin <https://github.com/adrianludwin> How does this look to you? Is it likely that this would fit into the 1.0 release? — Reply to this email directly, view it on GitHub <#149 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE43PZBQRO6HJKOJTY4STETVACMS3ANCNFSM5PG3LKAA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[adrianludwin]

This is looking great, thanks for making these changes. Just a few minor revisions.

/ok-to-test

[adrianludwin]

Also, can you please update the commit message to describe the change as well as how you've tested it (e.g. "Added integ tests and tested manually" is probably enough for this case).

[adrianludwin]

E.g. see this commit message: 48df984

[adrianludwin]

lgtm after one last cleanup of the commit message. It currently looks like you've merged a bunch of commit messages together? If you could modify it to look more like 48df984 I'd appreciate that!

[mishamo]

Thanks for that! I've updated the commit message with a brief description of the feature and removed the multiple interim commit messages.

[mishamo]

/retest

[mishamo]

I'm not quite sure why these tests (go vet) are failing? The imports look OK to me and I can't reproduce this locally.

[erikgb]

I'm not quite sure why these tests (go vet) are failing? The imports look OK to me and I can't reproduce this locally.

@mishamo I think the pipeline runs the result of adding your commit on top of the master branch. If you rebase on top on master you will be able to reproduce locally. I just did from a copy of your PR branch. 😄

[mishamo]

@adrianludwin what are the chances of this making it into the 1.0 cut?

[adrianludwin]

Let's get #160 in first, then we'll merge this. I think v1.0 will go out shortly after that.

[adrianludwin]

Thanks for this change!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

[rjbez17]

@adrianludwin I think we can remove this hold correct?

[adrianludwin]

Whoops, thanks!

/hold cancel