From 900b2af1b0ab237f8d32d393207c07a7b380e565 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 20 Jul 2023 08:57:05 +0200 Subject: [PATCH 1/4] fix: admit pod without creating KLT resources if owner of the pod is not one of the supported ones Signed-off-by: Florian Bacher --- .../pod_mutator/pod_mutating_webhook.go | 16 ++- .../pod_mutator/pod_mutating_webhook_test.go | 103 +++++++++++++++++- .../unsupported-owner-of-pod/00-assert.yaml | 6 + .../unsupported-owner-of-pod/00-install.yaml | 18 +++ .../unsupported-owner-of-pod/00-teststep.yaml | 4 + 5 files changed, 140 insertions(+), 7 deletions(-) create mode 100644 test/integration/unsupported-owner-of-pod/00-assert.yaml create mode 100644 test/integration/unsupported-owner-of-pod/00-install.yaml create mode 100644 test/integration/unsupported-owner-of-pod/00-teststep.yaml diff --git a/operator/webhooks/pod_mutator/pod_mutating_webhook.go b/operator/webhooks/pod_mutator/pod_mutating_webhook.go index 3a43b61368..cb391d1cc8 100644 --- a/operator/webhooks/pod_mutator/pod_mutating_webhook.go +++ b/operator/webhooks/pod_mutator/pod_mutating_webhook.go @@ -81,6 +81,14 @@ func (a *PodMutatingWebhook) Handle(ctx context.Context, req admission.Request) return admission.Allowed("namespace is not enabled for lifecycle controller") } + // check the OwnerReference of the pod to see if it is supported and intended to be managed by KLT + ownerRef := a.getOwnerReference(pod.ObjectMeta) + + if ownerRef.Kind == "" { + logger.Info("owner of pod is not supported by KLT", "namespace", req.Namespace) + return admission.Allowed("namespace is not enabled for lifecycle controller") + } + logger.Info(fmt.Sprintf("Pod annotations: %v", pod.Annotations)) podIsAnnotated := a.isPodAnnotated(pod) @@ -139,7 +147,7 @@ func (a *PodMutatingWebhook) isPodAnnotated(pod *corev1.Pod) bool { } func (a *PodMutatingWebhook) copyAnnotationsIfParentAnnotated(ctx context.Context, req *admission.Request, pod *corev1.Pod) bool { - podOwner := a.getOwnerReference(&pod.ObjectMeta) + podOwner := a.getOwnerReference(pod.ObjectMeta) if podOwner.UID == "" { return false } @@ -152,7 +160,7 @@ func (a *PodMutatingWebhook) copyAnnotationsIfParentAnnotated(ctx context.Contex } a.Log.Info("Done fetching RS") - rsOwner := a.getOwnerReference(&rs.ObjectMeta) + rsOwner := a.getOwnerReference(rs.ObjectMeta) if rsOwner.UID == "" { return false } @@ -381,7 +389,7 @@ func (a *PodMutatingWebhook) generateWorkload(ctx context.Context, pod *corev1.P traceContextCarrier := propagation.MapCarrier{} otel.GetTextMapPropagator().Inject(ctx, traceContextCarrier) - ownerRef := a.getOwnerReference(&pod.ObjectMeta) + ownerRef := a.getOwnerReference(pod.ObjectMeta) return &klcv1alpha3.KeptnWorkload{ ObjectMeta: metav1.ObjectMeta{ @@ -441,7 +449,7 @@ func (a *PodMutatingWebhook) getAppName(pod *corev1.Pod) string { return strings.ToLower(applicationName) } -func (a *PodMutatingWebhook) getOwnerReference(resource *metav1.ObjectMeta) metav1.OwnerReference { +func (a *PodMutatingWebhook) getOwnerReference(resource metav1.ObjectMeta) metav1.OwnerReference { reference := metav1.OwnerReference{} if len(resource.OwnerReferences) != 0 { for _, owner := range resource.OwnerReferences { diff --git a/operator/webhooks/pod_mutator/pod_mutating_webhook_test.go b/operator/webhooks/pod_mutator/pod_mutating_webhook_test.go index 7562b1d18b..8cfcf36216 100644 --- a/operator/webhooks/pod_mutator/pod_mutating_webhook_test.go +++ b/operator/webhooks/pod_mutator/pod_mutating_webhook_test.go @@ -3,6 +3,7 @@ package pod_mutator import ( "context" "encoding/json" + "k8s.io/apimachinery/pkg/api/errors" "reflect" "testing" @@ -34,7 +35,7 @@ func TestPodMutatingWebhook_getOwnerReference(t *testing.T) { Log logr.Logger } type args struct { - resource *metav1.ObjectMeta + resource metav1.ObjectMeta } tests := []struct { name string @@ -45,7 +46,7 @@ func TestPodMutatingWebhook_getOwnerReference(t *testing.T) { { name: "Test simple return when UID and Kind is set", args: args{ - resource: &metav1.ObjectMeta{ + resource: metav1.ObjectMeta{ UID: "the-pod-uid", OwnerReferences: []metav1.OwnerReference{ { @@ -65,7 +66,7 @@ func TestPodMutatingWebhook_getOwnerReference(t *testing.T) { { name: "Test return is input argument if owner is not found", args: args{ - resource: &metav1.ObjectMeta{ + resource: metav1.ObjectMeta{ UID: "the-pod-uid", OwnerReferences: []metav1.OwnerReference{ { @@ -1007,6 +1008,102 @@ func TestPodMutatingWebhook_Handle_DisabledNamespace(t *testing.T) { require.True(t, resp.Allowed) } +func TestPodMutatingWebhook_Handle_UnsupportedOwner(t *testing.T) { + fakeClient := fakeclient.NewClient(&corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Annotations: map[string]string{ + apicommon.NamespaceEnabledAnnotation: "enabled", + }, + }, + }) + + tr := &fakeclient.ITracerMock{StartFunc: func(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span) { + return ctx, trace.SpanFromContext(ctx) + }} + + decoder, err := admission.NewDecoder(runtime.NewScheme()) + require.Nil(t, err) + + wh := &PodMutatingWebhook{ + Client: fakeClient, + Tracer: tr, + Decoder: decoder, + EventSender: controllercommon.NewEventSender(record.NewFakeRecorder(100)), + Log: testr.New(t), + } + + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-pod", + Namespace: "default", + Annotations: map[string]string{ + apicommon.WorkloadAnnotation: "my-workload", + apicommon.VersionAnnotation: "0.1", + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "batchv1", + Kind: "Job", + Name: "my-job", + UID: "1234", + }, + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "example-container", + Image: "nginx", + }, + }, + }, + } + + // Convert the Pod object to a byte array + podBytes, err := json.Marshal(pod) + require.Nil(t, err) + + // Create an AdmissionRequest object + request := admissionv1.AdmissionRequest{ + UID: "12345", + Kind: metav1.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}, + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: podBytes, + }, + Namespace: "default", + } + + resp := wh.Handle(context.TODO(), admission.Request{ + AdmissionRequest: request, + }) + + require.NotNil(t, resp) + require.True(t, resp.Allowed) + + // if we get an unsupported owner for the pod, we expect not to have any KLT resources to have been created + kacr := &klcv1alpha3.KeptnAppCreationRequest{} + + err = fakeClient.Get(context.Background(), types.NamespacedName{ + Namespace: "default", + Name: "my-workload", + }, kacr) + + require.NotNil(t, err) + require.True(t, errors.IsNotFound(err)) + + workload := &klcv1alpha3.KeptnWorkload{} + + err = fakeClient.Get(context.TODO(), types.NamespacedName{ + Namespace: "default", + Name: "my-workload-my-workload", + }, workload) + + require.NotNil(t, err) + require.True(t, errors.IsNotFound(err)) +} + func TestPodMutatingWebhook_Handle_SingleService(t *testing.T) { fakeClient := fakeclient.NewClient(&corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ diff --git a/test/integration/unsupported-owner-of-pod/00-assert.yaml b/test/integration/unsupported-owner-of-pod/00-assert.yaml new file mode 100644 index 0000000000..8ea760fb51 --- /dev/null +++ b/test/integration/unsupported-owner-of-pod/00-assert.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: Pod +metadata: + labels: + app: test + name: test \ No newline at end of file diff --git a/test/integration/unsupported-owner-of-pod/00-install.yaml b/test/integration/unsupported-owner-of-pod/00-install.yaml new file mode 100644 index 0000000000..5c826319a5 --- /dev/null +++ b/test/integration/unsupported-owner-of-pod/00-install.yaml @@ -0,0 +1,18 @@ +--- +apiVersion: batch/v1 +kind: Job +metadata: + name: test +spec: + template: + metadata: + labels: + app: test + annotations: + keptn.sh/workload: waiter + keptn.sh/version: "0.4" + spec: + containers: + - name: init-myservice + image: busybox:1.36.1 + command: ['sh', '-c', 'sleep 30'] \ No newline at end of file diff --git a/test/integration/unsupported-owner-of-pod/00-teststep.yaml b/test/integration/unsupported-owner-of-pod/00-teststep.yaml new file mode 100644 index 0000000000..ad4f1d95d5 --- /dev/null +++ b/test/integration/unsupported-owner-of-pod/00-teststep.yaml @@ -0,0 +1,4 @@ +apiVersion: kuttl.dev/v1 +kind: TestStep +commands: + - script: kubectl annotate ns $NAMESPACE keptn.sh/lifecycle-toolkit='enabled' From c689ffe3627f327409eb176c13d9aa7c4c336abf Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 20 Jul 2023 09:20:05 +0200 Subject: [PATCH 2/4] fix: admit pod without creating KLT resources if owner of the pod is not one of the supported ones Signed-off-by: Florian Bacher --- operator/webhooks/pod_mutator/pod_mutating_webhook.go | 5 +++-- operator/webhooks/pod_mutator/pod_mutating_webhook_test.go | 2 +- test/integration/unsupported-owner-of-pod/00-assert.yaml | 2 +- test/integration/unsupported-owner-of-pod/00-install.yaml | 3 ++- test/integration/unsupported-owner-of-pod/00-teststep.yaml | 1 + 5 files changed, 8 insertions(+), 5 deletions(-) diff --git a/operator/webhooks/pod_mutator/pod_mutating_webhook.go b/operator/webhooks/pod_mutator/pod_mutating_webhook.go index cb391d1cc8..c6f71a6f75 100644 --- a/operator/webhooks/pod_mutator/pod_mutating_webhook.go +++ b/operator/webhooks/pod_mutator/pod_mutating_webhook.go @@ -85,8 +85,9 @@ func (a *PodMutatingWebhook) Handle(ctx context.Context, req admission.Request) ownerRef := a.getOwnerReference(pod.ObjectMeta) if ownerRef.Kind == "" { - logger.Info("owner of pod is not supported by KLT", "namespace", req.Namespace) - return admission.Allowed("namespace is not enabled for lifecycle controller") + msg := "owner of pod is not supported by lifecycle controller" + logger.Info(msg, "namespace", req.Namespace, "pod", req.Name) + return admission.Allowed(msg) } logger.Info(fmt.Sprintf("Pod annotations: %v", pod.Annotations)) diff --git a/operator/webhooks/pod_mutator/pod_mutating_webhook_test.go b/operator/webhooks/pod_mutator/pod_mutating_webhook_test.go index 8cfcf36216..764a79781c 100644 --- a/operator/webhooks/pod_mutator/pod_mutating_webhook_test.go +++ b/operator/webhooks/pod_mutator/pod_mutating_webhook_test.go @@ -3,7 +3,6 @@ package pod_mutator import ( "context" "encoding/json" - "k8s.io/apimachinery/pkg/api/errors" "reflect" "testing" @@ -18,6 +17,7 @@ import ( admissionv1 "k8s.io/api/admission/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" diff --git a/test/integration/unsupported-owner-of-pod/00-assert.yaml b/test/integration/unsupported-owner-of-pod/00-assert.yaml index 8ea760fb51..1c4ab73dba 100644 --- a/test/integration/unsupported-owner-of-pod/00-assert.yaml +++ b/test/integration/unsupported-owner-of-pod/00-assert.yaml @@ -3,4 +3,4 @@ kind: Pod metadata: labels: app: test - name: test \ No newline at end of file + name: test diff --git a/test/integration/unsupported-owner-of-pod/00-install.yaml b/test/integration/unsupported-owner-of-pod/00-install.yaml index 5c826319a5..f933121e7e 100644 --- a/test/integration/unsupported-owner-of-pod/00-install.yaml +++ b/test/integration/unsupported-owner-of-pod/00-install.yaml @@ -12,7 +12,8 @@ spec: keptn.sh/workload: waiter keptn.sh/version: "0.4" spec: + restartPolicy: Never containers: - name: init-myservice image: busybox:1.36.1 - command: ['sh', '-c', 'sleep 30'] \ No newline at end of file + command: ['sh', '-c', 'sleep 60'] diff --git a/test/integration/unsupported-owner-of-pod/00-teststep.yaml b/test/integration/unsupported-owner-of-pod/00-teststep.yaml index ad4f1d95d5..add4788068 100644 --- a/test/integration/unsupported-owner-of-pod/00-teststep.yaml +++ b/test/integration/unsupported-owner-of-pod/00-teststep.yaml @@ -2,3 +2,4 @@ apiVersion: kuttl.dev/v1 kind: TestStep commands: - script: kubectl annotate ns $NAMESPACE keptn.sh/lifecycle-toolkit='enabled' + From b444e768dcf53705433664cb8a30b9903d8cef24 Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 20 Jul 2023 09:56:13 +0200 Subject: [PATCH 3/4] fix test assertion and yaml lint issue Signed-off-by: Florian Bacher --- test/integration/unsupported-owner-of-pod/00-assert.yaml | 1 - test/integration/unsupported-owner-of-pod/00-teststep.yaml | 1 - 2 files changed, 2 deletions(-) diff --git a/test/integration/unsupported-owner-of-pod/00-assert.yaml b/test/integration/unsupported-owner-of-pod/00-assert.yaml index 1c4ab73dba..1280385103 100644 --- a/test/integration/unsupported-owner-of-pod/00-assert.yaml +++ b/test/integration/unsupported-owner-of-pod/00-assert.yaml @@ -3,4 +3,3 @@ kind: Pod metadata: labels: app: test - name: test diff --git a/test/integration/unsupported-owner-of-pod/00-teststep.yaml b/test/integration/unsupported-owner-of-pod/00-teststep.yaml index add4788068..ad4f1d95d5 100644 --- a/test/integration/unsupported-owner-of-pod/00-teststep.yaml +++ b/test/integration/unsupported-owner-of-pod/00-teststep.yaml @@ -2,4 +2,3 @@ apiVersion: kuttl.dev/v1 kind: TestStep commands: - script: kubectl annotate ns $NAMESPACE keptn.sh/lifecycle-toolkit='enabled' - From 95d3ee16a69a09ca2bca6702c66b0e845884fb9c Mon Sep 17 00:00:00 2001 From: Florian Bacher Date: Thu, 20 Jul 2023 10:55:00 +0200 Subject: [PATCH 4/4] incorporate PR review comment Signed-off-by: Florian Bacher --- .../apis/lifecycle/v1alpha3/common/common.go | 6 ++ .../lifecycle/v1alpha3/common/common_test.go | 74 +++++++++++++++++++ .../pod_mutator/pod_mutating_webhook.go | 2 +- 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/operator/apis/lifecycle/v1alpha3/common/common.go b/operator/apis/lifecycle/v1alpha3/common/common.go index 1fee458789..13872a60cb 100644 --- a/operator/apis/lifecycle/v1alpha3/common/common.go +++ b/operator/apis/lifecycle/v1alpha3/common/common.go @@ -9,6 +9,7 @@ import ( operatorcommon "github.com/keptn/lifecycle-toolkit/operator/common" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) const WorkloadAnnotation = "keptn.sh/workload" @@ -200,3 +201,8 @@ func MergeMaps(m1 map[string]string, m2 map[string]string) map[string]string { } return merged } + +// IsOwnerSupported returns whether the owner of the given object is supported to be considered a KeptnWorklooad +func IsOwnerSupported(owner metav1.OwnerReference) bool { + return owner.Kind == "ReplicaSet" || owner.Kind == "Deployment" || owner.Kind == "StatefulSet" || owner.Kind == "DaemonSet" || owner.Kind == "Rollout" +} diff --git a/operator/apis/lifecycle/v1alpha3/common/common_test.go b/operator/apis/lifecycle/v1alpha3/common/common_test.go index 293f3f9251..c39eaed9fe 100644 --- a/operator/apis/lifecycle/v1alpha3/common/common_test.go +++ b/operator/apis/lifecycle/v1alpha3/common/common_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/require" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) const ExtraLongName = "loooooooooooooooooooooo00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000ooooooo01234567891234567890123456789" @@ -415,3 +416,76 @@ func Test_MergeMaps(t *testing.T) { }) } } + +func TestIsOwnerSupported(t *testing.T) { + type args struct { + owner v1.OwnerReference + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "Deployment -> true", + args: args{ + owner: v1.OwnerReference{ + Kind: "Deployment", + }, + }, + want: true, + }, + { + name: "DaemonSet-> true", + args: args{ + owner: v1.OwnerReference{ + Kind: "DaemonSet", + }, + }, + want: true, + }, + { + name: "ReplicaSet-> true", + args: args{ + owner: v1.OwnerReference{ + Kind: "ReplicaSet", + }, + }, + want: true, + }, + { + name: "StatefulSet-> true", + args: args{ + owner: v1.OwnerReference{ + Kind: "StatefulSet", + }, + }, + want: true, + }, + { + name: "Rollout-> true", + args: args{ + owner: v1.OwnerReference{ + Kind: "Rollout", + }, + }, + want: true, + }, + { + name: "Job-> false", + args: args{ + owner: v1.OwnerReference{ + Kind: "Job", + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsOwnerSupported(tt.args.owner); got != tt.want { + t.Errorf("IsOwnerSupported() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/operator/webhooks/pod_mutator/pod_mutating_webhook.go b/operator/webhooks/pod_mutator/pod_mutating_webhook.go index c6f71a6f75..593862d48c 100644 --- a/operator/webhooks/pod_mutator/pod_mutating_webhook.go +++ b/operator/webhooks/pod_mutator/pod_mutating_webhook.go @@ -454,7 +454,7 @@ func (a *PodMutatingWebhook) getOwnerReference(resource metav1.ObjectMeta) metav reference := metav1.OwnerReference{} if len(resource.OwnerReferences) != 0 { for _, owner := range resource.OwnerReferences { - if owner.Kind == "ReplicaSet" || owner.Kind == "Deployment" || owner.Kind == "StatefulSet" || owner.Kind == "DaemonSet" || owner.Kind == "Rollout" { + if apicommon.IsOwnerSupported(owner) { reference.UID = owner.UID reference.Kind = owner.Kind reference.Name = owner.Name