Allow cert-manager to be provisioned externally #11354

codablock · 2021-04-29T12:41:07Z

This PR allows provisioning of cert-manager externally. It actually leads to a no-op, but makes checks (IsCertManagerEnabled) succeed so that other dependent addons are deployable.

k8s-ci-robot · 2021-04-29T12:41:15Z

Hi @codablock. Thanks for your PR.

I'm waiting for a kubernetes 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.

olemarkus · 2021-04-29T12:48:46Z

pkg/apis/kops/validation/validation.go

+	if fi.BoolValue(spec.Enabled) && fi.BoolValue(spec.External) {
+		allErrs = append(allErrs, field.Forbidden(fldPath, "Cert manager must be either enabled or external but not both"))
+	}
+	if fi.BoolValue(spec.External) && fi.StringValue(spec.Image) != "" {


This seems a bit overkill. I think we can just document that when external is set, any other flag is ignored.

Removed the last 2 checks.

olemarkus · 2021-04-29T12:49:37Z

pkg/apis/kops/componentconfig.go

@@ -930,6 +930,10 @@ type CertManagerConfig struct {
 	// Default: false
 	Enabled *bool `json:"enabled,omitempty"`

+	// External Tell kops that cert manager is deployed externally.
+	// Default: false
+	External *bool `json:"external,omitempty"`


Let me check if this naming is consistent with other structs where we allow this.

I think some other time we discussed about "Managed".

you mean setting "managed" to false?

Example:

kops/pkg/apis/kops/ntpconfig.go

Line 23 in ba3914a

Managed *bool `json:"managed,omitempty"`

Changed the PR to do it the same way

olemarkus · 2021-04-29T12:50:11Z

/ok-to-test

olemarkus · 2021-04-30T12:51:01Z

docs/addons.md

+either remove this installation prior to enabling this addon, or mark cert-manger as not being managed by kOps (see below).
+As long as you are using v1 versions of the cert-manager resources, it is safe to remove existing installs and replace it with this addon**
+
+The following cert-manager configuration allows provisioning cert-manager externally and allows all dependent plugins


Can you add the version macro?
{{ kops_feature_table(kops_added_default='1.21', k8s_min='1.16') }}

I was not sure where to add this, I hope I got it right

Usually we just add a subheading and place the macro under.

Added another commit. I'm still unsure if this is how you expect it, if not, can you show me how you expect it?

Looks perfect

olemarkus

Can you squash? then this is ready to go.

olemarkus · 2021-04-30T17:00:25Z

docs/addons.md

+either remove this installation prior to enabling this addon, or mark cert-manger as not being managed by kOps (see below).
+As long as you are using v1 versions of the cert-manager resources, it is safe to remove existing installs and replace it with this addon**
+
+The following cert-manager configuration allows provisioning cert-manager externally and allows all dependent plugins


Looks perfect

codablock · 2021-04-30T18:34:18Z

Squashed

olemarkus

Much obliged

k8s-ci-robot · 2021-04-30T19:17:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus

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

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

kellycampbell · 2021-05-19T22:47:57Z

Could this be cherry-picked back to 1.20? We're running into an error trying to upgrade from 1.19 to 1.20 due to metricsServer being enabled, but now certManager requirement is checked. We already have cert manager in the cluster and want to take more time to make sure all our certs are going to port over if we switch to the add-on. (I'm working around this for now by disabling metricsServer since this on a dev cluster.)

olemarkus · 2021-05-20T06:25:24Z

For metrics-server you can set insecure: true, which will disable the cert manager check.

I don't think we'll be doing a 1.20 release before 1.21 anyway.

…-upstream-release-1.20 Automated cherry pick of #11354: Allow cert-manager to be provisioned externally

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 29, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/api labels Apr 29, 2021

k8s-ci-robot requested review from olemarkus and zetaab April 29, 2021 12:41

k8s-ci-robot added the area/documentation label Apr 29, 2021

olemarkus requested changes Apr 29, 2021

View reviewed changes

k8s-ci-robot assigned olemarkus Apr 29, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 29, 2021

codablock force-pushed the external-cert-manager branch 2 times, most recently from 7d7e13f to 4ad50f0 Compare April 30, 2021 10:25

olemarkus reviewed Apr 30, 2021

View reviewed changes

olemarkus requested changes Apr 30, 2021

View reviewed changes

Allow cert-manager to be provisioned externally

d1ab0af

codablock force-pushed the external-cert-manager branch from 02a31c3 to d1ab0af Compare April 30, 2021 18:34

olemarkus approved these changes Apr 30, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2021

k8s-ci-robot merged commit 3704ffd into kubernetes:master Apr 30, 2021

k8s-ci-robot added this to the v1.21 milestone Apr 30, 2021

codablock deleted the external-cert-manager branch May 1, 2021 11:38

hakman mentioned this pull request May 21, 2021

Automated cherry pick of #11354: Allow cert-manager to be provisioned externally #11548

Merged

k8s-ci-robot added a commit that referenced this pull request May 21, 2021

Merge pull request #11548 from hakman/automated-cherry-pick-of-#11354…

f4f4f9a

…-upstream-release-1.20 Automated cherry pick of #11354: Allow cert-manager to be provisioned externally

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow cert-manager to be provisioned externally #11354

Allow cert-manager to be provisioned externally #11354

codablock commented Apr 29, 2021

k8s-ci-robot commented Apr 29, 2021

olemarkus Apr 29, 2021

codablock Apr 29, 2021

olemarkus Apr 29, 2021

hakman Apr 29, 2021

codablock Apr 29, 2021

hakman Apr 29, 2021

codablock Apr 30, 2021

olemarkus commented Apr 29, 2021

olemarkus Apr 30, 2021

codablock Apr 30, 2021

olemarkus Apr 30, 2021

codablock Apr 30, 2021

olemarkus Apr 30, 2021

olemarkus left a comment

olemarkus Apr 30, 2021

codablock commented Apr 30, 2021

olemarkus left a comment

k8s-ci-robot commented Apr 30, 2021

kellycampbell commented May 19, 2021

olemarkus commented May 20, 2021

Allow cert-manager to be provisioned externally #11354

Allow cert-manager to be provisioned externally #11354

Conversation

codablock commented Apr 29, 2021

k8s-ci-robot commented Apr 29, 2021

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

olemarkus commented Apr 29, 2021

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

olemarkus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codablock commented Apr 30, 2021

olemarkus left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 30, 2021

kellycampbell commented May 19, 2021

olemarkus commented May 20, 2021