Merge pull request #837 from hashicorp/lkysow/cert-manager-fixes

Fix bug where webhookconfig would not be updated
hashicorp · Nov 4, 2021 · a08ddb0 · a08ddb0
2 parents 0b30795 + 1de9bca
commit a08ddb0
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,8 @@ BUG FIXES:
     then on subsequent re-runs of server-acl-init the tokens are never set. [[GH-825](https://github.com/hashicorp/consul-k8s/issues/825)]
   * ACLs: Fix issue where if the number of Consul servers is increased, the new servers are never provisioned
     an ACL token. [[GH-677](https://github.com/hashicorp/consul-k8s/issues/677)]
+  * Fix issue where after a `helm upgrade`, users would see `x509: certificate signed by unknown authority.`
+    errors when modifying config entry resources. [[GH-837](https://github.com/hashicorp/consul-k8s/pull/837)]
 
 ## 0.36.0 (November 02, 2021)
 

diff --git a/charts/consul/templates/controller-mutatingwebhookconfiguration.yaml b/charts/consul/templates/controller-mutatingwebhookconfiguration.yaml
@@ -12,7 +12,6 @@ metadata:
     component: controller
 webhooks:
 - clientConfig:
-    caBundle: Cg==
     service:
       name: {{ template "consul.fullname" . }}-controller-webhook
       namespace: {{ .Release.Namespace }}
@@ -34,7 +33,6 @@ webhooks:
       - proxydefaults
   sideEffects: None
 - clientConfig:
-    caBundle: Cg==
     service:
       name: {{ template "consul.fullname" . }}-controller-webhook
       namespace: {{ .Release.Namespace }}
@@ -56,7 +54,6 @@ webhooks:
       - meshes
   sideEffects: None
 - clientConfig:
-    caBundle: Cg==
     service:
       name: {{ template "consul.fullname" . }}-controller-webhook
       namespace: {{ .Release.Namespace }}
@@ -78,7 +75,6 @@ webhooks:
     - servicedefaults
   sideEffects: None
 - clientConfig:
-    caBundle: Cg==
     service:
       name: {{ template "consul.fullname" . }}-controller-webhook
       namespace: {{ .Release.Namespace }}
@@ -100,7 +96,6 @@ webhooks:
     - serviceresolvers
   sideEffects: None
 - clientConfig:
-    caBundle: Cg==
     service:
       name: {{ template "consul.fullname" . }}-controller-webhook
       namespace: {{ .Release.Namespace }}
@@ -122,7 +117,6 @@ webhooks:
     - servicerouters
   sideEffects: None
 - clientConfig:
-    caBundle: Cg==
     service:
       name: {{ template "consul.fullname" . }}-controller-webhook
       namespace: {{ .Release.Namespace }}
@@ -144,7 +138,6 @@ webhooks:
     - servicesplitters
   sideEffects: None
 - clientConfig:
-    caBundle: Cg==
     service:
       name: {{ template "consul.fullname" . }}-controller-webhook
       namespace: {{ .Release.Namespace }}
@@ -166,7 +159,6 @@ webhooks:
     - serviceintentions
   sideEffects: None
 - clientConfig:
-    caBundle: Cg==
     service:
       name: {{ template "consul.fullname" . }}-controller-webhook
       namespace: {{ .Release.Namespace }}
@@ -188,7 +180,6 @@ webhooks:
       - ingressgateways
   sideEffects: None
 - clientConfig:
-    caBundle: Cg==
     service:
       name: {{ template "consul.fullname" . }}-controller-webhook
       namespace: {{ .Release.Namespace }}

diff --git a/control-plane/subcommand/webhook-cert-manager/command.go b/control-plane/subcommand/webhook-cert-manager/command.go
@@ -157,8 +157,6 @@ func (c *Command) Run(args []string) int {
 		}
 	}
 
-	certCh := make(chan cert.MetaBundle)
-
 	// Create the certificate notifier so we can update certificates,
 	// then start all the background routines for updating certificates.
 	var notifiers []*cert.Notify
@@ -179,13 +177,14 @@ func (c *Command) Run(args []string) int {
 				Expiry: expiry,
 			}
 		}
+
+		certCh := make(chan cert.MetaBundle)
 		certNotify := &cert.Notify{Source: certSource, Ch: certCh, WebhookConfigName: config.Name, SecretName: config.SecretName, SecretNamespace: config.SecretNamespace}
 		notifiers = append(notifiers, certNotify)
 		go certNotify.Start(ctx)
+		go c.certWatcher(ctx, certCh, c.clientset, c.logger)
 	}
 
-	go c.certWatcher(ctx, certCh, c.clientset, c.logger)
-
 	// We define a signal handler for OS interrupts, and when an SIGINT or SIGTERM is received,
 	// we gracefully shut down, by first stopping our cert notifiers and then cancelling
 	// all the contexts that have been created by the process.

diff --git a/control-plane/subcommand/webhook-cert-manager/command_test.go b/control-plane/subcommand/webhook-cert-manager/command_test.go
@@ -8,6 +8,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/hashicorp/consul-k8s/control-plane/subcommand/common"
 	"github.com/hashicorp/consul-k8s/control-plane/subcommand/webhook-cert-manager/mocks"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/mitchellh/cli"
@@ -485,6 +486,105 @@ func TestRun_SecretUpdates(t *testing.T) {
 	})
 }
 
+// Test that when the MutatingWebhookConfiguration is modified, that we correctly
+// reset it to the expected CA bundle.
+func TestRun_WebhookConfigModified(t *testing.T) {
+	t.Parallel()
+
+	deploymentName := "deployment"
+	deploymentNamespace := "deploy-ns"
+	webhook1ConfigName := "webhookOne"
+	webhook2ConfigName := "webhookTwo"
+	caBundle1 := []byte("bootstrapped-CA1")
+	caBundle2 := []byte("bootstrapped-CA2")
+
+	deployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      deploymentName,
+			Namespace: deploymentNamespace,
+			UID:       types.UID("this-is-a-uid"),
+		},
+	}
+
+	initialWebhook1Config := &admissionv1.MutatingWebhookConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: webhook1ConfigName,
+		},
+		Webhooks: []admissionv1.MutatingWebhook{
+			{
+				Name: "webhook1-under-test",
+				ClientConfig: admissionv1.WebhookClientConfig{
+					CABundle: caBundle1,
+				},
+			},
+		},
+	}
+	initialWebhook2Config := &admissionv1.MutatingWebhookConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: webhook2ConfigName,
+		},
+		Webhooks: []admissionv1.MutatingWebhook{
+			{
+				Name: "webhook2-under-test",
+				ClientConfig: admissionv1.WebhookClientConfig{
+					CABundle: caBundle2,
+				},
+			},
+		},
+	}
+
+	// The k8s cluster will start with the two webhook configs and the deployment.
+	k8s := fake.NewSimpleClientset(initialWebhook1Config, initialWebhook2Config, deployment)
+	ctx := context.Background()
+
+	// We don't want the certs to expire. This test is only checking if
+	// the MutatingWebhookConfiguration is modified that it gets reset.
+	certExpiry := 1 * time.Hour
+
+	// Start the command.
+	cmd := Command{
+		UI:         cli.NewMockUi(),
+		clientset:  k8s,
+		certExpiry: &certExpiry,
+	}
+
+	configFile := common.WriteTempFile(t, configFile)
+	exitCh := runCommandAsynchronously(&cmd, []string{
+		"-config-file", configFile,
+		"-deployment-name", deploymentName,
+		"-deployment-namespace", deploymentNamespace,
+	})
+	defer stopCommand(t, &cmd, exitCh)
+
+	// First, check that the mutatingwebhookconfiguration contents are updated when the cert-manager starts.
+	timer := &retry.Timer{Timeout: 10 * time.Second, Wait: 500 * time.Millisecond}
+	retry.RunWith(timer, t, func(r *retry.R) {
+		webhookConfig1, err := k8s.AdmissionregistrationV1().MutatingWebhookConfigurations().Get(ctx, webhook1ConfigName, metav1.GetOptions{})
+		require.NoError(r, err)
+		require.NotEqual(r, webhookConfig1.Webhooks[0].ClientConfig.CABundle, caBundle1)
+
+		webhookConfig2, err := k8s.AdmissionregistrationV1().MutatingWebhookConfigurations().Get(ctx, webhook2ConfigName, metav1.GetOptions{})
+		require.NoError(r, err)
+		require.NotEqual(r, webhookConfig2.Webhooks[0].ClientConfig.CABundle, caBundle2)
+	})
+
+	// Now, edit the mutatingwebhookconfigurations and reset the caBundle fields.
+	k8s.AdmissionregistrationV1().MutatingWebhookConfigurations().Update(ctx, initialWebhook1Config, metav1.UpdateOptions{})
+	k8s.AdmissionregistrationV1().MutatingWebhookConfigurations().Update(ctx, initialWebhook2Config, metav1.UpdateOptions{})
+
+	// Check that both mutatingwebhookconfigurations have their caBundle fields reset.
+	timer = &retry.Timer{Timeout: 10 * time.Second, Wait: 500 * time.Millisecond}
+	retry.RunWith(timer, t, func(r *retry.R) {
+		webhookConfig1, err := k8s.AdmissionregistrationV1().MutatingWebhookConfigurations().Get(ctx, webhook1ConfigName, metav1.GetOptions{})
+		require.NoError(r, err)
+		require.NotEqual(r, webhookConfig1.Webhooks[0].ClientConfig.CABundle, caBundle1)
+
+		webhookConfig2, err := k8s.AdmissionregistrationV1().MutatingWebhookConfigurations().Get(ctx, webhook2ConfigName, metav1.GetOptions{})
+		require.NoError(r, err)
+		require.NotEqual(r, webhookConfig2.Webhooks[0].ClientConfig.CABundle, caBundle2)
+	})
+}
+
 // This test verifies that when there is an error while attempting to update
 // the certs or the webhook config, it retries the update every second until
 // it succeeds.