exclude openshift namespaces via regex

[zfrhv]

Adding --included-namespace-regex option to the helm chart.
as mentioned in the docs there is only --included-namespace-regex or --excluded-namespace.
so in order to exclude all openshift.* namespaces I used the --included-namespace-regex as mentioned in this issue, this is added as an example in values.yaml

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zfrhv
Once this PR has been reviewed and has the lgtm label, please assign adrianludwin for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[k8s-ci-robot]

Hi @zfrhv. 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-sigs/prow repository.

[zfrhv]

@mochizuki875 can you please tell if you like it?

[mochizuki875]

@zfrhv

Thank you for your suggestion and PR!

There is just one thing to note.
Now, to generate Helm Chart, we need to execute make charts.
It is for keeping up-to-date with changes in HCN manifests(manifests/*.yaml generated by make manifests), which was barrier to create Helm chart previously.

Then, all files in charts/hnc/templates are re-generated by make charts like this, so you should not edit charts/hnc/templates/xxx.yaml directly.
Sorry, I should left comment somewhere...

So in this case, it's good to update hack/helm_patches/update-helm.sh to update template like that.

[zfrhv]

@mochizuki875
thank you for mentioning.
I updated the files accordingly, after running make charts it did updated the templates as expected.
i didnt added the make charts result to the PR because it also modified many other files

let me know if you want me to include the make charts result in the PR

[mochizuki875]

@zfrhv
IMO, I think it's better to add make charts result if it works as expected, because I think latest charts are expected to be in charts dir of project and some users may use the chart without make charts.

Ideally, make charts should be executed automatically and the helm chart should be updated to charts dir and github pages as commented here, but I’ve not started working yet...

[zfrhv]

in values.yaml the image repository seems to be broken
am I supposed to dig in and figure out why?

[mochizuki875]

Have you set some environment values? (see here)

By setting these values as below,

$ export HNC_REGISTRY=gcr.io/k8s-staging-multitenancy
$ export HNC_IMG_NAME=hnc-manager
$ export HNC_IMG_TAG=v1.1.0 

values.yaml should be generated like this as a result of make charts.

image:
  repository: gcr.io/k8s-staging-multitenancy/hnc-manager
  tag: v1.1.0
  imagePullPolicy: {}
  ...

[zfrhv]

sorry for making you feed me with a spoon

[mochizuki875]

/ok-to-test

[mochizuki875]

@zfrhv
Thank you for your work!
This PR seems to make sense.
/lgtm
/cc @rjbez17

Related this PR, I have two questions.
If you have some information, please let me know.

1. Some ClusterRoles(admin, edit, view) templates are replaced from admin-role in this PR.
   However, there doesn’t seems to be Binding resource to bind them.
   Is this intentional?
   It comes from Add permissions on hnc CRDs to default clusterRoles: cluster-reader, view, edit, admin #318.

2. (NIT) At first, I thought the range operation should be used for --included-namespace-regex like --excluded-namespace.
   However, includedNamespacesRegex which comes from --included-namespace-regex is defined as string not arrayArg, so we can't.
   How do you think?

[zfrhv]

@mochizuki875

1. those roles are the default permissions that users will get.
   for example if someone has view on his namespace, then he will also be able to view the hnc resources on his namespace.
   if someone has edit then he can edit and also admin.
   the clusterRoles are not supposed to be bound to anything, they have the label rbac.authorization.k8s.io/aggregate-to-edit: "true" which kubernetes uses to merge the cluster role into the default clusterRole edit.
   its just better permissions assignment instead of just giving admin to admin
2. im not good at go but when I checked how to use the includeNamespaceRegex I saw the pr that added it, and it mentions that its a string and not arrayArg here lines 69-70

[mochizuki875]

@zfrhv Thanks your information;)