cert-manager: fast-forward to upstream 3945595b by munnerz · Pull Request #7644 · helm/charts

munnerz · 2018-09-10T14:30:34Z

What this PR does / why we need it:

This PR should not be merged yet

This is a draft PR to update the Helm chart for the v0.5 cert-manager release.

I am opening it now ahead of time, as it involves a few larger changes than usual and I'd like to get review feedback ahead of cutting a release to ensure the PR can be accepted in its current form.

A breakdown of what is is included and corresponding PRs:

added affinity and tolerations (added affinity and tolerations cert-manager/cert-manager#869)
Add validating webhook and webhook tls autoconfiguration (Add validating webhook and webhook tls autoconfiguration cert-manager/cert-manager#478)
chart: annotate all CRDs with "crd-install" hook (chart: annotate all CRDs with "crd-install" hook cert-manager/cert-manager#823)
helm chart: remove endpoints from rbac resources (helm chart: remove endpoints from rbac resources cert-manager/cert-manager#769)

/hold
/cc @unguiculus

munnerz

Added a few comments to help with review!

munnerz · 2018-09-10T14:30:47Z

stable/cert-manager/Chart.yaml

These will both be bumped to v0.5

munnerz · 2018-09-10T14:31:02Z

stable/cert-manager/README.md

This change will be reverted and updated to v0.5.0 accordingly

munnerz · 2018-09-10T14:32:26Z

stable/cert-manager/templates/00-namespace.yaml

This file is only created when generating static deployment manifests, and the updates here are to keep it in line with default Helm 2.10 behaviour (and the second one is to help with the webhook deployment, and is effectively a no-op as we also match on the 'name' label)

munnerz · 2018-09-10T14:32:57Z

stable/cert-manager/templates/certificate-crd.yaml

I noticed issues when adding the helm.sh/hook: crd-install hook with versions of Tiller that do not support the hook.

munnerz · 2018-09-10T14:33:31Z

stable/cert-manager/templates/rbac.yaml

This means we drop support for cert-manager v0.2.0-v0.2.2 (iirc). In general, our rule is v0.4.x of the chart is compatible with v0.4.y of cert-manager (i.e. we do not pin the patch version of appVersion and version, but we do pin minor versions).

munnerz · 2018-09-10T14:34:44Z

stable/cert-manager/values.yaml

Will be updated to v0.5.0 and IfNotPresent

munnerz · 2018-09-10T14:34:55Z

stable/cert-manager/values.yaml

We disable the new webhook chart by default

can you add a test to validate when the webhook is true?

cpanato · 2018-09-10T14:39:33Z

@munnerz please sign the DCO and

since you are introducing some new things please bump the minor version or move to 1.0.0, since this chart is in the stable folder and was discussed in the helm chart meeting last week that all charts in stable should be at least 1.0.0 and not 0.x.y

side question: why use a dev image?

munnerz · 2018-09-10T14:45:02Z

Hey @cpanato,

re: DCO - will do. I am just putting a few changes together for this PR (i.e. the image tags). As I say, still WIP 😄

... in the helm chart meeting last week that all charts in stable should be at least 1.0.0 and not 0.x.y

This goes against our own versioning strategy - as explained here #7644 (comment), to make user's lives easier, we pin our minor versions to each other so they can be sure that v0.4.x of the Helm chart works with (and deploys) v0.4.y of cert-manager. This makes users rolling back/forward far easier, and helps when users understand when they can and cannot use a simple helm upgrade cert-manager ... --set image.tag=v0.4.y.

We'd really like to not have to modify this behaviour, as I think it hurts usability and will generally just not help users at all.

As such, I'd like to request we can continue to use a v0.x.y format until cert-manager itself reaches (v1.0.0) (which will hopefully be within the next few months).

edit:

if not, I think it's best this chart is moved to incubator, or otherwise removed from stable so we can host it ourselves until cert-manager does reach 1.0

cpanato · 2018-09-10T14:53:31Z

thanks so much for the clarification.

Can you set the title if the PR to say that is in WIP? like [WIP] cert-manager: fast-forward to upstream 3945595b

* Update version numbers for v0.5.0 (cert-manager/cert-manager#885) * added affinity and tolerations (cert-manager/cert-manager#869) * Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478) * chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823) * helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769) Signed-off-by: James Munnelly <james@munnelly.eu>

munnerz · 2018-09-10T15:52:01Z

I've updated the version numbers as mentioned in my above comments.

It'd be great if someone can verify the use of a subchart here is correct 😄

munnerz · 2018-09-10T17:02:16Z

Removed the WIP designation - if someone could take a look, I can then go ahead and release v0.5 on our end 😄

cpanato · 2018-09-10T17:04:29Z

stable/cert-manager/webhook/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+appVersion: "v0.5.0"
+description: A Helm chart for Kubernetes


maybe add a better description of what that is.

cpanato · 2018-09-10T17:04:56Z

stable/cert-manager/webhook/templates/ca-sync.yaml

+          restartPolicy: OnFailure
+          containers:
+          - name: ca-helper
+            image: quay.io/munnerz/apiextensions-ca-helper:canary


Is this image correct?

Updating to v0.1.0

Signed-off-by: James Munnelly <james@munnelly.eu>

munnerz · 2018-09-10T17:46:27Z

Addressed comments @cpanato

cpanato · 2018-09-11T07:04:50Z

@munnerz test failed because the image does not exist in quay

munnerz · 2018-09-12T09:55:20Z

I've now cut the v0.5.0 release, so docker images should be available now!

This should be good to go 😄

/retest
/hold cancel

munnerz · 2018-09-12T09:56:06Z

@mattfarina looks like the dco-labeler is not working - could you give it a kick? I am not too sure what events you have configured the labeler bot to listen for so not sure how to trigger it myself!

cpanato · 2018-09-13T10:37:22Z

/lgtm

k8s-ci-robot · 2018-09-13T10:37:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, munnerz

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~stable/cert-manager/OWNERS~~ [munnerz]

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

cpanato · 2018-09-13T10:38:04Z

/check-cla

cpanato · 2018-09-13T10:38:20Z

/check-dco

munnerz · 2018-09-13T10:39:41Z

Thanks @cpanato! I will remember the incantation for next time 😄

cpanato · 2018-09-13T10:41:58Z

/retest

cpanato · 2018-09-13T10:44:52Z

saw this error

Error: customresourcedefinitions.apiextensions.k8s.io "certificates.certmanager.k8s.io" already exists

should we provide a test input to change that? how was it done in the past?

@mattfarina

cpanato · 2018-09-13T10:47:07Z

@munnerz maybe you can add a test values.yaml to set the createCustomResource: true to false

munnerz · 2018-09-13T10:49:31Z

I think that'd work in this instance, but is surely a temporary workaround?

I see two reasons for that error:

cert-manager is being used in the test cluster for some other purpose already, meaning the CRD exists (seems unlikely)
a previous helm install half succeeded (i.e. created the CRD resource), but eventually failed, meaning helm delete was not run (thus leaving the CRD installed).

Perhaps I am wrong 😄 setting createCustomResource: false would work, until that test cluster is rebuilt and the CRD is removed.

I've not run into this problem before, which leads me to think this is caused by (2) 😄

cpanato · 2018-09-13T10:51:55Z

lets try a retest again 😄

/retest

cpanato · 2018-09-13T10:52:40Z

Then if the delete failed or did not run for any reason, we will need someone from the inside to make some cleanup

mattfarina · 2018-09-13T15:09:33Z

/retest

mattfarina · 2018-09-13T15:16:51Z

The issue is that the CRDs were not cleaned up, for some reason, in the test cluster on a previous run.

cpanato · 2018-09-13T15:25:35Z

@mattfarina thanks for fixing/cleaning

Antiarchitect · 2018-09-17T15:13:47Z

I have an issue that CRD's are not created at all.
Upgrading from 0.3.1 to 0.5.0 and having latest Tiller:

Client: &version.Version{SemVer:"v2.10.0", GitCommit:"9ad53aac42165a5fadc6c87be0dea6b115f93090", GitTreeState:"clean"}
Server: &version.Version{SemVer:"v2.10.0", GitCommit:"9ad53aac42165a5fadc6c87be0dea6b115f93090", GitTreeState:"clean"}

I have this while deploying:

Error: UPGRADE FAILED: unable to recognize "": no matches for kind "ClusterIssuer" in version "certmanager.k8s.io/v1alpha1"

@munnerz

* cert-manager: fast-forward to upstream bcffc635 * Update version numbers for v0.5.0 (cert-manager/cert-manager#885) * added affinity and tolerations (cert-manager/cert-manager#869) * Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478) * chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823) * helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769) Signed-off-by: James Munnelly <james@munnelly.eu> * Update image tag and add description Signed-off-by: James Munnelly <james@munnelly.eu> Signed-off-by: jenkin-x <jicowan@hotmail.com>

* cert-manager: fast-forward to upstream bcffc635 * Update version numbers for v0.5.0 (cert-manager/cert-manager#885) * added affinity and tolerations (cert-manager/cert-manager#869) * Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478) * chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823) * helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769) Signed-off-by: James Munnelly <james@munnelly.eu> * Update image tag and add description Signed-off-by: James Munnelly <james@munnelly.eu> Signed-off-by: Jakob Niggel <info@jakobniggel.de>

eldondevat · 2018-11-26T22:15:58Z

@Antiarchitect are you still encountering this bug, I believe I am, too. Did you ever find a workaround?

* cert-manager: fast-forward to upstream bcffc635 * Update version numbers for v0.5.0 (cert-manager/cert-manager#885) * added affinity and tolerations (cert-manager/cert-manager#869) * Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478) * chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823) * helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769) Signed-off-by: James Munnelly <james@munnelly.eu> * Update image tag and add description Signed-off-by: James Munnelly <james@munnelly.eu>

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2018

k8s-ci-robot requested a review from unguiculus September 10, 2018 14:30

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2018

munnerz commented Sep 10, 2018

View reviewed changes

munnerz changed the title ~~cert-manager: fast-forward to upstream 3945595b~~ WIP: cert-manager: fast-forward to upstream 3945595b Sep 10, 2018

munnerz force-pushed the cm-0.5-chart-draft branch from 9aaaecd to 07ffec7 Compare September 10, 2018 15:47

munnerz force-pushed the cm-0.5-chart-draft branch from 07ffec7 to e058fde Compare September 10, 2018 15:47

munnerz changed the title ~~WIP: cert-manager: fast-forward to upstream 3945595b~~ cert-manager: fast-forward to upstream 3945595b Sep 10, 2018

cpanato reviewed Sep 10, 2018

View reviewed changes

Update image tag and add description

265e0fd

Signed-off-by: James Munnelly <james@munnelly.eu>

munnerz force-pushed the cm-0.5-chart-draft branch from 51ca170 to 265e0fd Compare September 10, 2018 17:46

mattfarina added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 11, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2018

k8s-ci-robot assigned cpanato Sep 13, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2018

ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 13, 2018

k8s-ci-robot merged commit e0505bb into helm:master Sep 13, 2018

Conversation

munnerz commented Sep 10, 2018

Uh oh!

munnerz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpanato commented Sep 10, 2018

Uh oh!

munnerz commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpanato commented Sep 10, 2018

Uh oh!

munnerz commented Sep 10, 2018

Uh oh!

munnerz commented Sep 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpanato Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

munnerz commented Sep 10, 2018

Uh oh!

cpanato commented Sep 11, 2018

Uh oh!

munnerz commented Sep 12, 2018

Uh oh!

munnerz commented Sep 12, 2018

Uh oh!

cpanato commented Sep 13, 2018

Uh oh!

k8s-ci-robot commented Sep 13, 2018

Uh oh!

cpanato commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpanato commented Sep 13, 2018

Uh oh!

munnerz commented Sep 13, 2018

Uh oh!

cpanato commented Sep 13, 2018

Uh oh!

cpanato commented Sep 13, 2018

Uh oh!

cpanato commented Sep 13, 2018

Uh oh!

munnerz commented Sep 13, 2018

Uh oh!

cpanato commented Sep 13, 2018

Uh oh!

cpanato commented Sep 13, 2018

Uh oh!

mattfarina commented Sep 13, 2018

Uh oh!

mattfarina commented Sep 13, 2018

Uh oh!

cpanato commented Sep 13, 2018

Uh oh!

Antiarchitect commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

munnerz commented Sep 10, 2018 •

edited

Loading

cpanato Sep 10, 2018 •

edited

Loading

cpanato commented Sep 13, 2018 •

edited

Loading

Antiarchitect commented Sep 17, 2018 •

edited

Loading