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.

Sign up for GitHub

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 Pod Mutation loop #953

Merged
merged 10 commits into from
Jul 4, 2022
Merged

Fix Pod Mutation loop #953

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c7974da
chore(webhookhandler): check if pod already instrumented
mat-rumian Jun 27, 2022
cfb8b81
chore(instrumentations): add check for volume and instrumentation env…
mat-rumian Jun 27, 2022
5acbe82
Merge branch 'open-telemetry:main' into fix-pod-mutation-loop
mat-rumian Jun 27, 2022
7f3989c
chore(webhookhandler): add helper tests
mat-rumian Jun 28, 2022
5dbafa7
chore(instrumentation): add helper tests
mat-rumian Jun 28, 2022
b906f7d
chore(instrumentation): update volume checking method
mat-rumian Jun 28, 2022
b5f1cc1
chore(lint): fix
mat-rumian Jun 28, 2022
99346db
chore(webhookhandler): move out pod instrumentation check
mat-rumian Jun 30, 2022
1fdb631
chore(instrumentation): add pod instrumentation inject check
mat-rumian Jun 30, 2022
08e0c1d
chore(instrumentation): clean up
mat-rumian Jul 4, 2022
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
27 changes: 27 additions & 0 deletions internal/webhookhandler/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package webhookhandler
mat-rumian marked this conversation as resolved.
Show resolved Hide resolved

import corev1 "k8s.io/api/core/v1"

// Checks if Pod is already instrumented by checking Instrumentation InitContainer presence.
func IsPodInstrumentationMissing(pod corev1.Pod) bool {
mat-rumian marked this conversation as resolved.
Show resolved Hide resolved
for _, cont := range pod.Spec.InitContainers {
if cont.Name == initContainerName {
return false
}
}
return true
}
74 changes: 74 additions & 0 deletions internal/webhookhandler/helper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package webhookhandler

import (
"testing"

"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
)

func TestPodInstrumentationMissing(t *testing.T) {
tests := []struct {
name string
pod corev1.Pod
expected bool
}{
{
name: "PodInstrumentation_Already_Inject",
pod: corev1.Pod{
Spec: corev1.PodSpec{
InitContainers: []corev1.Container{
{
Name: "magic-init",
},
{
Name: initContainerName,
},
},
},
},
expected: false,
},
{
name: "PodInstrumentation_Absent_1",
pod: corev1.Pod{
Spec: corev1.PodSpec{
InitContainers: []corev1.Container{
{
Name: "magic-init",
},
},
},
},
expected: true,
},
{
name: "PodInstrumentation_Absent_2",
pod: corev1.Pod{
Spec: corev1.PodSpec{},
},
expected: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
result := IsPodInstrumentationMissing(test.pod)
assert.Equal(t, test.expected, result)
})
}
}
14 changes: 9 additions & 5 deletions internal/webhookhandler/webhookhandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import (
// +kubebuilder:rbac:groups=opentelemetry.io,resources=instrumentations,verbs=get;list;watch
// +kubebuilder:rbac:groups="apps",resources=replicasets,verbs=get;list;watch

const initContainerName = "opentelemetry-auto-instrumentation"
mat-rumian marked this conversation as resolved.
Show resolved Hide resolved

var _ WebhookHandler = (*podSidecarInjector)(nil)

// WebhookHandler is a webhook handler that analyzes new pods and injects appropriate sidecars into it.
Expand Down Expand Up @@ -89,11 +91,13 @@ func (p *podSidecarInjector) Handle(ctx context.Context, req admission.Request)
}

for _, m := range p.podMutators {
pod, err = m.Mutate(ctx, ns, pod)
if err != nil {
res := admission.Errored(http.StatusInternalServerError, err)
res.Allowed = true
return res
if IsPodInstrumentationMissing(pod) {
pod, err = m.Mutate(ctx, ns, pod)
if err != nil {
res := admission.Errored(http.StatusInternalServerError, err)
res.Allowed = true
return res
}
}
}

Expand Down
24 changes: 23 additions & 1 deletion pkg/instrumentation/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@

package instrumentation

import corev1 "k8s.io/api/core/v1"
import (
"strings"

corev1 "k8s.io/api/core/v1"
)

// Calculate if we already inject InitContainers.
func IsInitContainerMissing(pod corev1.Pod) bool {
Expand All @@ -25,3 +29,21 @@ func IsInitContainerMissing(pod corev1.Pod) bool {
}
return true
}

// Check if opentelemetry-auto-instrumentation volume is on the list.
func IsOtAIVolumeMissing(volumeMounts []corev1.VolumeMount) bool {
mat-rumian marked this conversation as resolved.
Show resolved Hide resolved
for _, volumeMount := range volumeMounts {
if volumeMount.Name == volumeName {
return false
}
}
return true
}

// Check if EnvVar value contains instrumentation string.
func IsEnvVarValueInstrumentationMissing(envVar corev1.EnvVar, instrumentation string) bool {
mat-rumian marked this conversation as resolved.
Show resolved Hide resolved
if strings.Contains(envVar.Value, instrumentation) {
return false
}
return true
}
79 changes: 79 additions & 0 deletions pkg/instrumentation/helper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,82 @@ func TestInitContainerMissing(t *testing.T) {
})
}
}

func TestOtAiVolumeMissing(t *testing.T) {
tests := []struct {
name string
volume []corev1.VolumeMount
expected bool
}{
{
name: "Volume_Already_Inject",
volume: []corev1.VolumeMount{
{
Name: volumeName,
},
},
expected: false,
},
{
name: "Volume_Absent_1",
volume: []corev1.VolumeMount{
{
Name: "magic-volume",
},
},
expected: true,
},
{
name: "Volume_Absent_2",
volume: []corev1.VolumeMount{},
expected: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
result := IsOtAIVolumeMissing(test.volume)
assert.Equal(t, test.expected, result)
})
}
}

func TestEnvVarInstrumentationValueMissing(t *testing.T) {
tests := []struct {
name string
envVar corev1.EnvVar
instrumentationStr string
expected bool
}{
{
name: "EnvVar_Instrumentation_Value_Already_Inject",
envVar: corev1.EnvVar{
Name: envJavaToolsOptions,
Value: javaJVMArgument,
},
instrumentationStr: javaJVMArgument,
expected: false,
},
{
name: "EnvVar_Instrumentation_Value_Absent_1",
envVar: corev1.EnvVar{

Name: envNodeOptions,
Value: "some-magic-node-options",
},
instrumentationStr: envNodeOptions,
expected: true,
}, {
name: "EnvVar_Instrumentation_Value_Absent_2",
envVar: corev1.EnvVar{},
instrumentationStr: envNodeOptions,
expected: true,
}}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
result := IsEnvVarValueInstrumentationMissing(test.envVar, test.instrumentationStr)
assert.Equal(t, test.expected, result)
})
}
}
16 changes: 11 additions & 5 deletions pkg/instrumentation/javaagent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,18 @@ func injectJavaagent(logger logr.Logger, javaSpec v1alpha1.Java, pod corev1.Pod,
logger.Info("Skipping javaagent injection, the container defines JAVA_TOOL_OPTIONS env var value via ValueFrom", "container", container.Name)
return pod
}
container.Env[idx].Value = container.Env[idx].Value + javaJVMArgument

if IsEnvVarValueInstrumentationMissing(container.Env[idx], javaJVMArgument) {
mat-rumian marked this conversation as resolved.
Show resolved Hide resolved
container.Env[idx].Value = container.Env[idx].Value + javaJVMArgument
}
}

if IsOtAIVolumeMissing(container.VolumeMounts) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this check needed?

The IsAutoInstrumentationInjected is not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So IsOtAIVolumeMissing and IsEnvVarValueInstrumentationMissing were applied before I've added check for pod mutation. When Pod was mutated few times k8s was throwing errors like duplicated volume name. In case of env var it happened that e.g. javaJVMArgument was added few times to JAVA_TOOL_OPTIONS. So I left this checks for safety for other unexpected mutations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to remove this and IsEnvVarValueInstrumentationMissing as well and rely only on the init container for simplicity.

We should keep it here only if there is a known scenario which the init container does not cover

container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: "/otel-auto-instrumentation",
})
}
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: "/otel-auto-instrumentation",
})

// We just inject Volumes and init containers for the first processed container
if IsInitContainerMissing(pod) {
Expand Down
16 changes: 11 additions & 5 deletions pkg/instrumentation/nodejs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,18 @@ func injectNodeJSSDK(logger logr.Logger, nodeJSSpec v1alpha1.NodeJS, pod corev1.
logger.Info("Skipping NodeJS SDK injection, the container defines NODE_OPTIONS env var value via ValueFrom", "container", container.Name)
return pod
}
container.Env[idx].Value = container.Env[idx].Value + nodeRequireArgument

if IsEnvVarValueInstrumentationMissing(container.Env[idx], nodeRequireArgument) {
container.Env[idx].Value = container.Env[idx].Value + nodeRequireArgument
}
}

if IsOtAIVolumeMissing(container.VolumeMounts) {
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: "/otel-auto-instrumentation",
})
}
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: "/otel-auto-instrumentation",
})

// We just inject Volumes and init containers for the first processed container
if IsInitContainerMissing(pod) {
Expand Down
15 changes: 10 additions & 5 deletions pkg/instrumentation/python.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1.
logger.Info("Skipping Python SDK injection, the container defines PYTHONPATH env var value via ValueFrom", "container", container.Name)
return pod
}
container.Env[idx].Value = fmt.Sprintf("%s:%s:%s", pythonPathPrefix, container.Env[idx].Value, pythonPathSuffix)

if IsEnvVarValueInstrumentationMissing(container.Env[idx], pythonPathPrefix) {
container.Env[idx].Value = fmt.Sprintf("%s:%s:%s", pythonPathPrefix, container.Env[idx].Value, pythonPathSuffix)
}
}

// Set OTEL_TRACES_EXPORTER to HTTP exporter if not set by user because it is what our autoinstrumentation supports.
Expand All @@ -66,10 +69,12 @@ func injectPythonSDK(logger logr.Logger, pythonSpec v1alpha1.Python, pod corev1.
})
}

container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: "/otel-auto-instrumentation",
})
if IsOtAIVolumeMissing(container.VolumeMounts) {
container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: "/otel-auto-instrumentation",
})
}

// We just inject Volumes and init containers for the first processed container
if IsInitContainerMissing(pod) {
Expand Down