Support Naming Policy #17

bells17 · 2021-09-21T18:08:16Z

ymmt2005

Please validate the regular expressions beforehand, as commented.
Also, please add a user manual for this feature.

ymmt2005 · 2021-09-26T14:41:34Z

hooks/subnamespace.go

+	return admission.Allowed("")
+}
+
+func (v *subNamespaceValidator) notMatchingNamingPolicy(ctx context.Context, sn *accuratev1.SubNamespace) (bool, error) {


The regular expression should be pre-compiled when loaded from the configuration file,
so that no error would be returned during execution.

pkg/config/types.go

ymmt2005 · 2021-09-26T14:44:24Z

pkg/config/types.go

+	Root  string `json:"root,omitempty"`
+	Match string `json:"match,omitempty"`


These two are not expected to be empty, so omitempty should be removed.

…e when setuping subNamespaceValidator

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

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

Also, please add a user manual for this feature.

I added the description and example for the naming policy setting to values.yaml.
Should we write the docs aside from the above description and example?

zoetrope

I think it would be useful to be able to refer to the group that matched root pattern in match pattern.

for example:

namingPolicies:
  - root: app-(.*)
    match: app-\1-.*

This way, we don't have to change our namingPolicies as the number of teams increases.
What do you think?

zoetrope · 2021-10-07T07:06:19Z

hooks/subnamespace.go

-	return admission.Denied(fmt.Sprintf("namespace %s is neither a root nor a sub namespace", ns.Name))
+func (v *subNamespaceValidator) notMatchingNamingPolicy(ctx context.Context, sn *accuratev1.SubNamespace) (bool, error) {
+	for _, policy := range v.namingPolicies {
+		if policy.Root.MatchString(sn.Namespace) {


I think sn.Namespace is the parent, not the root.
Is this correct?

ymmt2005 · 2021-10-07T07:14:41Z

hooks/subnamespace.go

 	serv := mgr.GetWebhookServer()

 	m := &subNamespaceMutator{
 		dec: dec,
 	}
 	serv.Register("/mutate-accurate-cybozu-com-v1-subnamespace", &webhook.Admission{Handler: m})

+	namingPolicyRegexps, err := compileNamingPolicies(namingPolicies)


compile and validate policies in pkg/config.Config.Validate method.

…onfig.Validate method

ymmt2005 · 2021-10-13T07:10:15Z

docs/config.md

+    # namingPolicies:
+    #   - root: foo
+    #     match: foo_.*
+    #   - root: bar
+    #     match: bar_.*


Why don't we uncomment these lines?

uncommented.

ymmt2005 · 2021-10-13T07:11:13Z

hooks/subnamespace.go

+
+func (v *subNamespaceValidator) notMatchingNamingPolicy(ctx context.Context, ns, root string) (bool, string, error) {
+	for _, policy := range v.namingPolicies {
+		matches := policy.Root.FindAllSubmatch([]byte(root), -1)


It'd be simpler if we use Regexp.Expand(String).

ymmt2005 · 2021-10-13T07:12:12Z

charts/accurate/values.yaml

+    #   - root: foo
+    #     match: foo_.*
+    #   - root: bar
+    #     match: bar_.*


If we adopt the proposed spec from @zoetrope, this example should be updated too.

I added an example.

ymmt2005 · 2021-10-25T07:10:18Z

hooks/subnamespace.go

+
+func (v *subNamespaceValidator) notMatchingNamingPolicy(ctx context.Context, ns, root string) (bool, string, error) {
+	for _, policy := range v.namingPolicies {
+		matches := policy.Root.FindAllSubmatchIndex([]byte(root), -1)


Why don't you use this?
https://pkg.go.dev/regexp#Regexp.FindAllStringSubmatchIndex

ymmt2005 · 2021-10-25T07:12:30Z

hooks/subnamespace.go

+		if len(matches) > 0 {
+			m := []byte{}
+			for _, match := range matches {
+				m = policy.Root.Expand(m, []byte(policy.Match), []byte(root), match)


Likewise,
https://pkg.go.dev/regexp#Regexp.ExpandString

ymmt2005 · 2021-10-25T07:16:08Z

config.yaml

@@ -25,3 +25,5 @@ watches:
  kind: Secret
 - version: v1
  kind: ResourceQuota
+
+namingPolicies: []


Please add comments just like in the other fields.

zoetrope · 2021-10-28T06:07:46Z

charts/accurate/Chart.yaml

@@ -15,7 +15,7 @@ type: application
 # This is the chart version. This version number should be incremented each time you make changes
 # to the chart and its templates, including the app version.
 # Versions are expected to follow Semantic Versioning (https://semver.org/)
-version: 0.1.2


I updated the chart version.
#32

Please fix it.

zoetrope

LGTM

pkg/config/types.go

zoetrope

LGTM

ymmt2005 · 2021-11-05T06:19:40Z

@bells17
I'm a little bit confused that there are almost two identical files, namely,
charts/accurate/values.yaml and config.yaml.

Can we remove config.yaml?

bells17 · 2021-11-08T11:40:38Z

@ymmt2005 Thank you for your reviewing and I removed config.yaml.
Would you check it?

ymmt2005

LGTM

Support Naming Policy

1748cc3

bells17 force-pushed the support-naming-policy branch from f116f46 to 1748cc3 Compare September 21, 2021 18:19

bells17 requested a review from ymmt2005 September 21, 2021 18:43

bells17 marked this pull request as ready for review September 21, 2021 18:43

zoetrope self-requested a review September 22, 2021 04:11

Merge branch 'main' into support-naming-policy

e2e94da

ymmt2005 requested changes Sep 26, 2021

View reviewed changes

bells17 added 4 commits September 27, 2021 18:06

Fix regular expressions that is compiled from naming policy to compil…

18d211a

…e when setuping subNamespaceValidator

Add comment and remove omitempty tags

81f24f4

Add namingPolicies config example to docs

634f66f

Update chart version

76f9830

zoetrope reviewed Oct 7, 2021

View reviewed changes

zoetrope requested a review from ymmt2005 October 7, 2021 07:07

ymmt2005 requested changes Oct 7, 2021

View reviewed changes

bells17 added 6 commits October 12, 2021 16:00

Fix checking naming policy process

80c5742

Move compiling and validating for the naming policies to pkg/config.C…

83b738b

…onfig.Validate method

Support naming policies subMatchs

1ccc93c

Update chart version

8e3f2da

Merge branch 'main' into support-naming-policy

ccc9986

Fix testcase

d050eaa

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

ymmt2005 requested changes Oct 13, 2021

View reviewed changes

bells17 added 3 commits October 19, 2021 18:34

Use Regexp.Expand

5c91ab4

Merge branch 'main' into support-naming-policy

01d189c

Update chart version

2de266e

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

ymmt2005 requested changes Oct 25, 2021

View reviewed changes

zoetrope reviewed Oct 28, 2021

View reviewed changes

bells17 added 3 commits November 1, 2021 16:11

Fix codes and add comment

b45c179

Merge branch 'main' into support-naming-policy

6d0ed08

Update chart version

71870a2

zoetrope previously approved these changes Nov 2, 2021

View reviewed changes

ymmt2005 reviewed Nov 2, 2021

View reviewed changes

pkg/config/types.go Outdated Show resolved Hide resolved

Update pkg/config/types.go

9421085

ymmt2005 dismissed zoetrope’s stale review via 9421085 November 2, 2021 08:54

zoetrope previously approved these changes Nov 5, 2021

View reviewed changes

zoetrope requested a review from ymmt2005 November 5, 2021 06:30

Remove config.yaml

4de6fd4

bells17 dismissed zoetrope’s stale review via 4de6fd4 November 8, 2021 11:30

bells17 requested a review from zoetrope November 8, 2021 11:40

zoetrope approved these changes Nov 11, 2021

View reviewed changes

ymmt2005 approved these changes Nov 11, 2021

View reviewed changes

ymmt2005 merged commit 606318c into main Nov 11, 2021

ymmt2005 deleted the support-naming-policy branch November 11, 2021 09:30

zoetrope mentioned this pull request Dec 8, 2021

[helm] namingPolicies in values.yaml is not reflected in configmap #41

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Naming Policy #17

Support Naming Policy #17

bells17 commented Sep 21, 2021

ymmt2005 left a comment

ymmt2005 Sep 26, 2021

ymmt2005 Sep 26, 2021

bells17 commented Oct 6, 2021

zoetrope left a comment

zoetrope Oct 7, 2021 •

edited

Loading

ymmt2005 Oct 7, 2021

ymmt2005 Oct 13, 2021

bells17 Oct 19, 2021

ymmt2005 Oct 13, 2021

bells17 Oct 19, 2021

ymmt2005 Oct 13, 2021

bells17 Oct 19, 2021

ymmt2005 Oct 25, 2021

ymmt2005 Oct 25, 2021

ymmt2005 Oct 25, 2021

zoetrope Oct 28, 2021

zoetrope left a comment

zoetrope left a comment

ymmt2005 commented Nov 5, 2021

bells17 commented Nov 8, 2021

ymmt2005 left a comment

		Root string `json:"root,omitempty"`
		Match string `json:"match,omitempty"`

Support Naming Policy #17

Support Naming Policy #17

Conversation

bells17 commented Sep 21, 2021

ymmt2005 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bells17 commented Oct 6, 2021

zoetrope left a comment

Choose a reason for hiding this comment

zoetrope Oct 7, 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

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

zoetrope left a comment

Choose a reason for hiding this comment

zoetrope left a comment

Choose a reason for hiding this comment

ymmt2005 commented Nov 5, 2021

bells17 commented Nov 8, 2021

ymmt2005 left a comment

Choose a reason for hiding this comment

zoetrope Oct 7, 2021 •

edited

Loading