Skip to content

Commit

Permalink
Set owner reference to secrets created by webhook cert manager
Browse files Browse the repository at this point in the history 
When the certificate secret is created or updated, set an OwnerReference on the secret as the webhook-cert-manager deployment. This ensures that deletion of the deployment will also delete the secrets. This addresses the race condition bug that we sometimes see when re-installing consul on a cluster that had a consul deleted from it. This was because the helm delete would not delete the existing secrets with certificates. When the controller would get created with a new installation, it would mount the existing secret (which was stale) and the secret on disk would get rotated before the cert watcher started which would lead to the controller using certificates signed by a CA different from the CA bundle on the MWC which would lead to x509 errors.

This change would ensure the secrets get deleted every single time and hence, a new secret would always get created during a helm install. This also ensure an existing secret, when updated is updated with the owner ref ensuring helm upgrades or installs to a cluster with an existing secret give people the desired behavior as well.
  • Loading branch information
@thisisnotashwin
Ashwin Venkatesh authored and thisisnotashwin committed Jun 11, 2021
1 parent 8bcaee1 commit 61f66df
Show file tree
Hide file tree
Showing 3 changed files with 137 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
IMPROVEMENTS:
* Connect: skip service registration when a service with the same name but in a different Kubernetes namespace is found
and Consul namespaces are not enabled. [[GH-527](https://github.com/hashicorp/consul-k8s/pull/527)]
* Delete secrets created by webhook-cert-manager when the deployment is deleted. [[GH-530](https://github.com/hashicorp/consul-k8s/pull/530)]

BUG FIXES:
* CRDs: Update the type of connectTimeout and TTL in ServiceResolver and ServiceRouter from time.Duration to metav1.Duration.
Expand Down
40 changes: 40 additions & 0 deletions subcommand/webhook-cert-manager/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ type Command struct {
flagConfigFile string
flagLogLevel string

flagDeploymentName string
flagDeploymentNamespace string

clientset kubernetes.Interface

once sync.Once
Expand All @@ -59,6 +62,10 @@ func (c *Command) init() {
c.flagSet = flag.NewFlagSet("", flag.ContinueOnError)
c.flagSet.StringVar(&c.flagConfigFile, "config-file", "",
"Path to a config file to read webhook configs from. This file must be in JSON format.")
c.flagSet.StringVar(&c.flagDeploymentName, "deployment-name", "",
"Name of deployment that the cert-manager pod is managed by.")
c.flagSet.StringVar(&c.flagDeploymentNamespace, "deployment-namespace", "",
"Namespace of deployment that the cert-manager pod is managed by.")
c.flagSet.StringVar(&c.flagLogLevel, "log-level", "info",
"Log verbosity level. Supported values (in order of detail) are \"trace\", "+
"\"debug\", \"info\", \"warn\", and \"error\".")
Expand Down Expand Up @@ -92,6 +99,16 @@ func (c *Command) Run(args []string) int {
return 1
}

if c.flagDeploymentName == "" {
c.UI.Error(fmt.Sprintf("-deployment-name must be set"))
return 1
}

if c.flagDeploymentNamespace == "" {
c.UI.Error(fmt.Sprintf("-deployment-namespace must be set"))
return 1
}

// Create the Kubernetes clientset
if c.clientset == nil {
config, err := subcommand.K8SConfig(c.k8s.KubeConfig())
Expand Down Expand Up @@ -214,11 +231,24 @@ func (c *Command) certWatcher(ctx context.Context, ch <-chan cert.MetaBundle, cl
func (c *Command) reconcileCertificates(ctx context.Context, clientset kubernetes.Interface, bundle cert.MetaBundle, log hclog.Logger) error {
iterLog := log.With("mutatingwebhookconfig", bundle.WebhookConfigName, "secret", bundle.SecretName, "secretNS", bundle.SecretNamespace)

deployment, err := clientset.AppsV1().Deployments(c.flagDeploymentNamespace).Get(ctx, c.flagDeploymentName, metav1.GetOptions{})
if err != nil {
return err
}

certSecret, err := clientset.CoreV1().Secrets(bundle.SecretNamespace).Get(ctx, bundle.SecretName, metav1.GetOptions{})
if err != nil && k8serrors.IsNotFound(err) {
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: bundle.SecretName,
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: "apps/v1",
Kind: "Deployment",
Name: deployment.Name,
UID: deployment.UID,
},
},
},
Data: map[string][]byte{
corev1.TLSCertKey: bundle.Cert,
Expand Down Expand Up @@ -251,6 +281,16 @@ func (c *Command) reconcileCertificates(ctx context.Context, clientset kubernete

certSecret.Data[corev1.TLSCertKey] = bundle.Cert
certSecret.Data[corev1.TLSPrivateKeyKey] = bundle.Key
// Update the Owner Reference on an existing secret in case the secret
// already existed in the cluster and was not created by this job.
certSecret.OwnerReferences = []metav1.OwnerReference{
{
APIVersion: "apps/v1",
Kind: "Deployment",
Name: deployment.Name,
UID: deployment.UID,
},
}

iterLog.Info("Updating secret with new certificate")
_, err = clientset.CoreV1().Secrets(bundle.SecretNamespace).Update(ctx, certSecret, metav1.UpdateOptions{})
Expand Down
101 changes: 96 additions & 5 deletions subcommand/webhook-cert-manager/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ import (
"github.com/mitchellh/cli"
"github.com/stretchr/testify/require"
admissionv1beta1 "k8s.io/api/admissionregistration/v1beta1"
appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/fake"
)
Expand All @@ -26,12 +28,24 @@ func TestRun_ExitsCleanlyOnSignals(t *testing.T) {

func testSignalHandling(sig os.Signal) func(*testing.T) {
return func(t *testing.T) {
deploymentName := "deployment"
deploymentNamespace := "deploy-ns"
uid := types.UID("this-is-a-uid")

webhookConfigOneName := "webhookOne"
webhookConfigTwoName := "webhookTwo"

caBundleOne := []byte("bootstrapped-CA-one")
caBundleTwo := []byte("bootstrapped-CA-two")

deployment := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: deploymentName,
Namespace: deploymentNamespace,
UID: uid,
},
}

webhookOne := &admissionv1beta1.MutatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{
Name: webhookConfigOneName,
Expand Down Expand Up @@ -65,7 +79,7 @@ func testSignalHandling(sig os.Signal) func(*testing.T) {
},
}

k8s := fake.NewSimpleClientset(webhookOne, webhookTwo)
k8s := fake.NewSimpleClientset(webhookOne, webhookTwo, deployment)
ui := cli.NewMockUi()
cmd := Command{
UI: ui,
Expand All @@ -82,6 +96,8 @@ func testSignalHandling(sig os.Signal) func(*testing.T) {

exitCh := runCommandAsynchronously(&cmd, []string{
"-config-file", file.Name(),
"-deployment-name", deploymentName,
"-deployment-namespace", deploymentNamespace,
})
cmd.sendSignal(sig)

Expand All @@ -107,6 +123,14 @@ func TestRun_FlagValidation(t *testing.T) {
flags: nil,
expErr: "-config-file must be set",
},
{
flags: []string{"-config-file", "foo"},
expErr: "-deployment-name must be set",
},
{
flags: []string{"-config-file", "foo", "-deployment-name", "bar"},
expErr: "-deployment-namespace must be set",
},
}

for _, c := range cases {
Expand All @@ -124,6 +148,10 @@ func TestRun_FlagValidation(t *testing.T) {

func TestRun_SecretDoesNotExist(t *testing.T) {
t.Parallel()
deploymentName := "deployment"
deploymentNamespace := "deploy-ns"
uid := types.UID("this-is-a-uid")

secretOneName := "secret-deploy-1"
secretTwoName := "secret-deploy-2"

Expand All @@ -133,6 +161,14 @@ func TestRun_SecretDoesNotExist(t *testing.T) {
caBundleOne := []byte("bootstrapped-CA-one")
caBundleTwo := []byte("bootstrapped-CA-two")

deployment := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: deploymentName,
Namespace: deploymentNamespace,
UID: uid,
},
}

webhookOne := &admissionv1beta1.MutatingWebhookConfiguration{
ObjectMeta: metav1.ObjectMeta{
Name: webhookConfigOneName,
Expand Down Expand Up @@ -166,7 +202,7 @@ func TestRun_SecretDoesNotExist(t *testing.T) {
},
}

k8s := fake.NewSimpleClientset(webhookOne, webhookTwo)
k8s := fake.NewSimpleClientset(webhookOne, webhookTwo, deployment)
ui := cli.NewMockUi()
cmd := Command{
UI: ui,
Expand All @@ -183,6 +219,8 @@ func TestRun_SecretDoesNotExist(t *testing.T) {

exitCh := runCommandAsynchronously(&cmd, []string{
"-config-file", file.Name(),
"-deployment-name", deploymentName,
"-deployment-namespace", deploymentNamespace,
})
defer stopCommand(t, &cmd, exitCh)

Expand All @@ -192,10 +230,14 @@ func TestRun_SecretDoesNotExist(t *testing.T) {
secretOne, err := k8s.CoreV1().Secrets("default").Get(ctx, secretOneName, metav1.GetOptions{})
require.NoError(r, err)
require.Equal(r, secretOne.Type, v1.SecretTypeTLS)
require.Equal(r, deploymentName, secretOne.OwnerReferences[0].Name)
require.Equal(r, uid, secretOne.OwnerReferences[0].UID)

secretTwo, err := k8s.CoreV1().Secrets("default").Get(ctx, secretTwoName, metav1.GetOptions{})
require.NoError(r, err)
require.Equal(r, secretTwo.Type, v1.SecretTypeTLS)
require.Equal(r, deploymentName, secretTwo.OwnerReferences[0].Name)
require.Equal(r, uid, secretTwo.OwnerReferences[0].UID)

webhookConfigOne, err := k8s.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Get(ctx, webhookConfigOneName, metav1.GetOptions{})
require.NoError(r, err)
Expand All @@ -211,6 +253,10 @@ func TestRun_SecretDoesNotExist(t *testing.T) {

func TestRun_SecretExists(t *testing.T) {
t.Parallel()
deploymentName := "deployment"
deploymentNamespace := "deploy-ns"
uid := types.UID("this-is-a-uid")

secretOneName := "secret-deploy-1"
secretTwoName := "secret-deploy-2"

Expand All @@ -220,6 +266,14 @@ func TestRun_SecretExists(t *testing.T) {
caBundleOne := []byte("bootstrapped-CA-one")
caBundleTwo := []byte("bootstrapped-CA-two")

deployment := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: deploymentName,
Namespace: deploymentNamespace,
UID: uid,
},
}

secretOne := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretOneName,
Expand Down Expand Up @@ -274,7 +328,7 @@ func TestRun_SecretExists(t *testing.T) {
},
}

k8s := fake.NewSimpleClientset(webhookOne, webhookTwo, secretOne, secretTwo)
k8s := fake.NewSimpleClientset(webhookOne, webhookTwo, secretOne, secretTwo, deployment)
ui := cli.NewMockUi()
cmd := Command{
UI: ui,
Expand All @@ -291,6 +345,8 @@ func TestRun_SecretExists(t *testing.T) {

exitCh := runCommandAsynchronously(&cmd, []string{
"-config-file", file.Name(),
"-deployment-name", deploymentName,
"-deployment-namespace", deploymentNamespace,
})
defer stopCommand(t, &cmd, exitCh)

Expand All @@ -301,11 +357,15 @@ func TestRun_SecretExists(t *testing.T) {
require.NoError(r, err)
require.NotEqual(r, secretOne.Data[v1.TLSCertKey], []byte("cert-1"))
require.NotEqual(r, secretOne.Data[v1.TLSPrivateKeyKey], []byte("private-key-1"))
require.Equal(r, deploymentName, secretOne.OwnerReferences[0].Name)
require.Equal(r, uid, secretOne.OwnerReferences[0].UID)

secretTwo, err := k8s.CoreV1().Secrets("default").Get(ctx, secretTwoName, metav1.GetOptions{})
require.NoError(r, err)
require.NotEqual(r, secretTwo.Data[v1.TLSCertKey], []byte("cert-2"))
require.NotEqual(r, secretTwo.Data[v1.TLSPrivateKeyKey], []byte("private-key-2"))
require.Equal(r, deploymentName, secretTwo.OwnerReferences[0].Name)
require.Equal(r, uid, secretTwo.OwnerReferences[0].UID)

webhookConfigOne, err := k8s.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Get(ctx, webhookConfigOneName, metav1.GetOptions{})
require.NoError(r, err)
Expand All @@ -321,12 +381,24 @@ func TestRun_SecretExists(t *testing.T) {

func TestRun_SecretUpdates(t *testing.T) {
t.Parallel()
deploymentName := "deployment"
deploymentNamespace := "deploy-ns"
uid := types.UID("this-is-a-uid")

secretOne := "secret-deploy-1"

webhookConfigOne := "webhookOne"

caBundleOne := []byte("bootstrapped-CA-one")

deployment := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: deploymentName,
Namespace: deploymentNamespace,
UID: uid,
},
}

secret1 := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretOne,
Expand All @@ -352,7 +424,7 @@ func TestRun_SecretUpdates(t *testing.T) {
},
}

k8s := fake.NewSimpleClientset(webhookOne, secret1)
k8s := fake.NewSimpleClientset(webhookOne, secret1, deployment)
ui := cli.NewMockUi()
oneSec := 1 * time.Second

Expand All @@ -372,6 +444,8 @@ func TestRun_SecretUpdates(t *testing.T) {

exitCh := runCommandAsynchronously(&cmd, []string{
"-config-file", file.Name(),
"-deployment-name", deploymentName,
"-deployment-namespace", deploymentNamespace,
})
defer stopCommand(t, &cmd, exitCh)

Expand All @@ -385,6 +459,8 @@ func TestRun_SecretUpdates(t *testing.T) {
require.NoError(r, err)
require.NotEqual(r, secret1.Data[v1.TLSCertKey], []byte("cert-1"))
require.NotEqual(r, secret1.Data[v1.TLSPrivateKeyKey], []byte("private-key-1"))
require.Equal(r, deploymentName, secret1.OwnerReferences[0].Name)
require.Equal(r, uid, secret1.OwnerReferences[0].UID)

certificate = secret1.Data[v1.TLSCertKey]
key = secret1.Data[v1.TLSPrivateKeyKey]
Expand All @@ -404,6 +480,8 @@ func TestRun_SecretUpdates(t *testing.T) {
require.NoError(r, err)
require.NotEqual(r, secret1.Data[v1.TLSCertKey], certificate)
require.NotEqual(r, secret1.Data[v1.TLSPrivateKeyKey], key)
require.Equal(r, deploymentName, secret1.OwnerReferences[0].Name)
require.Equal(r, uid, secret1.OwnerReferences[0].UID)
})
}

Expand All @@ -425,9 +503,20 @@ func TestCertWatcher(t *testing.T) {
},
},
}

deploymentName := "deployment"
deploymentNamespace := "deploy-ns"
uid := types.UID("this-is-a-uid")
deployment := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: deploymentName,
Namespace: deploymentNamespace,
UID: uid,
},
}
certSource := &mocks.MockCertSource{}

k8s := fake.NewSimpleClientset(webhook)
k8s := fake.NewSimpleClientset(webhook, deployment)
ui := cli.NewMockUi()

cmd := Command{
Expand All @@ -446,6 +535,8 @@ func TestCertWatcher(t *testing.T) {

exitCh := runCommandAsynchronously(&cmd, []string{
"-config-file", file.Name(),
"-deployment-name", deploymentName,
"-deployment-namespace", deploymentNamespace,
})
defer stopCommand(t, &cmd, exitCh)

Expand Down

0 comments on commit 61f66df

Please sign in to comment.