Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Detect different versions of cert-manager #1546

Merged
merged 9 commits into from Jun 4, 2020
Merged

Detect different versions of cert-manager #1546

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5c0d76d
Detect different versions of cert-manager CRDs and create the correct…
ANeumann82 Jun 3, 2020
4c24894
Small cleanup and added comment
ANeumann82 Jun 3, 2020
1ac91ca
getting the cert-man deployment by label
kensipe Jun 3, 2020
473fe25
Dry-Run does PreInstallVerify now to generate correct yaml
ANeumann82 Jun 4, 2020
77840f5
Merge branch 'master' into an/allow-older-cert-manager
ANeumann82 Jun 4, 2020
a766e8a
Add simple cluster version check in PreInstallVerify to provide bette…
ANeumann82 Jun 4, 2020
d4d2005
Removed test that is not used anymore
ANeumann82 Jun 4, 2020
45d5aa4
Fixed version for old certmanager group
ANeumann82 Jun 4, 2020
654b921
when output style is explicit, it must be honored or no print
kensipe Jun 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions pkg/kudoctl/cmd/init.go
View file
Open in desktop
Expand Up @@ -154,10 +154,12 @@ func (initCmd *initCmd) run() error {
}
}

installer := setup.NewInstaller(opts, initCmd.crdOnly)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


//TODO: implement output=yaml|json (define a type for output to constrain)
//define an Encoder to replace YAMLWriter
if strings.ToLower(initCmd.output) == "yaml" {
manifests, err := setup.AsYamlManifests(opts, initCmd.crdOnly)
manifests, err := installer.AsYamlManifests()
if err != nil {
return err
}
Expand Down Expand Up @@ -186,15 +188,15 @@ func (initCmd *initCmd) run() error {
}
initCmd.client = client
}
ok, err := initCmd.preInstallVerify(opts)
ok, err := initCmd.preInstallVerify(installer)
if err != nil {
return err
}
if !ok {
return fmt.Errorf("failed to verify installation requirements")
}

if err := setup.Install(initCmd.client, opts, initCmd.crdOnly); err != nil {
if err := installer.Install(initCmd.client); err != nil {
return clog.Errorf("error installing: %s", err)
}

Expand All @@ -211,9 +213,9 @@ func (initCmd *initCmd) run() error {
}

// preInstallVerify runs the pre-installation verification and returns true if the installation can continue
func (initCmd *initCmd) preInstallVerify(opts kudoinit.Options) (bool, error) {
func (initCmd *initCmd) preInstallVerify(v kudoinit.InstallVerifier) (bool, error) {
result := verifier.NewResult()
if err := setup.PreInstallVerify(initCmd.client, opts, initCmd.crdOnly, &result); err != nil {
if err := v.PreInstallVerify(initCmd.client, &result); err != nil {
return false, err
}
result.PrintWarnings(initCmd.out)
Expand Down
136 changes: 99 additions & 37 deletions pkg/kudoctl/kudoinit/prereq/webhook.go
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"strings"

"github.com/thoas/go-funk"
admissionv1beta1 "k8s.io/api/admissionregistration/v1beta1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
Expand All @@ -28,41 +29,43 @@ import (
var _ kudoinit.Step = &KudoWebHook{}

type KudoWebHook struct {
opts kudoinit.Options
issuer unstructured.Unstructured
certificate unstructured.Unstructured
opts kudoinit.Options

certManagerGroup string
certManagerAPIVersion string

issuer *unstructured.Unstructured
certificate *unstructured.Unstructured
}

const (
certManagerAPIVersion = "v1alpha2"
certManagerControllerVersion = "v0.12.0"
certManagerOldGroup = "certmanager.k8s.io"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a group -> api mapping instead like?

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So even easier to check all permutatations 👍

certManagerNewGroup = "cert-manager.io"
)

var (
certManagerControllerImageSuffix = fmt.Sprintf("cert-manager-controller:%s", certManagerControllerVersion)
certManagerAPIVersions = []string{"v1alpha2", "v1alpha1"}
)

func NewWebHookInitializer(options kudoinit.Options) KudoWebHook {
return KudoWebHook{
opts: options,
certificate: certificate(options.Namespace),
issuer: issuer(options.Namespace),
func NewWebHookInitializer(options kudoinit.Options) *KudoWebHook {
return &KudoWebHook{
opts: options,
}
}

func (k KudoWebHook) String() string {
func (k *KudoWebHook) String() string {
return "webhook"
}

func (k KudoWebHook) PreInstallVerify(client *kube.Client, result *verifier.Result) error {
func (k *KudoWebHook) PreInstallVerify(client *kube.Client, result *verifier.Result) error {
// skip verification if webhooks are not used or self-signed CA is used
if k.opts.SelfSignedWebhookCA {
return nil
}
return validateCertManagerInstallation(client, result)
return k.validateCertManagerInstallation(client, result)
}

func (k KudoWebHook) installWithCertManager(client *kube.Client) error {
func (k *KudoWebHook) installWithCertManager(client *kube.Client) error {
if err := installUnstructured(client.DynamicClient, k.issuer); err != nil {
return err
}
Expand All @@ -75,7 +78,7 @@ func (k KudoWebHook) installWithCertManager(client *kube.Client) error {
return nil
}

func (k KudoWebHook) installWithSelfSignedCA(client *kube.Client) error {
func (k *KudoWebHook) installWithSelfSignedCA(client *kube.Client) error {
iaw, s, err := k.resourcesWithSelfSignedCA()
if err != nil {
return nil
Expand All @@ -92,14 +95,14 @@ func (k KudoWebHook) installWithSelfSignedCA(client *kube.Client) error {
return nil
}

func (k KudoWebHook) Install(client *kube.Client) error {
func (k *KudoWebHook) Install(client *kube.Client) error {
if k.opts.SelfSignedWebhookCA {
return k.installWithSelfSignedCA(client)
}
return k.installWithCertManager(client)
}

func (k KudoWebHook) Resources() []runtime.Object {
func (k *KudoWebHook) Resources() []runtime.Object {
if k.opts.SelfSignedWebhookCA {
iaw, s, err := k.resourcesWithSelfSignedCA()
if err != nil {
Expand All @@ -112,15 +115,19 @@ func (k KudoWebHook) Resources() []runtime.Object {
return k.resourcesWithCertManager()
}

func (k KudoWebHook) resourcesWithCertManager() []runtime.Object {
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])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...


av := InstanceAdmissionWebhook(k.opts.Namespace)
objs := []runtime.Object{&av}
objs = append(objs, &k.issuer)
objs = append(objs, &k.certificate)
objs = append(objs, k.issuer)
objs = append(objs, k.certificate)
return objs
}

func (k KudoWebHook) resourcesWithSelfSignedCA() (*admissionv1beta1.MutatingWebhookConfiguration, *corev1.Secret, error) {
func (k *KudoWebHook) resourcesWithSelfSignedCA() (*admissionv1beta1.MutatingWebhookConfiguration, *corev1.Secret, error) {
tinyCA, err := NewTinyCA(kudoinit.DefaultServiceName, k.opts.Namespace)
if err != nil {
return nil, nil, fmt.Errorf("unable to set up webhook CA: %v", err)
Expand All @@ -142,11 +149,63 @@ func (k KudoWebHook) resourcesWithSelfSignedCA() (*admissionv1beta1.MutatingWebh
return &iaw, &ws, nil
}

func validateCertManagerInstallation(client *kube.Client, result *verifier.Result) error {
if err := validateCrdVersion(client.ExtClient, "certificates.cert-manager.io", certManagerAPIVersion, result); err != nil {
func (k *KudoWebHook) detectCertManagerVersion(client *kube.Client, result *verifier.Result) error {
extClient := client.ExtClient

// Cert-Manager 0.11.0+
if err := k.detectCertManagerCRD(extClient, certManagerNewGroup); err != nil {
return err
}
if k.certManagerGroup != "" {
return nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

  1. detectCertManagerVersion seems like a very specific func name when it could be a generic kube func
  2. 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
  3. 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.


// Cert-Manager 0.10.1
if err := k.detectCertManagerCRD(extClient, certManagerOldGroup); err != nil {
return err
}
if k.certManagerGroup != "" {
return nil
}
result.AddErrors(fmt.Sprintf("failed to detect any valid cert-manager CRDs. Make sure cert-manager is installed."))
return nil
}

func (k *KudoWebHook) detectCertManagerCRD(extClient clientset.Interface, group string) error {
testCRD := fmt.Sprintf("certificates.%s", group)
clog.V(4).Printf("Try to retrieve cert-manager CRD %s", testCRD)
crd, err := extClient.ApiextensionsV1().CustomResourceDefinitions().Get(testCRD, metav1.GetOptions{})
if err == nil {
clog.V(4).Printf("Got CRD. Group: %s, Version: %s", group, crd.Spec.Versions[0].Name)
k.certManagerGroup = group
k.certManagerAPIVersion = crd.Spec.Versions[0].Name
return nil
}
if !kerrors.IsNotFound(err) {
return fmt.Errorf("failed to detect cert manager CRD %s: %v", testCRD, err)
}
return nil
}

func (k *KudoWebHook) validateCertManagerInstallation(client *kube.Client, result *verifier.Result) error {
if err := k.detectCertManagerVersion(client, result); err != nil {
return err
}
if !result.IsValid() {
return nil
}
clog.V(4).Printf("Detected cert-manager CRDs %s/%s", k.certManagerGroup, k.certManagerAPIVersion)

if !funk.Contains(certManagerAPIVersions, k.certManagerAPIVersion) {
result.AddWarnings(fmt.Sprintf("Detected cert-manager CRDs with version %s, only versions %v are fully supported. Certificates for webhooks may not work.", k.certManagerAPIVersion, certManagerAPIVersions))
}

certificateCRD := fmt.Sprintf("certificates.%s", k.certManagerGroup)
if err := validateCrdVersion(client.ExtClient, certificateCRD, k.certManagerAPIVersion, result); err != nil {
return err
}
if err := validateCrdVersion(client.ExtClient, "issuers.cert-manager.io", certManagerAPIVersion, result); err != nil {
issuerCRD := fmt.Sprintf("issuers.%s", k.certManagerGroup)
if err := validateCrdVersion(client.ExtClient, issuerCRD, k.certManagerAPIVersion, result); err != nil {
return err
}

Expand All @@ -155,6 +214,11 @@ func validateCertManagerInstallation(client *kube.Client, result *verifier.Resul
return nil
}

// Initialize the custom resources that we're going to install
k.certificate = certificate(k.opts.Namespace, k.certManagerGroup, k.certManagerAPIVersion)
k.issuer = issuer(k.opts.Namespace, k.certManagerGroup, k.certManagerAPIVersion)

// A couple extra checks, these may fail because cert-manager can be installed in different namespaces
deployment, err := client.KubeClient.AppsV1().Deployments("cert-manager").Get("cert-manager", metav1.GetOptions{})
if err != nil {
if kerrors.IsNotFound(err) {
Expand All @@ -167,10 +231,6 @@ func validateCertManagerInstallation(client *kube.Client, result *verifier.Resul
result.AddWarnings("failed to validate cert-manager controller deployment. Spec had no containers")
return nil
}
if !strings.HasSuffix(deployment.Spec.Template.Spec.Containers[0].Image, certManagerControllerImageSuffix) {
result.AddWarnings(fmt.Sprintf("cert-manager deployment had unexpected version. expected %s in controller image name but found %s", certManagerControllerVersion, deployment.Spec.Template.Spec.Containers[0].Image))
return nil
}
if err := health.IsHealthy(deployment); err != nil {
result.AddWarnings("cert-manager seems not to be running correctly. Make sure cert-manager is working")
return nil
Expand All @@ -196,13 +256,13 @@ func validateCrdVersion(extClient clientset.Interface, crdName string, expectedV
}

// installUnstructured accepts kubernetes resource as unstructured.Unstructured and installs it into cluster
func installUnstructured(dynamicClient dynamic.Interface, item unstructured.Unstructured) error {
func installUnstructured(dynamicClient dynamic.Interface, item *unstructured.Unstructured) error {
gvk := item.GroupVersionKind()
_, err := dynamicClient.Resource(schema.GroupVersionResource{
Group: gvk.Group,
Version: gvk.Version,
Resource: fmt.Sprintf("%ss", strings.ToLower(gvk.Kind)), // since we know what kinds are we dealing with here, this is OK
}).Namespace(item.GetNamespace()).Create(&item, metav1.CreateOptions{})
}).Namespace(item.GetNamespace()).Create(item, metav1.CreateOptions{})
if kerrors.IsAlreadyExists(err) {
clog.V(4).Printf("resource %s already registered", item.GetName())
} else if err != nil {
Expand Down Expand Up @@ -281,10 +341,11 @@ func InstanceAdmissionWebhook(ns string) admissionv1beta1.MutatingWebhookConfigu
}
}

func issuer(ns string) unstructured.Unstructured {
return unstructured.Unstructured{
func issuer(ns string, group string, apiVersion string) *unstructured.Unstructured {
apiString := fmt.Sprintf("%s/%s", group, apiVersion)
return &unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "cert-manager.io/v1alpha2",
"apiVersion": apiString,
"kind": "Issuer",
"metadata": map[string]interface{}{
"name": "selfsigned-issuer",
Expand All @@ -297,10 +358,11 @@ func issuer(ns string) unstructured.Unstructured {
}
}

func certificate(ns string) unstructured.Unstructured {
return unstructured.Unstructured{
func certificate(ns string, group string, apiVersion string) *unstructured.Unstructured {
apiString := fmt.Sprintf("%s/%s", group, apiVersion)
return &unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "cert-manager.io/v1alpha2",
"apiVersion": apiString,
"kind": "Certificate",
"metadata": map[string]interface{}{
"name": "kudo-webhook-server-certificate",
Expand Down
36 changes: 32 additions & 4 deletions pkg/kudoctl/kudoinit/prereq/webhook_test.go
View file
Open in desktop
Expand Up @@ -35,8 +35,7 @@ func TestPrereq_Fail_PreValidate_Webhook_NoCertificate(t *testing.T) {
_ = init.PreInstallVerify(client, &result)

assert.EqualValues(t, verifier.NewError(
"failed to find CRD 'certificates.cert-manager.io': customresourcedefinitions.apiextensions.k8s.io \"certificates.cert-manager.io\" not found",
"failed to find CRD 'issuers.cert-manager.io': customresourcedefinitions.apiextensions.k8s.io \"issuers.cert-manager.io\" not found",
"failed to detect any valid cert-manager CRDs. Make sure cert-manager is installed.",
), result)
}

Expand All @@ -51,8 +50,23 @@ func TestPrereq_Fail_PreValidate_Webhook_WrongCertificateVersion(t *testing.T) {
result := verifier.NewResult()
_ = init.PreInstallVerify(client, &result)

assert.EqualValues(t, verifier.NewWarning(
"Detected cert-manager CRDs with version v0, only versions [v1alpha2 v1alpha1] are fully supported. Certificates for webhooks may not work.",
), result)
}

func TestPrereq_Fail_PreValidate_Webhook_WrongCertManagerInstallation(t *testing.T) {
client := getFakeClient()

mockCRD(client, "certificates.cert-manager.io", "v1alpha2")
mockCRD(client, "issuers.cert-manager.io", "v0")

init := NewWebHookInitializer(kudoinit.NewOptions("", "", "", false))

result := verifier.NewResult()
_ = init.PreInstallVerify(client, &result)

assert.EqualValues(t, verifier.NewError(
"invalid CRD version found for 'certificates.cert-manager.io': v0 instead of v1alpha2",
"invalid CRD version found for 'issuers.cert-manager.io': v0 instead of v1alpha2",
), result)
}
Expand Down Expand Up @@ -84,7 +98,7 @@ func TestPrereq_Fail_PreValidate_Webhook_WrongIssuerVersion(t *testing.T) {
assert.EqualValues(t, verifier.NewError("invalid CRD version found for 'issuers.cert-manager.io': v0 instead of v1alpha2"), result)
}

func TestPrereq_Ok_PreValidate_Webhook(t *testing.T) {
func TestPrereq_Ok_PreValidate_Webhook_CertManager_v1alpha2(t *testing.T) {
client := getFakeClient()

mockCRD(client, "certificates.cert-manager.io", "v1alpha2")
Expand All @@ -98,6 +112,20 @@ func TestPrereq_Ok_PreValidate_Webhook(t *testing.T) {
assert.Equal(t, 0, len(result.Errors))
}

func TestPrereq_Ok_PreValidate_Webhook_CertManager_v1alpha1(t *testing.T) {
client := getFakeClient()

mockCRD(client, "certificates.certmanager.k8s.io", "v1alpha1")
mockCRD(client, "issuers.certmanager.k8s.io", "v1alpha1")

init := NewWebHookInitializer(kudoinit.NewOptions("", "", "", false))

result := verifier.NewResult()
_ = init.PreInstallVerify(client, &result)

assert.Equal(t, 0, len(result.Errors))
}

func mockCRD(client *kube.Client, crdName string, apiVersion string) {
client.ExtClient.(*apiextensionsfake.Clientset).Fake.PrependReactor("get", "customresourcedefinitions", func(action testing2.Action) (handled bool, ret runtime.Object, err error) {

Expand Down