Detect different versions of cert-manager #1546

ANeumann82 · 2020-06-03T12:27:25Z

Detect different versions of cert-manager and install the correct CRDs

Supports cert-manager 0.10.1 and 0.11.0+
kudo init --dry-run --output yaml uses a default with the newest cert-manager API
Removed the warning and check for a cert-manager deployment, as we not really depend on the exact version
Had to make the whole setup/installation stateful so we can hold the required cert-manager versions, but this may come in handy later if we expand the installation

Signed-off-by: Andreas Neumann aneumann@mesosphere.com

… ones Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog

Lgtm. I'd prefer a cert-manager group -> API mapping (make handling future changes easier)

zen-dog · 2020-06-03T13:57:29Z

pkg/kudoctl/cmd/init.go

@@ -154,10 +154,12 @@ func (initCmd *initCmd) run() error {
 		}
 	}

+	installer := setup.NewInstaller(opts, initCmd.crdOnly)


I still don't know why crdOnly is not part of the opts. But that's another story

zen-dog · 2020-06-03T14:02:17Z

pkg/kudoctl/kudoinit/prereq/webhook.go

 }

 const (
-	certManagerAPIVersion        = "v1alpha2"
-	certManagerControllerVersion = "v0.12.0"
+	certManagerOldGroup = "certmanager.k8s.io"


What about a group -> api mapping instead like?

certManagerGroupAPI := map[string]string{ "v1alpha1": "certmanager.k8s.io", "v1alpha2": "cert-manager.io", }

It's not necessarily a map... It might be a map of lists:

certManagerAPIs := map[string][]string{ "certmanager.k8s.io": []string{ "v1alpha1"}, "cert-manager.io": []string{ "v1alpha1", "v1alpha2", "v1alpha3" }, }

(at least I think "cert-manager.io" has "v1alpha1" as well in some version, it certainly has v1alpha2 and v1alpha3)

So even easier to check all permutatations 👍

nfnt

Awesome! 🚢

kensipe

This is incomplete and I would prefer some refactor for readability.

The incomplete element is:

webhook.goL225 has a "warning" that is still showing up.. which has multiple issues: first we should NOT print warning messages which use the wording "failed". second, if we successfully install for v1 (while preferring v2) it should NOT print any warnings... (unless verbose clog is > 0). If we are deprecating support and provide a warning that indicates that... that seems cool.

output of init

go run cmd/kubectl-kudo/main.go init --wait
$KUDO_HOME has been configured at /Users/kensipe/.kudo
Warnings                                                                                           
failed to get cert-manager deployment in namespace cert-manager. Make sure cert-manager is running.
✅ installed crds
✅ installed namespace
✅ installed service account
✅ installed webhook
✅ installed kudo controller
⌛Waiting for KUDO controller to be ready in your cluster...

The Warning section should not exist IMO.

Also... our docs for uninstall are: https://kudo.dev/docs/runbooks/admin/remove-kudo.html

our output for uninstall is:

 go run cmd/kubectl-kudo/main.go init --dry-run --output yaml | kubectl delete -f -
customresourcedefinition.apiextensions.k8s.io "operators.kudo.dev" deleted
customresourcedefinition.apiextensions.k8s.io "operatorversions.kudo.dev" deleted
customresourcedefinition.apiextensions.k8s.io "instances.kudo.dev" deleted
namespace "kudo-system" deleted
serviceaccount "kudo-manager" deleted
clusterrolebinding.rbac.authorization.k8s.io "kudo-manager-rolebinding" deleted
mutatingwebhookconfiguration.admissionregistration.k8s.io "kudo-manager-instance-admission-webhook-config" deleted
service "kudo-controller-manager-service" deleted
statefulset.apps "kudo-controller-manager" deleted
unable to recognize "STDIN": no matches for kind "Issuer" in version "cert-manager.io/v1alpha2"
unable to recognize "STDIN": no matches for kind "Certificate" in version "cert-manager.io/v1alpha2"

we should not try to output issuer and cert if they are not requested.... and/or we need doc updates for users to understand this message.

kensipe · 2020-06-03T19:30:52Z

pkg/kudoctl/kudoinit/prereq/webhook.go

+	}
+	if k.certManagerGroup != "" {
+		return nil
+	}


this is odd combo of funcs and state... this function needs intimate knowledge of the detectCertManagerCRD func... which is odd..

there is a lot of refactor here IMO...

detectCertManagerVersion seems like a very specific func name when it could be a generic kube func

we should prefer the return of values rather than the change of values passed by ref. It would be much easy to read and reason about IMO

it also seems strange that error handling is happening in 2 different ways... we have the return of an err and an adding of an error to the "result" which returns a nil. It isn't clear when one vs the other is appropriate.

kensipe · 2020-06-03T19:54:45Z

as I'm looking deeper at code... I don't understand why we do all the checks on the cert-man... if can detect we have a deployment... why check for a container. and then why check for health? This is a moment in time... just because it isn't healthy at this moment... doesn't mean it won't eventually be... and just because it is healthy in this moment... doesn't mean it will be when needed? this seems like a lot of extra and brittle code for questionable value.

Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe · 2020-06-03T21:14:36Z

pkg/kudoctl/kudoinit/prereq/webhook.go

+func (k *KudoWebHook) resourcesWithCertManager() []runtime.Object {
+	// We have to fall back to a default here as for a dry-run we can't detect the actual version of a cluster
+	k.issuer = issuer(k.opts.Namespace, certManagerNewGroup, certManagerAPIVersions[0])
+	k.certificate = certificate(k.opts.Namespace, certManagerNewGroup, certManagerAPIVersions[0])


these lines of code... can we get rid of them? Issuer and Certificate do NOT seem to be installed with --unsafe-self-signed-webhook-ca. it is unclear if they are ever used... other than --dry-run -o yaml. If we have a need to provide details around issuer and cert for this model of installation we should do it. If these are needed for a specific style of installation we need to talk about it.

These should be complete removed which will resolve the remaining blocking issue.

Well, they are not used when --unsafe-self-signed-webhook-ca is used. Issuer and Certificate are created when we don't use a self-signed-ca.

With this refactoring they are detected in the preInstallVerify step and the correct version of Issuer and Certificate for the installed cert-manager are created.

They are then applied in the Install step.

The problem with resources is that this step currently does not use the preInstallVerify step, and it usually doesn't even have a client initialized because it doesn't need an existing K8s cluster at all. Now that we need to detect the correct cert-manager version, we may have to do that. Maybe that's not a bad thing...

kensipe · 2020-06-03T21:19:33Z

I pushed a version of cert-man detection that works for konvoy and should work for any distro that provides the standard label of app=cert-manager for the certman deployment.

The remaining blocking issue is around the kinds Certification and Issuer. I guess I don't understand when and why they are used. I spent significant time around this today... it doesn't look like they are used if init on a cluster with a certman... nor are they used on a cluster with an --unsafe-self-signed-webhook-ca. It looks like they are only provided for dry-run with yaml output... why? It seems like we should remove them... I wanted to discuss first.

ANeumann82 · 2020-06-04T10:59:01Z

I pushed a version of cert-man detection that works for konvoy and should work for any distro that provides the standard label of app=cert-manager for the certman deployment.

Yeah, I like that. And I agree that we probably can get rid of the additional checks for cert-manager health, etc.

The remaining blocking issue is around the kinds Certification and Issuer. I guess I don't understand when and why they are used. I spent significant time around this today... it doesn't look like they are used if init on a cluster with a certman... nor are they used on a cluster with an --unsafe-self-signed-webhook-ca. It looks like they are only provided for dry-run with yaml output... why? It seems like we should remove them... I wanted to discuss first.

The cert is used here:

kudo/pkg/kudoctl/kudoinit/prereq/webhook.go

Line 248 in 2464231

    
           "cert-manager.io/inject-ca-from": fmt.Sprintf("%s/kudo-webhook-server-certificate", ns),

It's either this or the self-signed-webhook-ca...

Refactoring of webhook cert-manager detection Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

# Conflicts: # pkg/kudoctl/cmd/testdata/deploy-kudo-webhook.yaml.golden

…r error message when no cluster is reachable Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe

/lgtm

really nice work!

Detect different versions of cert-manager CRDs and create the correct…

5c0d76d

… ones Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 requested review from alenkacz, gerred, kensipe, nfnt and zen-dog as code owners June 3, 2020 12:27

Small cleanup and added comment

4c24894

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog approved these changes Jun 3, 2020

View reviewed changes

nfnt approved these changes Jun 3, 2020

View reviewed changes

kensipe requested changes Jun 3, 2020

View reviewed changes

getting the cert-man deployment by label

1ac91ca

Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe reviewed Jun 3, 2020

View reviewed changes

kensipe added the release/blocker label Jun 3, 2020

ANeumann82 and others added 6 commits June 4, 2020 14:01

Dry-Run does PreInstallVerify now to generate correct yaml

473fe25

Refactoring of webhook cert-manager detection Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Merge branch 'master' into an/allow-older-cert-manager

77840f5

# Conflicts: # pkg/kudoctl/cmd/testdata/deploy-kudo-webhook.yaml.golden

Add simple cluster version check in PreInstallVerify to provide bette…

a766e8a

…r error message when no cluster is reachable Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Removed test that is not used anymore

d4d2005

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed version for old certmanager group

45d5aa4

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

when output style is explicit, it must be honored or no print

654b921

Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe approved these changes Jun 4, 2020

View reviewed changes

kensipe merged commit f8d96bd into master Jun 4, 2020

kensipe deleted the an/allow-older-cert-manager branch June 4, 2020 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect different versions of cert-manager #1546

Detect different versions of cert-manager #1546

ANeumann82 commented Jun 3, 2020

zen-dog left a comment

zen-dog Jun 3, 2020

zen-dog Jun 3, 2020

ANeumann82 Jun 3, 2020

zen-dog Jun 3, 2020

nfnt left a comment

kensipe left a comment

kensipe Jun 3, 2020

kensipe commented Jun 3, 2020

kensipe Jun 3, 2020

ANeumann82 Jun 4, 2020

kensipe commented Jun 3, 2020

ANeumann82 commented Jun 4, 2020

kensipe left a comment

Detect different versions of cert-manager #1546

Detect different versions of cert-manager #1546

Conversation

ANeumann82 commented Jun 3, 2020

zen-dog left a comment

Choose a reason for hiding this comment

zen-dog Jun 3, 2020

Choose a reason for hiding this comment

zen-dog Jun 3, 2020

Choose a reason for hiding this comment

ANeumann82 Jun 3, 2020

Choose a reason for hiding this comment

zen-dog Jun 3, 2020

Choose a reason for hiding this comment

nfnt left a comment

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

kensipe Jun 3, 2020

Choose a reason for hiding this comment

kensipe commented Jun 3, 2020

kensipe Jun 3, 2020

Choose a reason for hiding this comment

ANeumann82 Jun 4, 2020

Choose a reason for hiding this comment

kensipe commented Jun 3, 2020

ANeumann82 commented Jun 4, 2020

kensipe left a comment

Choose a reason for hiding this comment