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.

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

fix: admit pod without creating KLT resources if owner of the pod is not supported #1752

Merged
merged 4 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions operator/apis/lifecycle/v1alpha3/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
}
74 changes: 74 additions & 0 deletions operator/apis/lifecycle/v1alpha3/common/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"testing"

"github.com/stretchr/testify/require"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

const ExtraLongName = "loooooooooooooooooooooo00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000ooooooo01234567891234567890123456789"
Expand Down Expand Up @@ -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)
}
})
}
}
19 changes: 14 additions & 5 deletions operator/webhooks/pod_mutator/pod_mutating_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,15 @@ 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 == "" {
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))

podIsAnnotated := a.isPodAnnotated(pod)
Expand Down Expand Up @@ -139,7 +148,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
}
Expand All @@ -152,7 +161,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
}
Expand Down Expand Up @@ -381,7 +390,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{
Expand Down Expand Up @@ -441,11 +450,11 @@ 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 {
RealAnna marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
103 changes: 100 additions & 3 deletions operator/webhooks/pod_mutator/pod_mutating_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,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"
Expand All @@ -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
Expand All @@ -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{
{
Expand All @@ -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{
{
Expand Down Expand Up @@ -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{
Expand Down
5 changes: 5 additions & 0 deletions test/integration/unsupported-owner-of-pod/00-assert.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
apiVersion: v1
kind: Pod
metadata:
labels:
app: test
19 changes: 19 additions & 0 deletions test/integration/unsupported-owner-of-pod/00-install.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
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:
restartPolicy: Never
containers:
- name: init-myservice
image: busybox:1.36.1
command: ['sh', '-c', 'sleep 60']
4 changes: 4 additions & 0 deletions test/integration/unsupported-owner-of-pod/00-teststep.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
apiVersion: kuttl.dev/v1
kind: TestStep
commands:
- script: kubectl annotate ns $NAMESPACE keptn.sh/lifecycle-toolkit='enabled'