🌱 clusterctl upgrade cert manager before upgrading providers #3364

fabriziopandini · 2020-07-17T13:25:34Z

What this PR does / why we need it:
This PR extends clusterctl upgrade in order to manage cert-manager upgrades before upgrading providers

This PR builds on top of #3313, which is not yet merged. In the meantime, please consider the latest commit only

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

/milestone v0.3.9

cmd/clusterctl/client/cluster/upgrader.go

cmd/clusterctl/client/cluster/cert_manager.go

wfernandes

Tested this out locally and the upgrade works.

A couple of things:

I think we should document that now clusterctl upgrade apply will also try upgrading cert-manager?
Should we have an entry in the clusterctl upgrade plan to check cert-manager versions as well? This need not be part of this PR.

cmd/clusterctl/client/cluster/cert_manager.go

cmd/clusterctl/client/cluster/cert_manager_test.go

wfernandes · 2020-08-10T20:56:06Z

cmd/clusterctl/client/cluster/cert_manager.go

+		case c < 0:
+			// if version < current, then upgrade
+			currentVersion = objVersion
+			needUpgrade = true


Could we break here? Then we may not need the if needUpgrade check below on line 281.
This way we have a consistent pattern that everywhere needUpgrade = true we break.

If we break here, we are breaking the switch statement while the goal of the statement on line 281 is to break the for loop

cmd/clusterctl/client/upgrade.go

fabriziopandini · 2020-08-11T10:18:00Z

@wfernandes thanks for the feedback

Should we have an entry in the clusterctl upgrade plan to check cert-manager versions as well? This need not be part of this PR.

This makes sense

wfernandes · 2020-08-11T15:24:40Z

We can open a separate issue to track this work.

Should we have an entry in the clusterctl upgrade plan to check cert-manager versions as well? This need not be part of this PR.

/lgtm

vincepri · 2020-08-18T15:03:40Z

Squash commits?

vincepri · 2020-08-18T15:04:08Z

cmd/clusterctl/client/cluster/cert_manager.go

+	// NOTE: // If the cert-manager.yaml asset is modified, this line **MUST** be updated
+	// accordingly else future upgrades of the cert-manager component will not
+	// be possible, as there'll be no record of the version installed.
+	embeddedCertManagerManifestVersion = "v0.16.0"


We should use v0.16.1 given that it was just released

If it is ok for you I will send a separate PR as soon as this one merge

I was going to suggest a separate PR because that way we can test out what updating the cert-manager manifest would look like.

vincepri · 2020-08-18T15:04:57Z

cmd/clusterctl/client/cluster/cert_manager.go

+	// NOTE: // If the cert-manager.yaml asset is modified, this line **MUST** be updated
+	// accordingly else future upgrades of the cert-manager component will not
+	// be possible, as there'll be no record of the version installed.
+	embeddedCertManagerManifestVersion = "v0.16.0"
+
+	// NOTE: The hash is used to ensure that when the cert-manager.yaml file is updated,
+	// the version number marker here is _also_ updated.
+	// You can either generate the SHA256 hash of the file, or alternatively
+	// run `go test` against this package. THe Test_VersionMarkerUpToDate will output
+	// the expected hash if it does not match the hash here.
+	embeddedCertManagerManifestHash = "5770f5f01c10a902355b3522b8ce44508ebb6ec88955efde9a443afe5b3969d7"


Should we be hardcoding versions and hashes within the codebase? Can we open an issue to get rid of this or find an alternative that doesn't require code changes?

This was inherited from #3313 and slightly re-worked here.
So far we have a unit test that ensures this value is in sync with the embedded manifest.

But happy to open an issue for another round of improvement

vincepri · 2020-08-18T15:06:19Z

/assign

fabriziopandini · 2020-08-19T10:04:29Z

squashed commits

k8s-ci-robot · 2020-08-19T10:06:39Z

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

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff	`6e0e62d`	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.

wfernandes · 2020-08-19T20:04:04Z

/lgtm

vincepri

/approve

k8s-ci-robot · 2020-08-19T20:15:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [vincepri]

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 17, 2020

k8s-ci-robot requested review from vincepri and wfernandes July 17, 2020 13:25

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

k8s-ci-robot added this to the v0.3.9 milestone Jul 31, 2020

vincepri mentioned this pull request Jul 31, 2020

[clusterctl] Manage the cert-manager lifecycle #2635

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2020

fabriziopandini force-pushed the clusterctl-upgrade-cert-manager branch from 4d6f723 to 2c1f290 Compare August 7, 2020 14:01

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 7, 2020

fabriziopandini force-pushed the clusterctl-upgrade-cert-manager branch 2 times, most recently from 23c76ce to cc648c5 Compare August 7, 2020 18:12

fabriziopandini commented Aug 7, 2020

View reviewed changes

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

fabriziopandini commented Aug 7, 2020

View reviewed changes

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

fabriziopandini commented Aug 7, 2020

View reviewed changes

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

fabriziopandini changed the title ~~[WIP] 🌱 clusterctl upgrade cert manager before upgrading providers~~ 🌱 clusterctl upgrade cert manager before upgrading providers Aug 7, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2020

wfernandes reviewed Aug 10, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2020

fabriziopandini force-pushed the clusterctl-upgrade-cert-manager branch from ecca906 to 89d13cd Compare August 11, 2020 10:11

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2020

k8s-ci-robot assigned wfernandes Aug 11, 2020

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

wfernandes mentioned this pull request Aug 17, 2020

Include cert-manager upgrade in clusterctl upgrade plan #3491

Closed

vincepri reviewed Aug 18, 2020

View reviewed changes

k8s-ci-robot assigned vincepri Aug 18, 2020

clusterctl upgrade cert-manager before providers

6e0e62d

fabriziopandini force-pushed the clusterctl-upgrade-cert-manager branch from 89d13cd to 6e0e62d Compare August 19, 2020 10:03

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

fabriziopandini mentioned this pull request Aug 19, 2020

Remove hard coded info about cert manager #3505

Closed

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

vincepri approved these changes Aug 19, 2020

View reviewed changes

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

k8s-ci-robot merged commit f8ef3fe into kubernetes-sigs:master Aug 19, 2020

fabriziopandini mentioned this pull request Aug 20, 2020

🌱 Start using cert-manager v0.16.1 #3511

Merged

fabriziopandini deleted the clusterctl-upgrade-cert-manager branch August 21, 2020 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 clusterctl upgrade cert manager before upgrading providers #3364

🌱 clusterctl upgrade cert manager before upgrading providers #3364

fabriziopandini commented Jul 17, 2020

vincepri commented Jul 31, 2020

wfernandes left a comment

wfernandes Aug 10, 2020

fabriziopandini Aug 11, 2020

fabriziopandini commented Aug 11, 2020

wfernandes commented Aug 11, 2020

vincepri commented Aug 18, 2020

vincepri Aug 18, 2020

fabriziopandini Aug 19, 2020

wfernandes Aug 19, 2020

vincepri Aug 18, 2020

fabriziopandini Aug 19, 2020

fabriziopandini Aug 19, 2020

vincepri commented Aug 18, 2020

fabriziopandini commented Aug 19, 2020

k8s-ci-robot commented Aug 19, 2020

wfernandes commented Aug 19, 2020

vincepri left a comment

k8s-ci-robot commented Aug 19, 2020

🌱 clusterctl upgrade cert manager before upgrading providers #3364

🌱 clusterctl upgrade cert manager before upgrading providers #3364

Conversation

fabriziopandini commented Jul 17, 2020

vincepri commented Jul 31, 2020

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

fabriziopandini commented Aug 11, 2020

wfernandes commented Aug 11, 2020

vincepri commented Aug 18, 2020

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

vincepri commented Aug 18, 2020

fabriziopandini commented Aug 19, 2020

k8s-ci-robot commented Aug 19, 2020

wfernandes commented Aug 19, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 19, 2020