feat: add validating webhook for Keptn Metrics (#668)

Signed-off-by: realanna <anna.reale@dynatrace.com>
Signed-off-by: RealAnna <89971034+RealAnna@users.noreply.github.com>
Co-authored-by: odubajDT <93584209+odubajDT@users.noreply.github.com>

klt-cert-manager/config/rbac/role.yaml

@@ -15,6 +15,16 @@ rules:
   - patch
   - update
   - watch
+- apiGroups:
+  - admissionregistration.k8s.io
+  resources:
+  - validatingwebhookconfigurations
+  verbs:
+  - get
+  - list
+  - patch
+  - update
+  - watch
 - apiGroups:
   - apiextensions.k8s.io
   resources:

klt-cert-manager/controllers/keptnwebhookcontroller/config.go

@@ -11,7 +11,8 @@ const (
 	DeploymentName             = "klc-controller-manager"
 	ServiceName                = "klc-webhook-service"
 	SuccessDuration            = 3 * time.Hour
-	Webhookconfig              = "klc-mutating-webhook-configuration"
+	MutatingWebhookconfig      = "klc-mutating-webhook-configuration"
+	ValidatingWebhookconfig    = "klc-validating-webhook-configuration"
 	secretPostfix              = "-certs"
 	crdGroup                   = "lifecycle.keptn.sh"
 	certificatesSecretEmptyErr = "certificates secret is empty"

klt-cert-manager/controllers/keptnwebhookcontroller/keptnwebhookcertificate_controller.go

@@ -29,6 +29,7 @@ type KeptnWebhookCertificateReconciler struct {
 }
 
 //clusterrole
+// +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=get;list;watch;update;patch;
 // +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=mutatingwebhookconfigurations,verbs=get;list;watch;update;patch;
 // +kubebuilder:rbac:groups="apiextensions.k8s.io",resources=customresourcedefinitions,verbs=get;list;watch;update;patch;
 // +kubebuilder:rbac:groups="apps",resources=deployments,verbs=get;list;watch;
@@ -51,6 +52,11 @@ func (r *KeptnWebhookCertificateReconciler) Reconcile(ctx context.Context, reque
 		r.Log.Error(err, "could not find mutating webhook configuration")
 	}
 
+	validatingWebhookConfiguration, err := r.getValidatingWebhookConfiguration()
+	if err != nil {
+		r.Log.Error(err, "could not find validating webhook configuration")
+	}
+
 	crds := &apiv1.CustomResourceDefinitionList{}
 	crds, err = r.getCRDConfigurations()
 	if err != nil {
@@ -73,11 +79,14 @@ func (r *KeptnWebhookCertificateReconciler) Reconcile(ctx context.Context, reque
 
 	mutatingWebhookConfigs := getClientConfigsFromMutatingWebhook(mutatingWebhookConfiguration)
 
+	validatingWebhookConfigs := getClientConfigsFromValidatingWebhook(validatingWebhookConfiguration)
+
 	areMutatingWebhookConfigsValid := certSecret.areWebhookConfigsValid(mutatingWebhookConfigs)
+	areValidatingWebhookConfigsValid := certSecret.areWebhookConfigsValid(validatingWebhookConfigs)
 	areCRDConversionsConfigValid := certSecret.areCRDConversionsValid(crds)
 	isCertSecretRecent := certSecret.isRecent()
 
-	if isCertSecretRecent && areMutatingWebhookConfigsValid && areCRDConversionsConfigValid {
+	if isCertSecretRecent && areMutatingWebhookConfigsValid && areValidatingWebhookConfigsValid && areCRDConversionsConfigValid {
 		r.Log.Info("secret for certificates up to date, skipping update")
 		r.cancelMgr()
 		return reconcile.Result{RequeueAfter: SuccessDuration}, nil
@@ -96,6 +105,10 @@ func (r *KeptnWebhookCertificateReconciler) Reconcile(ctx context.Context, reque
 		return reconcile.Result{}, errors.WithStack(err)
 	}
 
+	if err := r.updateClientConfigurations(bundle, validatingWebhookConfigs, validatingWebhookConfiguration); err != nil {
+		return reconcile.Result{}, errors.WithStack(err)
+	}
+
 	if err = r.updateCRDsConfiguration(crds, bundle); err != nil {
 		return reconcile.Result{}, errors.WithStack(err)
 	}
@@ -125,7 +138,7 @@ func (r *KeptnWebhookCertificateReconciler) getMutatingWebhookConfiguration() (
 	*admissionregistrationv1.MutatingWebhookConfiguration, error) {
 	var mutatingWebhook admissionregistrationv1.MutatingWebhookConfiguration
 	if err := r.Client.Get(r.ctx, client.ObjectKey{
-		Name: Webhookconfig,
+		Name: MutatingWebhookconfig,
 	}, &mutatingWebhook); err != nil {
 		return nil, err
 	}
@@ -136,6 +149,21 @@ func (r *KeptnWebhookCertificateReconciler) getMutatingWebhookConfiguration() (
 	return &mutatingWebhook, nil
 }
 
+func (r *KeptnWebhookCertificateReconciler) getValidatingWebhookConfiguration() (
+	*admissionregistrationv1.ValidatingWebhookConfiguration, error) {
+	var validatingWebhook admissionregistrationv1.ValidatingWebhookConfiguration
+	if err := r.Client.Get(r.ctx, client.ObjectKey{
+		Name: ValidatingWebhookconfig,
+	}, &validatingWebhook); err != nil {
+		return nil, err
+	}
+
+	if len(validatingWebhook.Webhooks) <= 0 {
+		return nil, errors.New("validating webhook configuration has no registered webhooks")
+	}
+	return &validatingWebhook, nil
+}
+
 func (r *KeptnWebhookCertificateReconciler) updateClientConfigurations(bundle []byte,
 	webhookClientConfigs []*admissionregistrationv1.WebhookClientConfig, webhookConfig client.Object) error {
 	if webhookConfig == nil || reflect.ValueOf(webhookConfig).IsNil() {

klt-cert-manager/controllers/keptnwebhookcontroller/webhook_cert_controller_test.go

@@ -147,7 +147,7 @@ func TestReconcile(t *testing.T) {
 	t.Run(`reconcile successfully with mutatingwebhookconfiguration`, func(t *testing.T) {
 		fakeClient := fake.NewClient(crd1, crd2, crd3, &admissionregistrationv1.MutatingWebhookConfiguration{
 			ObjectMeta: metav1.ObjectMeta{
-				Name: Webhookconfig,
+				Name: MutatingWebhookconfig,
 			},
 			Webhooks: []admissionregistrationv1.MutatingWebhook{
 				{
@@ -165,6 +165,27 @@ func TestReconcile(t *testing.T) {
 		assert.NoError(t, err)
 		assert.NotNil(t, result)
 	})
+	t.Run(`reconcile successfully with validatingwebhookconfiguration`, func(t *testing.T) {
+		fakeClient := fake.NewClient(crd1, crd2, crd3, &admissionregistrationv1.ValidatingWebhookConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: ValidatingWebhookconfig,
+			},
+			Webhooks: []admissionregistrationv1.ValidatingWebhook{
+				{
+					ClientConfig: admissionregistrationv1.WebhookClientConfig{},
+				},
+				{
+					ClientConfig: admissionregistrationv1.WebhookClientConfig{},
+				},
+			},
+		})
+
+		controller, request := prepareController(t, fakeClient)
+		result, err := controller.Reconcile(context.TODO(), request)
+
+		assert.NoError(t, err)
+		assert.NotNil(t, result)
+	})
 	t.Run(`update crd successfully with up-to-date secret`, func(t *testing.T) {
 		fakeClient := fake.NewClient(crd1, crd2, crd3)
 		cs := newCertificateSecret(fakeClient)
@@ -196,8 +217,65 @@ func TestReconcile(t *testing.T) {
 		require.NoError(t, err)
 		assert.Empty(t, actualCrd.Spec.Conversion.Webhook)
 	})
+
+	t.Run(`update crd and webhooks successfully with up-to-date secret`, func(t *testing.T) {
+		fakeClient := fake.NewClient(crd1, crd2, crd3, &admissionregistrationv1.MutatingWebhookConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: MutatingWebhookconfig,
+			},
+			Webhooks: []admissionregistrationv1.MutatingWebhook{
+				{
+					ClientConfig: admissionregistrationv1.WebhookClientConfig{},
+				},
+				{
+					ClientConfig: admissionregistrationv1.WebhookClientConfig{},
+				},
+			},
+		}, &admissionregistrationv1.ValidatingWebhookConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: ValidatingWebhookconfig,
+			},
+			Webhooks: []admissionregistrationv1.ValidatingWebhook{
+				{
+					ClientConfig: admissionregistrationv1.WebhookClientConfig{},
+				},
+				{
+					ClientConfig: admissionregistrationv1.WebhookClientConfig{},
+				},
+			},
+		})
+		cs := newCertificateSecret(fakeClient)
+		_ = cs.setSecretFromReader(context.TODO(), namespace, testr.New(t))
+		_ = cs.setCertificates(namespace)
+		_ = cs.createOrUpdateIfNecessary(context.TODO())
+
+		controller, request := prepareController(t, fakeClient)
+		result, err := controller.Reconcile(context.TODO(), request)
+		require.NoError(t, err)
+		assert.NotNil(t, result)
+
+		expectedBundle, err := cs.loadCombinedBundle()
+		require.NoError(t, err)
+		actualCrd := &apiv1.CustomResourceDefinition{}
+
+		// crd1 should get a new secret
+		err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: crd1.Name}, actualCrd)
+		require.NoError(t, err)
+		assert.Equal(t, expectedBundle, actualCrd.Spec.Conversion.Webhook.ClientConfig.CABundle)
+
+		// crd2 is not a keptn resource and should not be touched
+		err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: crd2.Name}, actualCrd)
+		require.NoError(t, err)
+		assert.Equal(t, crd2.Spec.Conversion.Webhook.ClientConfig.CABundle, actualCrd.Spec.Conversion.Webhook.ClientConfig.CABundle)
+
+		// crd 3 should not have a webhook conversion
+		err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: crd3.Name}, actualCrd)
+		require.NoError(t, err)
+		assert.Empty(t, actualCrd.Spec.Conversion.Webhook)
+	})
+
 	// Generation must not be skipped because webhook startup routine listens for the secret
-	// See cmd/operator/manager.go and cmd/operator/watcher.go
+	// See cmd/webhook/manager.go and cmd/certificate/watcher.go
 	t.Run(`do not skip certificates generation if no configuration exists`, func(t *testing.T) {
 		fakeClient := fake.NewClient()
 		controller, request := prepareController(t, fakeClient)
@@ -216,7 +294,7 @@ func prepareFakeClient(withSecret bool, generateValidSecret bool) client.Client
 	objs := []client.Object{
 		&admissionregistrationv1.MutatingWebhookConfiguration{
 			ObjectMeta: metav1.ObjectMeta{
-				Name: Webhookconfig,
+				Name: MutatingWebhookconfig,
 			},
 			Webhooks: []admissionregistrationv1.MutatingWebhook{
 				{
@@ -228,6 +306,20 @@ func prepareFakeClient(withSecret bool, generateValidSecret bool) client.Client
 			},
 		},
 
+		&admissionregistrationv1.ValidatingWebhookConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: ValidatingWebhookconfig,
+			},
+			Webhooks: []admissionregistrationv1.ValidatingWebhook{
+				{
+					ClientConfig: admissionregistrationv1.WebhookClientConfig{CABundle: []byte("myunmodifiedbundle")},
+				},
+				{
+					ClientConfig: admissionregistrationv1.WebhookClientConfig{},
+				},
+			},
+		},
+
 		&apiv1.CustomResourceDefinition{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: "mycrd1",
@@ -354,10 +446,18 @@ func verifyCertificates(t *testing.T, secret *corev1.Secret, clt client.Client,
 
 	mutatingWebhookConfig := &admissionregistrationv1.MutatingWebhookConfiguration{}
 	err := clt.Get(context.TODO(), client.ObjectKey{
-		Name: Webhookconfig,
+		Name: MutatingWebhookconfig,
 	}, mutatingWebhookConfig)
 	require.NoError(t, err)
 	assert.Len(t, mutatingWebhookConfig.Webhooks, 2)
 	testWebhookClientConfig(t, &mutatingWebhookConfig.Webhooks[0].ClientConfig, secret.Data, isUpdate)
 
+	validatingWebhookConfig := &admissionregistrationv1.ValidatingWebhookConfiguration{}
+	err = clt.Get(context.TODO(), client.ObjectKey{
+		Name: ValidatingWebhookconfig,
+	}, validatingWebhookConfig)
+	require.NoError(t, err)
+	assert.Len(t, validatingWebhookConfig.Webhooks, 2)
+	testWebhookClientConfig(t, &validatingWebhookConfig.Webhooks[0].ClientConfig, secret.Data, isUpdate)
+
 }

klt-cert-manager/controllers/keptnwebhookcontroller/webhook_configuration.go

@@ -13,3 +13,15 @@ func getClientConfigsFromMutatingWebhook(mutatingWebhookConfig *admissionregistr
 	}
 	return mutatingWebhookConfigs
 }
+
+func getClientConfigsFromValidatingWebhook(validatingWebhookConfig *admissionregistrationv1.ValidatingWebhookConfiguration) []*admissionregistrationv1.WebhookClientConfig {
+	if validatingWebhookConfig == nil {
+		return nil
+	}
+
+	mutatingWebhookConfigs := make([]*admissionregistrationv1.WebhookClientConfig, len(validatingWebhookConfig.Webhooks))
+	for i := range validatingWebhookConfig.Webhooks {
+		mutatingWebhookConfigs[i] = &validatingWebhookConfig.Webhooks[i].ClientConfig
+	}
+	return mutatingWebhookConfigs
+}

klt-cert-manager/controllers/keptnwebhookcontroller/webhook_configuration_test.go

@@ -21,6 +21,20 @@ func createTestMutatingWebhookConfig(_ *testing.T) *admissionregistrationv1.Muta
 	}
 }
 
+func createTestValidatingWebhookConfig(_ *testing.T) *admissionregistrationv1.ValidatingWebhookConfiguration {
+	return &admissionregistrationv1.ValidatingWebhookConfiguration{
+		Webhooks: []admissionregistrationv1.ValidatingWebhook{
+			{},
+			{ClientConfig: admissionregistrationv1.WebhookClientConfig{}},
+			{
+				ClientConfig: admissionregistrationv1.WebhookClientConfig{
+					CABundle: []byte{0, 1, 2, 3, 4},
+				},
+			},
+		},
+	}
+}
+
 func TestGetClientConfigsFromMutatingWebhook(t *testing.T) {
 	t.Run(`returns nil when config is nil`, func(t *testing.T) {
 		clientConfigs := getClientConfigsFromMutatingWebhook(nil)
@@ -34,3 +48,17 @@ func TestGetClientConfigsFromMutatingWebhook(t *testing.T) {
 		assert.Equal(t, expectedClientConfigs, len(clientConfigs))
 	})
 }
+
+func TestGetClientConfigsFromValidatingWebhook(t *testing.T) {
+	t.Run(`returns nil when config is nil`, func(t *testing.T) {
+		clientConfigs := getClientConfigsFromValidatingWebhook(nil)
+		assert.Nil(t, clientConfigs)
+	})
+	t.Run(`returns client configs of all configured webhooks`, func(t *testing.T) {
+		const expectedClientConfigs = 3
+		clientConfigs := getClientConfigsFromValidatingWebhook(createTestValidatingWebhookConfig(t))
+
+		assert.NotNil(t, clientConfigs)
+		assert.Equal(t, expectedClientConfigs, len(clientConfigs))
+	})
+}

operator/PROJECT

@@ -186,4 +186,7 @@ resources:
   kind: KeptnMetric
   path: github.com/keptn/lifecycle-toolkit/operator/apis/metrics/v1alpha1
   version: v1alpha1
+  webhooks:
+    validation: true
+    webhookVersion: v1
 version: "3"

operator/apis/metrics/v1alpha1/common/common.go

@@ -0,0 +1,9 @@
+package common
+
+import (
+	"github.com/pkg/errors"
+)
+
+const KeptnMetricProviderName = "keptn-metric"
+
+var ErrForbiddenProvider = errors.New("Forbidden! KeptnMetrics should define a provider different from keptn-metric")

operator/apis/metrics/v1alpha1/keptnmetric_types.go

@@ -17,6 +17,7 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"github.com/keptn/lifecycle-toolkit/operator/apis/metrics/v1alpha1/common"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -77,6 +78,14 @@ func init() {
 	SchemeBuilder.Register(&KeptnMetric{}, &KeptnMetricList{})
 }
 
-func (s KeptnMetric) IsStatusSet() bool {
+func (s *KeptnMetric) IsStatusSet() bool {
 	return s.Status.Value != ""
 }
+
+// IsProviderValid verifies that the KeptnMetric provider is not of the type keptn-metric.
+// This is avoiding having the keptn metric controller constantly probing the API server for a KeptnMetric
+// without any action being performed on the metric.
+// The keptn-metric provider should be used only to declare an evaluation provider and not a metric provider.
+func (s *KeptnMetric) IsProviderValid(provider string) bool {
+	return provider != common.KeptnMetricProviderName
+}

operator/apis/metrics/v1alpha1/keptnmetric_types_test.go

@@ -0,0 +1,42 @@
+package v1alpha1
+
+import (
+	"reflect"
+	"testing"
+
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+func Test_IsProviderValid(t *testing.T) {
+
+	tests := []struct {
+		name         string
+		providerName string
+		fldPath      *field.Path
+		want         bool
+	}{
+		{
+			name:         "bad provider",
+			providerName: "keptn-metric",
+			want:         false,
+		},
+		{
+			name:         "good provider",
+			providerName: "prometheus",
+			want:         true,
+		},
+	}
+	for _, tt := range tests {
+
+		s := &KeptnMetric{
+			ObjectMeta: v1.ObjectMeta{Name: tt.name},
+			Spec:       KeptnMetricSpec{Provider: ProviderRef{Name: tt.providerName}},
+		}
+		t.Run(tt.name, func(t *testing.T) {
+			if got := s.IsProviderValid(tt.providerName); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("validateProviderName() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}