Add labels and annotations field to SubNamespace resource #19

bells17 · 2021-09-23T11:01:46Z

api/v1/subnamespace_types.go

controllers/subnamespace_controller.go

zoetrope

I think this feature should be used with caution, because tenant users may get privileges by setting labels or annotations.
For example, PodSecurity Admission will control policies using namespace labels.

So, please write a note in the documentation like the following:
If you are using something that controls permissions by labels or annotations, such as PodSecurity Admission, an administrator should set root namespaces appropriate labels and annotations.

ymmt2005 · 2021-09-27T05:12:57Z

@zoetrope
Accurate only sets the labels and annotations that are explicitly permitted
in the configuration file. So, admins have full control over what a tenant
user can do.

ymmt2005 · 2021-09-27T05:14:24Z

@bells17 @zoetrope
One thing that I haven't determined is that whether we should have a different set of
label/annotation keys for this feature than the keys for propagation.

What do you think?

zoetrope · 2021-09-27T10:20:38Z

I understand.

I prefer a simple configuration.
I think we should not have the different set.

bells17 · 2021-09-28T01:51:02Z

I agree with @zoetrope.

bells17 · 2021-09-28T01:55:24Z

By the way, should we prioritize SubNamespace labels/annotations over parental Namespace labels/annotations if there have the same labels/annotations key?

ymmt2005 · 2021-09-28T01:55:25Z

OK, let's go with the current proposal.

ymmt2005 · 2021-09-28T01:56:31Z

@bells17

By the way, should we prioritize SubNamespace labels/annotations over parental Namespace labels/annotations if there have the same labels/annotations key?

Definitely not. And even if they would conflict, the parent Namespace labels/annotations win because they are propagated later on.

bells17 · 2021-10-06T06:35:56Z

Thank you @ymmt2005 @zoetrope .
I've corrected the points you commented on.
Would you review this PR again?

ymmt2005

Please add a section for this feature to the user manual, too.

api/v1/subnamespace_types.go

controllers/subnamespace_controller.go

ymmt2005

approving, but could you add a section to the user manual for this feature?
@bells17

ymmt2005

@bells17
@zoetrope and I have concluded that the current implementation is vulnerable
for sensitive labels/annotations like one for MetaLB address pools or one for
pod-security-admission policy.

As mentioned in #19 (comment) ,
we should have a separate set of label/annotation keys that SubNamespace can specify to
protect from malicious use.

Please update the design and fix the implementation.

bells17 · 2021-11-17T16:14:09Z

@ymmt2005 @zoetrope The following points have been updated:

SubNamespace spec.labels/spec.annotations is now propagated to descendants namespaces.
Added --labels and --annotations options to the kubectl-accurate sub create command.
Wrote docs.

Would you review these points?

zoetrope · 2021-11-18T06:27:34Z

controllers/namespace_controller.go

+				subNSHandler(ev.ObjectOld, q)
+			},
+			DeleteFunc: func(ev event.DeleteEvent, q workqueue.RateLimitingInterface) {
+				subNSHandler(ev.Object, q)


This is not necessary, because when SubNamespace is deleted, the target Namespace will be also deleted.

zoetrope

I read the code.
I think the following spec is fine.

SubNamespace spec.labels/spec.annotations is now propagated to descendants namespaces.

SubNamespace spec.labels/spec.annotations values are preferred more than parent namespace labels/annotations values.

zoetrope

LGTM

ymmt2005 · 2021-11-22T05:27:27Z

cmd/kubectl-accurate/sub/sub_create.go

@@ -36,6 +38,8 @@ This effectively creates a namespace named NAME as a sub-namespace of NS.`,
 		},
 	}

+	cmd.Flags().StringToStringVar(&opts.labels, "labels", opts.labels, "the labels to be propagated to the sub-namespace")


Please add an example of the usage to the description. e.g.,

Suggested change

cmd.Flags().StringToStringVar(&opts.labels, "labels", opts.labels, "the labels to be propagated to the sub-namespace")

cmd.Flags().StringToStringVar(&opts.labels, "labels", opts.labels, "the labels to be propagated to the sub-namespace. Example: a=b,c=d")

ymmt2005 · 2021-11-22T05:27:39Z

cmd/kubectl-accurate/sub/sub_create.go

@@ -36,6 +38,8 @@ This effectively creates a namespace named NAME as a sub-namespace of NS.`,
 		},
 	}

+	cmd.Flags().StringToStringVar(&opts.labels, "labels", opts.labels, "the labels to be propagated to the sub-namespace")
+	cmd.Flags().StringToStringVar(&opts.annotations, "annotations", opts.annotations, "the annotations to be propagated to the sub-namespace")


ymmt2005

Please validate the labels and annotations in SubNamespace spec field.
If a SubNamespace is created with an invalid key or value, the current implementation would fall into an infinite error loop.

Use v1validation.ValidateLabels to validate labels and validatoin.ValidateAnnotations to validate annotations.

ref. https://github.com/kubernetes/apimachinery/blob/v0.22.4/pkg/api/validation/objectmeta.go#L194-L195

Also, could you update this part of the user manual for this new feature?
https://cybozu-go.github.io/accurate/subnamespaces.html#creating-a-sub-namespace

docs/config.md

ymmt2005 · 2021-11-23T22:59:32Z

controllers/namespace_controller.go

@@ -92,6 +100,27 @@ func (r *NamespaceReconciler) propagateMeta(ctx context.Context, ns, parent *cor
 			ns.Annotations[k] = v
 		}
 	}
+
+	subNS := &accuratev1.SubNamespace{}
+	err := r.Get(ctx, types.NamespacedName{Name: ns.Name, Namespace: parent.Name}, subNS)


At the line 347, this method is called for a template instance namespace, not a sub-namespace.

I think this should look up a SubNamespace only when ns is a sub-namespace (i.e., it has accurate.cybozu.com/parent label).

Co-authored-by: Yamamoto, Hirotaka <ymmt2005@gmail.com>

bells17 · 2021-12-01T08:50:20Z

Also, could you update this part of the user manual for this new feature?
https://cybozu-go.github.io/accurate/subnamespaces.html#creating-a-sub-namespace

I've already updated the user manual, but should I need to make additional updates?
https://github.com/cybozu-go/accurate/pull/19/files#diff-b394f5d0e85c3cd9c802a88109196f87c4f5047d04b50c0c557476474460adc4R88-R118

ymmt2005 · 2021-12-01T08:54:04Z

@bells17

I've already updated the user manual, but should I need to make additional updates?

My bad. I overlooked that. It's enough, thank you.

bells17 · 2021-12-01T16:34:18Z

Please validate the labels and annotations in SubNamespace spec field.

added.

bells17 · 2021-12-01T16:37:16Z

@ymmt2005 (cc: @zoetrope) Thank you. I fixed points that are pointed on.

ymmt2005 · 2021-12-03T06:38:49Z

controllers/namespace_controller.go

+		err := r.Get(ctx, types.NamespacedName{Name: ns.Name, Namespace: parent.Name}, subNS)
+		if err != nil && !apierrors.IsNotFound(err) {
+			return fmt.Errorf("failed to get sub namespace %s/%s: %w", ns.Name, parent.Name, err)
+		}


What happens if apierrors.IsNotFound(err) is true?
In that case, subNS is invalid. Although the following code would work for such invalid data,
I want to check them and skip code using subNS.

ymmt2005

LGTM

Add labels and annotations field to SubNamespace resource

eccf069

bells17 requested a review from ymmt2005 September 24, 2021 06:44

bells17 marked this pull request as ready for review September 24, 2021 06:44

bells17 requested a review from zoetrope September 24, 2021 06:46

ymmt2005 requested changes Sep 26, 2021

View reviewed changes

api/v1/subnamespace_types.go Show resolved Hide resolved

api/v1/subnamespace_types.go Outdated Show resolved Hide resolved

controllers/subnamespace_controller.go Outdated Show resolved Hide resolved

zoetrope reviewed Sep 27, 2021

View reviewed changes

bells17 added 2 commits October 5, 2021 17:21

Fix

59b8588

Add comments

71b1a32

zoetrope requested review from zoetrope and ymmt2005 October 11, 2021 04:52

ymmt2005 requested changes Oct 11, 2021

View reviewed changes

api/v1/subnamespace_types.go Outdated Show resolved Hide resolved

api/v1/subnamespace_types.go Outdated Show resolved Hide resolved

controllers/subnamespace_controller.go Outdated Show resolved Hide resolved

bells17 added 2 commits October 13, 2021 01:03

Fix codes

cff5a5a

Merge branch 'main' into add-labels-and-annotations-field2

05ae0c2

bells17 requested a review from ymmt2005 October 13, 2021 06:41

ymmt2005 requested changes Oct 13, 2021

View reviewed changes

controllers/subnamespace_controller.go Outdated Show resolved Hide resolved

bells17 added 3 commits October 20, 2021 01:43

Fix code

15fd9ee

Update chart version

6b2cef1

Merge branch 'main' into add-labels-and-annotations-field2

8fac21a

bells17 requested a review from ymmt2005 October 19, 2021 16:55

ymmt2005 approved these changes Oct 25, 2021

View reviewed changes

ymmt2005 requested changes Oct 26, 2021

View reviewed changes

bells17 force-pushed the add-labels-and-annotations-field2 branch from 91aee10 to 8fac21a Compare November 1, 2021 08:03

Update docs

f8f1134

bells17 requested a review from zoetrope November 17, 2021 16:05

zoetrope reviewed Nov 18, 2021

View reviewed changes

Remove SubNamespace DeleteFunc from NamespaceReconciler

1302f9f

bells17 requested a review from zoetrope November 18, 2021 23:18

zoetrope previously approved these changes Nov 19, 2021

View reviewed changes

ymmt2005 reviewed Nov 22, 2021

View reviewed changes

ymmt2005 requested changes Nov 22, 2021

View reviewed changes

docs/config.md Outdated Show resolved Hide resolved

ymmt2005 reviewed Nov 23, 2021

View reviewed changes

Update docs/config.md

d812f76

Co-authored-by: Yamamoto, Hirotaka <ymmt2005@gmail.com>

bells17 dismissed zoetrope’s stale review via d812f76 December 1, 2021 07:51

bells17 added 3 commits December 2, 2021 01:17

Add example values to descriptions

5b89d8f

Add check that the namespace is a sub-namespace

a2ba084

Add validations for spec.labels and spec.annotations fields

5fa63b5

bells17 requested review from ymmt2005 and zoetrope December 1, 2021 16:34

ymmt2005 requested changes Dec 3, 2021

View reviewed changes

Modify to skip when apierrors.IsNotFound(err) is true

75d104c

bells17 requested a review from ymmt2005 December 5, 2021 16:48

ymmt2005 approved these changes Dec 6, 2021

View reviewed changes

ymmt2005 merged commit 6773c56 into main Dec 6, 2021

ymmt2005 deleted the add-labels-and-annotations-field2 branch December 6, 2021 02:27

ymmt2005 mentioned this pull request Dec 6, 2021

Set labels/annotations of sub-namespaces #7

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add labels and annotations field to SubNamespace resource #19

Add labels and annotations field to SubNamespace resource #19

bells17 commented Sep 23, 2021 •

edited

Loading

zoetrope left a comment •

edited

Loading

ymmt2005 commented Sep 27, 2021

ymmt2005 commented Sep 27, 2021

zoetrope commented Sep 27, 2021

bells17 commented Sep 28, 2021

bells17 commented Sep 28, 2021

ymmt2005 commented Sep 28, 2021

ymmt2005 commented Sep 28, 2021

bells17 commented Oct 6, 2021

ymmt2005 left a comment

ymmt2005 left a comment •

edited

Loading

ymmt2005 left a comment •

edited

Loading

bells17 commented Nov 17, 2021

zoetrope Nov 18, 2021

zoetrope left a comment

zoetrope left a comment

ymmt2005 Nov 22, 2021

bells17 Dec 1, 2021

ymmt2005 Nov 22, 2021

bells17 Dec 1, 2021

ymmt2005 left a comment •

edited

Loading

ymmt2005 Nov 23, 2021

bells17 Dec 1, 2021

bells17 commented Dec 1, 2021

ymmt2005 commented Dec 1, 2021

bells17 commented Dec 1, 2021

bells17 commented Dec 1, 2021

ymmt2005 Dec 3, 2021

ymmt2005 left a comment

	cmd.Flags().StringToStringVar(&opts.labels, "labels", opts.labels, "the labels to be propagated to the sub-namespace")
	cmd.Flags().StringToStringVar(&opts.labels, "labels", opts.labels, "the labels to be propagated to the sub-namespace. Example: a=b,c=d")

Add labels and annotations field to SubNamespace resource #19

Add labels and annotations field to SubNamespace resource #19

Conversation

bells17 commented Sep 23, 2021 • edited Loading

zoetrope left a comment • edited Loading

Choose a reason for hiding this comment

ymmt2005 commented Sep 27, 2021

ymmt2005 commented Sep 27, 2021

zoetrope commented Sep 27, 2021

bells17 commented Sep 28, 2021

bells17 commented Sep 28, 2021

ymmt2005 commented Sep 28, 2021

ymmt2005 commented Sep 28, 2021

bells17 commented Oct 6, 2021

ymmt2005 left a comment

Choose a reason for hiding this comment

ymmt2005 left a comment • edited Loading

Choose a reason for hiding this comment

ymmt2005 left a comment • edited Loading

Choose a reason for hiding this comment

bells17 commented Nov 17, 2021

Choose a reason for hiding this comment

zoetrope left a comment

Choose a reason for hiding this comment

zoetrope left a 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

ymmt2005 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bells17 commented Dec 1, 2021

ymmt2005 commented Dec 1, 2021

bells17 commented Dec 1, 2021

bells17 commented Dec 1, 2021

Choose a reason for hiding this comment

ymmt2005 left a comment

Choose a reason for hiding this comment

bells17 commented Sep 23, 2021 •

edited

Loading

zoetrope left a comment •

edited

Loading

ymmt2005 left a comment •

edited

Loading

ymmt2005 left a comment •

edited

Loading

ymmt2005 left a comment •

edited

Loading