✨ Upgrade cert-manager #3313

munnerz · 2020-07-08T17:47:46Z

What this PR does / why we need it:

Upgrades cert-manager to ~~v0.16.0-alpha.0~~ ~~v0.15.2~~ v0.16.0.

~~I can adjust this PR to install the latest stable version if preferred.~~ Done 🎉

The new 'wait for ready' logic should work more universally by actually attempting to create an Issuer and Certificate resource as-per the documentation we provide in the installation guide: https://cert-manager.io/docs/installation/kubernetes/#verifying-the-installation

Please note: This PR does not handle the upgrade process for cert-manager if a user already has it installed in their cluster (either through clusterctl init or through any other means).

Which issue(s) this PR fixes:
Fixes #2240

vincepri · 2020-07-08T17:52:44Z

/hold

To wait for v0.3.7 to be released first

munnerz · 2020-07-08T18:14:00Z

apidiff failing due to:

  Incompatible changes:
  - CertManagerClient.EnsureCertManager: added
  - CertManagerClient.EnsureWebhook: removed

I can keep the old method name around if preferred, and maybe introduce the new name as an extra function and deprecated the old one? EnsureWebhook isn't really very descriptive given the method actually installs cert-manager, not just the webhook :)

vincepri · 2020-07-08T18:15:23Z

The job is optional, I should have said something earlier. AFAIK these methods are only used within clusterctl, so it should be fine to change

vincepri · 2020-07-08T18:26:57Z

/milestone v0.3.x

wfernandes

Mostly looks good to me. Just some minor nits.
UPDATE: I also manually tested this using clusterctl init on a new kind cluster and everything was Running smoothly. Yay!
Thanks for this PR!

cmd/clusterctl/client/cluster/cert_manager.go

munnerz · 2020-07-09T07:19:37Z

cmd/clusterctl/client/cluster/cert_manager.go

-	// Nb. we are ignoring the error so this operation can support listing images even if there is no an existing management cluster;
-	// in case there is no an existing management cluster, we assume there is no web-hook installed in the cluster.
-	hasWebhook, _ := cm.hasWebhook()
-	if hasWebhook {


I'm not certain but this bit a code may still be required? If a user runs 'init' again on an existing cluster, what should this area of code do?

munnerz · 2020-07-09T07:20:42Z

cmd/clusterctl/client/cluster/cert_manager.go

 	log := logf.Log

-	// Checks if the cert-manager web-hook already exists, if yes, exit immediately


I suspect we still want this check too? Again, to avoid anything actually happening if 'clusterctl init' is run 😄

I've re-added this check with an additional function that calls Get on each resource in the cluster to verify whether cert-manager resources already exist before Creating them all again.

Now that I think some more though, it may be better to just call Create(), because currently if a single IsNotFound error is encountered, we go ahead and call Create anyway.

Whilst we may not want to address a proper upgrade process in this PR, because it is upgrading the manifest used, we probably want to implement some guards to avoid users downloading the new clusterctl and running init against an existing management cluster, as they could end out with orphaned resources from the previous release leftover.

I'm actually not 100% certain on how best to address this - updating the manifests in place without a defined upgrade process is tough, because we don't have any means to understand whether an older version is installed (unless we retain both copies of the cert-manager install manifests and explicitly check if all resources in an older version exist, but that gets.. slippery).

We do have this annotation on resources: helm.sh/chart: 'cert-manager-v0.15.2' but I'm not sure if relying on the cert-manager project to always set this in this format is wise. Perhaps we could annotate created objects (similar to what's done already), but with a version number too? For the 'transitionary period' for users coming from earlier clusterctl versions, we could check for the presence of the cert-manager value with no version annotation in the clusterctl.cluster.x-k8s.io/core annotation). But without recording exactly what resources are required to be installed in earlier versions of cert-manager, it won't be possible for us to be certain all resources exist (i.e. if a previous invocation of clusterctl init failed half way through, we'll struggle to recover).

cmd/clusterctl/client/cluster/cert_manager.go

fabriziopandini · 2020-07-09T10:12:15Z

cmd/clusterctl/client/cluster/cert_manager.go

+// Issuer and Certificate).
+// This ensures that the Kubernetes apiserver is ready to serve resources within the
+// cert-manager API group.
+func (cm *certManagerClient) waitForAPIReady(ctx context.Context, log logr.Logger) error {


Just for confirmation, in this func we are creating an issurer/a certificate, but we are not waiting for the certificate to be processed as documented https://cert-manager.io/docs/installation/kubernetes/#verifying-the-installation.
Is this enough for assuming cert-manager is working?

Another question, as far as I know cert-manager, this test can work both with v0.11.0 and v0.15.2

Is this enough for assuming cert-manager is working?

So it depends on how you want to handle this - you are correct, that it may not yet be ready to issue certificates as the controller could still be leader electing. However, presumably the parts of code that create the Certificate resource also then wait for those Certificates to be Ready before consuming the signed certificate/private key. Would you rather wait at the start for the controller to be ready, or wait until further into execution? Easy enough to change either way 😄

Another question, as far as I know cert-manager, this test can work both with v0.11.0 and v0.15.2

Correct, this is a far more universal test - if this stops working, it'll be because:

The v1alpha2 API version no longer exists in cert-manager

We made a big mistake in cert-manager 😅

fabriziopandini · 2020-07-09T10:17:48Z

cmd/clusterctl/client/cluster/cert_manager.go

-// This is required to check the web-hook is working and ready to accept requests.
-func (cm *certManagerClient) isWebhookAvailable(webhook *unstructured.Unstructured) (bool, error) {
-	conditions, found, err := unstructured.NestedSlice(webhook.Object, "status", "conditions")
+	testObjs, err := getTestResourcesManifestObjs()


Ideally, we should randomize the namespace name, but this can be addressed in a follow up as well

fabriziopandini · 2020-07-09T10:20:08Z

cmd/clusterctl/client/cluster/cert_manager.go


-	// installs the web-hook
+	// Install all cert-manager manifests


Question. Should we start to add a version label to each component so we can properly plan for cert-manager upgrades?

This definitely seems sensible - but we will need to store that version number somewhere, and remember to update it if the YAML asset changes. I wonder if we can have an automated test for this..? Maybe even just take a hash of the YAML file and have a variable: versionNumber which is a tuple of the version number and the hash of that file?

That way if you change the file, the hash will no longer match, forcing people to look at that area of code?

Perhaps you have other instances of this somewhere and have an existing pattern to solve it? 😄

Done in 4aa658b - keen to hear your comments on the technique for keeping the version number up to date.

I personally like the idea of the hash, so we can manage adjustment to the manifest without bumping a release.

cmd/clusterctl/config/assets/cert-manager.yaml

cmd/clusterctl/client/init.go

cmd/clusterctl/client/cluster/cert_manager.go

munnerz · 2020-07-09T13:48:39Z

Addressed your feedback - should be good for another look!

The only other thing I'm concerned about now is the limitations/requirements about label values - from what I can tell, every possible character we are using right now is valid/part of the character set permitted for labels. But it may be better to use annotations for some of this data if indexing/selecting based on these values is not required :)

munnerz · 2020-07-09T14:17:37Z

Seeing:

cmd/clusterctl/client/cluster/cert_manager_test.go:41:34: G401: Use of weak cryptographic primitive (gosec)
	actualHash := fmt.Sprintf("%x", md5.Sum(yaml))

Before we go ahead and merge this, do we want to stick with MD5 for this hashing? I don't think we are so concerned around the cryptographic properties of the hashing function, but it'll be a lot easier to get it right now than it would be to change it later 😄

vincepri · 2020-07-09T15:02:07Z

Let's just use sha256?

munnerz · 2020-07-09T15:30:28Z

Let's just use sha256?

Done ✅

vincepri · 2020-07-09T16:04:18Z

Seems like we might have to limit the hash to be in a label, IIRC usually can be limited to 10 chars

wfernandes

So I ran into the following problem.

Created kind cluster
clusterctl init on that cluster with older clusterctl (that is, clusterctl built from master branch).
Built clusterctl using this branch and ran clusterctl init.
Got the following output:

Using configuration File="/Users/wfernandes/.cluster-api/clusterctl.yaml"
Fetching providers
Waiting for cert-manager to be available...
Creating Namespace="cert-manager-test"
Failed to create cert-manager test resource Namespace="cert-manager-test"
Installing cert-manager
Creating Namespace="cert-manager"
Operation failed, retrying with backoff Cause="failed to create cert-manager component: /v1, Kind=Namespace, /cert-manager: Namespace \"cert-manager\" is
invalid: metadata.labels: Invalid value: \"d7a0492b0c1345023d79be75f475b3670c4109a305bdd85a36bb78d142b8febd\": must be no more than 63 characters"
...
Error: action failed after 10 attempts: failed to create cert-manager component: /v1, Kind=Namespace, /cert-manager: Namespace "cert-manager" is invalid:
metadata.labels: Invalid value: "d7a0492b0c1345023d79be75f475b3670c4109a305bdd85a36bb78d142b8febd": must be no more than 63 characters

Seem like there is a constraint on label length to be 63 characters in length.
https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set

UPDATE: LOL! Apologies...just noticed that the e2e's were failing for the same reason 🙂

cmd/clusterctl/client/cluster/cert_manager.go

wfernandes

Apologies for the multiple review comments.

cmd/clusterctl/client/cluster/cert_manager.go

wfernandes · 2020-07-09T17:56:25Z

cmd/clusterctl/client/cluster/cert_manager.go

+	// persist the version number of stored resources to make a
+	// future enhancement to add upgrade support possible.
+	labels["certmanager.clusterctl.cluster.x-k8s.io/version"] = embeddedCertManagerVersion()
+	labels["certmanager.clusterctl.cluster.x-k8s.io/hash"] = embeddedCertManagerHash()


I feel like we should have a test somewhere to ensure these are set if we are going to be depending on them in the future.

wfernandes · 2020-07-09T18:12:31Z

cmd/clusterctl/client/cluster/cert_manager.go

+		if err := cm.deleteObj(obj); err != nil {
+			log.V(5).Info("Failed to delete cert-manager test resource", logf.UnstructuredToValues(obj)...)
+			return err


I feel if there may be some interesting behavior here. If we delete the namespace cert-manager-test and then delete the certificate/issuer could we run into a chance of getting a NotFound error since deleting the namespace would delete the stuff inside it?

I haven't run into it while testing manually...but I'm wondering if we should ignore NotFound errors here. WDYT?

Might be we can simplify and simply delete the ns...

I've adjusted this to tolerate NotFound errors instead for now, given there is no specific namespace creation logic this seems simpler than having to inspect the manifests and infer what namespace to delete. This are can be improved as you've noted above to randomise the namespace name, which I think will make deletion a little simpler then too 😄

vincepri · 2020-07-15T13:59:53Z

Hey folks, now that we've released v0.3.7 I was wondering about the status of this PR, any particular updates or action items?

fabriziopandini · 2020-07-16T12:58:21Z

I'm +1 to rally and get this merged.
My comment are already addressed and I have a WIP PR ready for covering the upgrade of existing clusters as soon as this one merges.
But I see some comments from @wfernandes still to be addressed

wfernandes · 2020-07-16T20:07:47Z

If we are looking to get this merged in sooner rather than later, we can create separate issues and address some of the comments above. My comments aren't necessarily blockers.
However, we still need to fix the cause of the failing e2e test.

vincepri · 2020-07-31T15:23:46Z

/milestone v0.3.9

fabriziopandini · 2020-08-03T17:02:38Z

@munnerz is it ok for you trying to get this merged in this iteration?

vincepri · 2020-08-05T14:22:33Z

Thank you @munnerz for finishing this up!

Changes LGTM, can we squash commits?

/assign @fabriziopandini @wfernandes

vincepri · 2020-08-05T14:22:52Z

/approve
/hold
Feel free to unhold after squash

wfernandes · 2020-08-05T16:17:07Z

I'll do another pass on this now. Thanks @munnerz!

wfernandes

/lgtm
Waiting on commit squash.

wfernandes · 2020-08-05T17:02:33Z

cmd/clusterctl/client/cluster/cert_manager_test.go

 	"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/scheme"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/test"
 )

+func Test_VersionMarkerUpToDate(t *testing.T) {


nit: we can use g:=NewWithT(t) and use gomega matchers here.
This is not a blocker though.

fabriziopandini · 2020-08-06T09:49:31Z

Thanks @munnerz! looks great!
After squash, this is ready to go!!!

This also updates the installer to better test for API group readiness, as well as leaving version markers to make it possible to upgrade cert-manager in a future pull request.

munnerz · 2020-08-06T10:03:07Z

Done! Squashed and reworded the commit message to be summarise the changes.

k8s-ci-robot · 2020-08-06T10:04:48Z

@munnerz: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff	`19a37e6`	link	`/test pull-cluster-api-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

munnerz · 2020-08-06T10:06:04Z

For visibility on the apidiff failure, as discussed:

sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster
  Incompatible changes:
  - CertManagerClient.EnsureInstalled: added
  - CertManagerClient.EnsureWebhook: removed

fabriziopandini · 2020-08-06T11:21:56Z

/lgtm
/approve
Thanks!

k8s-ci-robot · 2020-08-06T11:22:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, vincepri

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:

~~cmd/clusterctl/OWNERS~~ [fabriziopandini,vincepri]

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

fabriziopandini · 2020-08-06T12:07:46Z

/hold cancel

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 8, 2020

k8s-ci-robot requested review from fabriziopandini and wfernandes July 8, 2020 17:47

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 8, 2020

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

k8s-ci-robot added this to the v0.3.x milestone Jul 8, 2020

wfernandes reviewed Jul 8, 2020

View reviewed changes

cmd/clusterctl/client/cluster/cert_manager.go Outdated Show resolved Hide resolved

cmd/clusterctl/client/cluster/cert_manager.go Outdated Show resolved Hide resolved

munnerz commented Jul 9, 2020

View reviewed changes

munnerz force-pushed the upgrade-cert-manager branch 2 times, most recently from 936562d to 465adf5 Compare July 9, 2020 09:15

fabriziopandini reviewed Jul 9, 2020

View reviewed changes

cmd/clusterctl/client/init.go Outdated Show resolved Hide resolved

cmd/clusterctl/client/cluster/cert_manager.go Show resolved Hide resolved

cmd/clusterctl/client/cluster/cert_manager.go Outdated Show resolved Hide resolved

wfernandes reviewed Jul 9, 2020

View reviewed changes

cmd/clusterctl/client/cluster/cert_manager.go Outdated Show resolved Hide resolved

wfernandes reviewed Jul 9, 2020

View reviewed changes

cmd/clusterctl/client/cluster/cert_manager.go Outdated Show resolved Hide resolved

wfernandes reviewed Jul 9, 2020

View reviewed changes

fabriziopandini mentioned this pull request Jul 16, 2020

clusterctl panics on init #3351

Closed

fabriziopandini mentioned this pull request Jul 17, 2020

🌱 clusterctl upgrade cert manager before upgrading providers #3364

Merged

k8s-ci-robot modified the milestones: v0.3.x, v0.3.9 Jul 31, 2020

vincepri mentioned this pull request Jul 31, 2020

[clusterctl] Manage the cert-manager lifecycle #2635

Closed

k8s-ci-robot assigned fabriziopandini and wfernandes Aug 5, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2020

wfernandes reviewed Aug 5, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2020

Upgrade cert-manager to v0.16.0

19a37e6

This also updates the installer to better test for API group readiness, as well as leaving version markers to make it possible to upgrade cert-manager in a future pull request.

munnerz force-pushed the upgrade-cert-manager branch from effa8da to 19a37e6 Compare August 6, 2020 10:02

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2020

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

k8s-ci-robot merged commit ad631d7 into kubernetes-sigs:master Aug 6, 2020

fabriziopandini mentioned this pull request Aug 6, 2020

🌱 Add hack to upgrade cert-manager in Tiltfile #3462

Merged

fabriziopandini mentioned this pull request Aug 19, 2020

Remove hard coded info about cert manager #3505

Closed

		log := logf.Log

		// Checks if the cert-manager web-hook already exists, if yes, exit immediately


		// installs the web-hook
		// Install all cert-manager manifests

✨ Upgrade cert-manager #3313

✨ Upgrade cert-manager #3313

Conversation

munnerz commented Jul 8, 2020 • edited Loading

vincepri commented Jul 8, 2020

munnerz commented Jul 8, 2020

vincepri commented Jul 8, 2020 • edited Loading

vincepri commented Jul 8, 2020

wfernandes 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

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

munnerz commented Jul 9, 2020

munnerz commented Jul 9, 2020

vincepri commented Jul 9, 2020

munnerz commented Jul 9, 2020

vincepri commented Jul 9, 2020 • edited Loading

wfernandes left a comment • edited Loading

Choose a reason for hiding this comment

wfernandes 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

vincepri commented Jul 15, 2020

fabriziopandini commented Jul 16, 2020

wfernandes commented Jul 16, 2020

vincepri commented Jul 31, 2020

fabriziopandini commented Aug 3, 2020

vincepri commented Aug 5, 2020

vincepri commented Aug 5, 2020

wfernandes commented Aug 5, 2020

wfernandes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Aug 6, 2020

munnerz commented Aug 6, 2020

k8s-ci-robot commented Aug 6, 2020

munnerz commented Aug 6, 2020

fabriziopandini commented Aug 6, 2020

k8s-ci-robot commented Aug 6, 2020

fabriziopandini commented Aug 6, 2020

munnerz commented Jul 8, 2020 •

edited

Loading

vincepri commented Jul 8, 2020 •

edited

Loading

wfernandes left a comment •

edited

Loading

vincepri commented Jul 9, 2020 •

edited

Loading

wfernandes left a comment •

edited

Loading