Fix RBAC for openshift and add openshift compatibility #1395

JGodin-C2C · 2019-02-21T12:26:30Z

What this PR does / why we need it:
This PR relaxes some right in order to enable this Chart to be deployed flawlessly on Openshift.

Special notes for your reviewer:
This helm chart have some trouble launching the webhook, after investigation, it sounded like it had some trouble creating or editing some of the needed secrets.
Also, cert-manager needs to edit ingress. This cannot be done with the actual rights of this chart.

Allow Openshift to install cert-manager chart

jetstack-bot · 2019-02-21T12:26:46Z

Hi @JGodin-C2C. Thanks for your PR.

I'm waiting for a jetstack or cert-manager 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.

JGodin-C2C · 2019-02-21T14:43:29Z

/assign @munnerz

munnerz

Thanks for investigating and testing here 😄 I've added some comments, if you could try it again without those permissions that'd be great 🙏

munnerz · 2019-02-22T13:11:17Z

deploy/charts/cert-manager/templates/rbac.yaml

@@ -10,14 +10,19 @@ metadata:
    heritage: {{ .Release.Service }}
 rules:
  - apiGroups: ["certmanager.k8s.io"]
-    resources: ["certificates", "certificates/finalizers", "issuers", "clusterissuers", "orders", "orders/finalizers", "challenges"]
+    resources: ["certificates", "certificates/finalizers", "issuers", "issuers/finalizers","clusterissuers", "orders", "orders/finalizers", "challenges",  "challenges/finalizers"]


We don't use finalizers on Issuer resources - are you sure this permission is required?

munnerz · 2019-02-22T13:11:34Z

deploy/charts/cert-manager/templates/rbac.yaml

    verbs: ["*"]
  - apiGroups: [""]
-    resources: ["configmaps", "secrets", "events", "services", "pods"]
+    resources: ["configmaps", "secrets", "secrets/finalizers","events", "services", "services/finalizers","pods", "pods/finalizers"]


We don't use finalizers on secrets, services or pods 😄

munnerz · 2019-02-22T13:11:59Z

deploy/charts/cert-manager/templates/rbac.yaml

    verbs: ["*"]
  - apiGroups: ["extensions"]
-    resources: ["ingresses"]
+    resources: ["ingresses", "ingresses/finalizers", "ingresses/status"]


I'm surprised we need any of these as we don't use finalizers or the status subresource on ingresses 🤔

We need the finalzer for this as it fire this error
ingress-shim controller: Re-queuing item "hello-world-ssl/prod-ingress" due to error processing: certificates.certmanager.k8s.io "prod-ingress-ssl" is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: no RBAC policy matched, <nil>

The status, however, does not seems to be needed
That however , may be an openshift centric problem

munnerz · 2019-02-22T13:12:18Z

deploy/charts/cert-manager/templates/rbac.yaml

    verbs: ["*"]
+{{- if .Values.global.isOpenshift }}
+  - apiGroups: ["route.openshift.io"]
+    resources: ["routes", "routes/custom-host", "routes/finalizers"]


Is this required? We don't modify 'route' resources ever, so I'm surprised this is needed 🤔

So, i will first answer this , as this was the most difficult one to find.
i have no idea why , but it looks like openshift NEEDS these rules to modify the ingress.

JGodin-C2C · 2019-02-22T13:52:36Z

Thanks for the feedback , i will checkity check all this . thanks ! 💪

JGodin-C2C · 2019-02-22T14:59:33Z

ok, so , here is the refined list of rights
Most of the finalizers are here for because it looks like openshift is enforcing the policies.

munnerz · 2019-02-22T15:11:58Z

/ok-to-test

JGodin-C2C · 2019-02-22T15:31:18Z

I guess all these are errors on the test suite as i dont see any of my part ?

munnerz · 2019-03-01T20:54:37Z

It looks like some of the test failures are genuine failures caused by this PR (notably, the -bazel job)

/retest

JGodin-C2C · 2019-03-05T09:37:38Z

How can i launch the test suite on my machine ? any documentation about all this ?
I found the cert-manager/test/chart directory, but no documentation .

J\x00

munnerz · 2019-03-08T09:39:29Z

👋 you need to bump the chart version as part of this PR as well, as you're modifying the Helm chart.

If you look in deploy/charts/cert-manager/Chart.yaml and change the version: field (increment the patch version by 1).

After you've done this, you'll need to run bazel run //hack:update-deploy-gen to regenerate the 'static' deployment manifests, and then commit the resulting changes 😄

munnerz · 2019-05-01T10:20:51Z

Thanks for your persistence here 😄

/lgtm
/approve

JGodin-C2C · 2019-05-01T10:50:40Z

Well , looks like i was too late.
I bumped the version.
Should be okay now i guess.

munnerz · 2019-05-01T18:11:08Z

🙈 I'm really sorry, there's just been a new change to these files which has once again caused your branch to go out of date. One day soon we'll remove the requirement to bump the chart version in the repo altogether, as it's just really annoying and doesn't help anyone.

Would you mind doing it one more time, and then I'll lgtm & approve this PR before others?

Signed-off-by: Julien Godin <julien.godin@camptocamp.com>

Co-Authored-By: JGodin-C2C <40758407+JGodin-C2C@users.noreply.github.com> Signed-off-by: Julien Godin <julien.godin@camptocamp.com>

Signed-off-by: Julien Godin <julien.godin@camptocamp.com>

JGodin-C2C · 2019-05-02T06:02:03Z

Are we there yet ? 😄

munnerz · 2019-05-02T09:45:44Z

/retest
/lgtm
/approve

jetstack-bot · 2019-05-02T09:45:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JGodin-C2C, munnerz

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~~ [munnerz]

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

JGodin-C2C · 2019-05-02T10:16:09Z

🎉

munnerz · 2019-05-02T10:22:11Z

Thanks for the hard work on this, and sorry it's taken so long!

jetstack-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Feb 21, 2019

jetstack-bot requested a review from munnerz February 21, 2019 12:26

jetstack-bot added area/deploy Indicates a PR modifies deployment configuration size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 21, 2019

JGodin-C2C force-pushed the openshift_compatility branch from 79b3198 to 2ebd67c Compare February 21, 2019 12:56

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Feb 21, 2019

JGodin-C2C force-pushed the openshift_compatility branch from 2ebd67c to cf44cc7 Compare February 21, 2019 13:33

jetstack-bot assigned munnerz Feb 21, 2019

munnerz reviewed Feb 22, 2019

View reviewed changes

JGodin-C2C force-pushed the openshift_compatility branch from cf44cc7 to c69e1e0 Compare February 22, 2019 14:55

jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 22, 2019

munnerz added this to the v0.8 milestone Mar 7, 2019

munnerz added this to In progress in v0.8 Mar 7, 2019

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2019

jetstack-bot added the area/acme Indicates a PR directly modifies the ACME Issuer code label Apr 1, 2019

jetstack-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 29, 2019

JGodin-C2C force-pushed the openshift_compatility branch from 4177ee2 to a774712 Compare April 29, 2019 14:28

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Apr 29, 2019

JGodin-C2C force-pushed the openshift_compatility branch from a774712 to f96debb Compare April 30, 2019 12:22

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2019

JGodin-C2C force-pushed the openshift_compatility branch from f96debb to 78f7ee9 Compare April 30, 2019 12:29

jetstack-bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 1, 2019

JGodin-C2C force-pushed the openshift_compatility branch from 78f7ee9 to 16e65a3 Compare May 1, 2019 10:49

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label May 1, 2019

JGodin-C2C and others added 5 commits May 2, 2019 07:56

Fix RBAC for openshift and add openshift compatibility

4871ac3

Signed-off-by: Julien Godin <julien.godin@camptocamp.com>

Update documentation for openshift

e11456b

Signed-off-by: Julien Godin <julien.godin@camptocamp.com>

Regenerate static files

71fdf30

Signed-off-by: Julien Godin <julien.godin@camptocamp.com>

Update the crds script link

7724c46

Co-Authored-By: JGodin-C2C <40758407+JGodin-C2C@users.noreply.github.com> Signed-off-by: Julien Godin <julien.godin@camptocamp.com>

Regenerate static file and bump version

240f3fc

Signed-off-by: Julien Godin <julien.godin@camptocamp.com>

JGodin-C2C force-pushed the openshift_compatility branch from 16e65a3 to 240f3fc Compare May 2, 2019 06:00

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2019

jetstack-bot merged commit 459bb84 into cert-manager:master May 2, 2019

v0.8 automation moved this from In progress to Done May 2, 2019

gparvin mentioned this pull request May 17, 2019

Create finalizer RBAC rules #1257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RBAC for openshift and add openshift compatibility #1395

Fix RBAC for openshift and add openshift compatibility #1395

JGodin-C2C commented Feb 21, 2019 •

edited

jetstack-bot commented Feb 21, 2019

JGodin-C2C commented Feb 21, 2019

munnerz left a comment

munnerz Feb 22, 2019

munnerz Feb 22, 2019

munnerz Feb 22, 2019

JGodin-C2C Feb 22, 2019 •

edited

munnerz Feb 22, 2019

JGodin-C2C Feb 22, 2019

JGodin-C2C commented Feb 22, 2019

JGodin-C2C commented Feb 22, 2019

munnerz commented Feb 22, 2019

JGodin-C2C commented Feb 22, 2019

munnerz commented Mar 1, 2019

JGodin-C2C commented Mar 5, 2019 •

edited

munnerz commented Mar 8, 2019

munnerz commented May 1, 2019

JGodin-C2C commented May 1, 2019

munnerz commented May 1, 2019

JGodin-C2C commented May 2, 2019

munnerz commented May 2, 2019

jetstack-bot commented May 2, 2019

JGodin-C2C commented May 2, 2019

munnerz commented May 2, 2019

Fix RBAC for openshift and add openshift compatibility #1395

Fix RBAC for openshift and add openshift compatibility #1395

Conversation

JGodin-C2C commented Feb 21, 2019 • edited

jetstack-bot commented Feb 21, 2019

JGodin-C2C commented Feb 21, 2019

munnerz left a comment

Choose a reason for hiding this comment

munnerz Feb 22, 2019

Choose a reason for hiding this comment

munnerz Feb 22, 2019

Choose a reason for hiding this comment

munnerz Feb 22, 2019

Choose a reason for hiding this comment

JGodin-C2C Feb 22, 2019 • edited

Choose a reason for hiding this comment

munnerz Feb 22, 2019

Choose a reason for hiding this comment

JGodin-C2C Feb 22, 2019

Choose a reason for hiding this comment

JGodin-C2C commented Feb 22, 2019

JGodin-C2C commented Feb 22, 2019

munnerz commented Feb 22, 2019

JGodin-C2C commented Feb 22, 2019

munnerz commented Mar 1, 2019

JGodin-C2C commented Mar 5, 2019 • edited

munnerz commented Mar 8, 2019

munnerz commented May 1, 2019

JGodin-C2C commented May 1, 2019

munnerz commented May 1, 2019

JGodin-C2C commented May 2, 2019

munnerz commented May 2, 2019

jetstack-bot commented May 2, 2019

JGodin-C2C commented May 2, 2019

munnerz commented May 2, 2019

JGodin-C2C commented Feb 21, 2019 •

edited

JGodin-C2C Feb 22, 2019 •

edited

JGodin-C2C commented Mar 5, 2019 •

edited