Skip to content

Commit 1d4112d

Browse files
yuriolisayuriolisa
committed
Fixed pod's precedence in auto-instrumentation
Signed-off-by: Yuri Oliveira <[email protected]> Fixed pod's precedence in auto-instrumentation Signed-off-by: Yuri Oliveira <[email protected]> Fixed pod's precedence in auto-instrumentation Signed-off-by: Yuri Oliveira <[email protected]> Fixed pod's precedence in auto-instrumentation Signed-off-by: Yuri Oliveira <[email protected]> Fixed pod's precedence in auto-instrumentation Signed-off-by: Yuri Oliveira <[email protected]> Fixed pod's precedence in auto-instrumentation Signed-off-by: Yuri Oliveira <[email protected]>
1 parent e078cc9 commit 1d4112d

15 files changed

+322
-155
lines changed

‎pkg/instrumentation/annotation.go

+39-26
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,6 @@
44
package instrumentation
55

66
import (
7-
"strings"
8-
97
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
108
)
119

@@ -34,39 +32,54 @@ const (
3432
annotationInjectNginxContainersName = "instrumentation.opentelemetry.io/inject-nginx-container-names"
3533
)
3634

37-
// annotationValue returns the effective annotationInjectJava value, based on the annotations from the pod and namespace.
38-
func annotationValue(ns metav1.ObjectMeta, pod metav1.ObjectMeta, annotation string) string {
39-
// is the pod annotated with instructions to inject sidecars? is the namespace annotated?
40-
// if any of those is true, a sidecar might be desired.
41-
podAnnValue := pod.Annotations[annotation]
42-
nsAnnValue := ns.Annotations[annotation]
43-
44-
// if the namespace value is empty, the pod annotation should be used, whatever it is
45-
if len(nsAnnValue) == 0 {
46-
return podAnnValue
35+
func hasMatchingInjectAnnotation(annotations map[string]string, containerAnnotation string) (string, bool) {
36+
annotationPairs := map[string]string{
37+
annotationInjectJavaContainersName: annotationInjectJava,
38+
annotationInjectNodeJSContainersName: annotationInjectNodeJS,
39+
annotationInjectPythonContainersName: annotationInjectPython,
40+
annotationInjectDotnetContainersName: annotationInjectDotNet,
41+
annotationInjectGoContainersName: annotationInjectGo,
42+
annotationInjectSdkContainersName: annotationInjectSdk,
43+
annotationInjectApacheHttpdContainersName: annotationInjectApacheHttpd,
44+
annotationInjectNginxContainersName: annotationInjectNginx,
4745
}
4846

49-
// if the pod value is empty, the annotation should be used (true, false, instance)
50-
if len(podAnnValue) == 0 {
51-
return nsAnnValue
47+
injectAnnotation, exists := annotationPairs[containerAnnotation]
48+
if !exists {
49+
return "", false
5250
}
5351

54-
// the pod annotation isn't empty -- if it's an instance name, or false, that's the decision
55-
if !strings.EqualFold(podAnnValue, "true") {
56-
return podAnnValue
52+
_, found := annotations[injectAnnotation]
53+
return injectAnnotation, found
54+
}
55+
56+
// annotationValue returns the effective annotationInjectJava value, based on the annotations from the pod and namespace.
57+
func annotationValue(ns metav1.ObjectMeta, pod metav1.ObjectMeta, annotation string) string {
58+
if injectAnnotation, isContainerNames := hasMatchingInjectAnnotation(pod.Annotations, annotation); isContainerNames {
59+
if _, injectExists := pod.Annotations[injectAnnotation]; injectExists {
60+
if val, exists := pod.Annotations[annotation]; exists && len(val) > 0 {
61+
return val
62+
}
63+
}
5764
}
5865

59-
// pod annotation is 'true', and if the namespace annotation is false, we just return 'true'
60-
if strings.EqualFold(nsAnnValue, "false") {
61-
return podAnnValue
66+
if injectAnnotation, isContainerNames := hasMatchingInjectAnnotation(ns.Annotations, annotation); isContainerNames {
67+
if _, injectExists := ns.Annotations[injectAnnotation]; injectExists {
68+
if val, exists := ns.Annotations[annotation]; exists && len(val) > 0 {
69+
return val
70+
}
71+
}
6272
}
6373

64-
// if both are 'true' or a defined instance-name, pod takes precedence
65-
if !strings.EqualFold(nsAnnValue, "false") && !strings.EqualFold(podAnnValue, "false") {
74+
podAnnValue, podExists := pod.Annotations[annotation]
75+
nsAnnValue, nsExists := ns.Annotations[annotation]
76+
77+
if podExists && len(podAnnValue) > 0 {
6678
return podAnnValue
6779
}
80+
if nsExists && len(nsAnnValue) > 0 {
81+
return nsAnnValue
82+
}
6883

69-
// by now, the pod annotation is 'true', and the namespace annotation is either true or an instance name
70-
// so, the namespace annotation can be used
71-
return nsAnnValue
84+
return ""
7285
}

‎pkg/instrumentation/annotation_test.go

+42-3
Original file line numberDiff line numberDiff line change
@@ -58,18 +58,57 @@ func TestEffectiveAnnotationValue(t *testing.T) {
5858

5959
{
6060
"pod-has-concrete-instance",
61-
"some-instance-from-pod",
61+
"",
6262
corev1.Pod{
6363
ObjectMeta: metav1.ObjectMeta{
6464
Annotations: map[string]string{
65-
annotationInjectJava: "some-instance-from-pod",
65+
annotationInjectJavaContainersName: "some-instance-from-pod",
6666
},
6767
},
6868
},
6969
corev1.Namespace{
7070
ObjectMeta: metav1.ObjectMeta{
7171
Annotations: map[string]string{
72-
annotationInjectJava: "some-instance",
72+
annotationInjectJavaContainersName: "some-instance",
73+
},
74+
},
75+
},
76+
},
77+
78+
{
79+
"pod-has-concrete-instance-and-inject",
80+
"true",
81+
corev1.Pod{
82+
ObjectMeta: metav1.ObjectMeta{
83+
Annotations: map[string]string{
84+
annotationInjectJava: "true",
85+
annotationInjectJavaContainersName: "some-instance-from-pod",
86+
},
87+
},
88+
},
89+
corev1.Namespace{
90+
ObjectMeta: metav1.ObjectMeta{
91+
Annotations: map[string]string{
92+
annotationInjectJavaContainersName: "some-instance",
93+
},
94+
},
95+
},
96+
},
97+
98+
{
99+
"pod-has-concrete-instance-and-ns-sdk",
100+
"true",
101+
corev1.Pod{
102+
ObjectMeta: metav1.ObjectMeta{
103+
Annotations: map[string]string{
104+
annotationInjectJava: "true",
105+
},
106+
},
107+
},
108+
corev1.Namespace{
109+
ObjectMeta: metav1.ObjectMeta{
110+
Annotations: map[string]string{
111+
annotationInjectSdk: "true",
73112
},
74113
},
75114
},

‎pkg/instrumentation/helper_test.go

+5
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,11 @@ func TestDuplicatedContainers(t *testing.T) {
168168
containers: []string{"app1", "app2", "app1", "app1", "app3", "app4", "app4"},
169169
expectedDuplicates: fmt.Errorf("duplicated container names detected: [app1 app4]"),
170170
},
171+
{
172+
name: "No duplicates",
173+
containers: []string{"app1"},
174+
expectedDuplicates: nil,
175+
},
171176
}
172177

173178
for _, test := range tests {

‎pkg/instrumentation/podmutator.go

-44
Original file line numberDiff line numberDiff line change
@@ -52,77 +52,51 @@ type languageInstrumentations struct {
5252
Sdk instrumentationWithContainers
5353
}
5454

55-
// Check if specific containers are provided for configured instrumentation.
5655
func (langInsts languageInstrumentations) areInstrumentedContainersCorrect() (bool, error) {
5756
var instrWithoutContainers int
5857
var instrWithContainers int
5958
var allContainers []string
60-
var instrumentationWithNoContainers bool
6159

6260
// Check for instrumentations with and without containers.
6361
if langInsts.Java.Instrumentation != nil {
6462
instrWithContainers += isInstrWithContainers(langInsts.Java)
6563
instrWithoutContainers += isInstrWithoutContainers(langInsts.Java)
6664
allContainers = append(allContainers, langInsts.Java.Containers...)
67-
if len(langInsts.Java.Containers) == 0 {
68-
instrumentationWithNoContainers = true
69-
}
7065
}
7166
if langInsts.NodeJS.Instrumentation != nil {
7267
instrWithContainers += isInstrWithContainers(langInsts.NodeJS)
7368
instrWithoutContainers += isInstrWithoutContainers(langInsts.NodeJS)
7469
allContainers = append(allContainers, langInsts.NodeJS.Containers...)
75-
if len(langInsts.NodeJS.Containers) == 0 {
76-
instrumentationWithNoContainers = true
77-
}
7870
}
7971
if langInsts.Python.Instrumentation != nil {
8072
instrWithContainers += isInstrWithContainers(langInsts.Python)
8173
instrWithoutContainers += isInstrWithoutContainers(langInsts.Python)
8274
allContainers = append(allContainers, langInsts.Python.Containers...)
83-
if len(langInsts.Python.Containers) == 0 {
84-
instrumentationWithNoContainers = true
85-
}
8675
}
8776
if langInsts.DotNet.Instrumentation != nil {
8877
instrWithContainers += isInstrWithContainers(langInsts.DotNet)
8978
instrWithoutContainers += isInstrWithoutContainers(langInsts.DotNet)
9079
allContainers = append(allContainers, langInsts.DotNet.Containers...)
91-
if len(langInsts.DotNet.Containers) == 0 {
92-
instrumentationWithNoContainers = true
93-
}
9480
}
9581
if langInsts.ApacheHttpd.Instrumentation != nil {
9682
instrWithContainers += isInstrWithContainers(langInsts.ApacheHttpd)
9783
instrWithoutContainers += isInstrWithoutContainers(langInsts.ApacheHttpd)
9884
allContainers = append(allContainers, langInsts.ApacheHttpd.Containers...)
99-
if len(langInsts.ApacheHttpd.Containers) == 0 {
100-
instrumentationWithNoContainers = true
101-
}
10285
}
10386
if langInsts.Nginx.Instrumentation != nil {
10487
instrWithContainers += isInstrWithContainers(langInsts.Nginx)
10588
instrWithoutContainers += isInstrWithoutContainers(langInsts.Nginx)
10689
allContainers = append(allContainers, langInsts.Nginx.Containers...)
107-
if len(langInsts.Nginx.Containers) == 0 {
108-
instrumentationWithNoContainers = true
109-
}
11090
}
11191
if langInsts.Go.Instrumentation != nil {
11292
instrWithContainers += isInstrWithContainers(langInsts.Go)
11393
instrWithoutContainers += isInstrWithoutContainers(langInsts.Go)
11494
allContainers = append(allContainers, langInsts.Go.Containers...)
115-
if len(langInsts.Go.Containers) == 0 {
116-
instrumentationWithNoContainers = true
117-
}
11895
}
11996
if langInsts.Sdk.Instrumentation != nil {
12097
instrWithContainers += isInstrWithContainers(langInsts.Sdk)
12198
instrWithoutContainers += isInstrWithoutContainers(langInsts.Sdk)
12299
allContainers = append(allContainers, langInsts.Sdk.Containers...)
123-
if len(langInsts.Sdk.Containers) == 0 {
124-
instrumentationWithNoContainers = true
125-
}
126100
}
127101

128102
// Look for duplicated containers.
@@ -131,26 +105,10 @@ func (langInsts languageInstrumentations) areInstrumentedContainersCorrect() (bo
131105
return false, containerDuplicates
132106
}
133107

134-
// Look for mixed multiple instrumentations with and without container names.
135-
if instrWithoutContainers > 0 && instrWithContainers > 0 {
136-
return false, fmt.Errorf("incorrect instrumentation configuration - please provide container names for all instrumentations")
137-
}
138-
139-
// Look for multiple instrumentations without container names.
140-
if instrWithoutContainers > 1 && instrWithContainers == 0 {
141-
return false, fmt.Errorf("incorrect instrumentation configuration - please provide container names for all instrumentations")
142-
}
143-
144108
if instrWithoutContainers == 0 && instrWithContainers == 0 {
145109
return false, fmt.Errorf("instrumentation configuration not provided")
146110
}
147111

148-
enabledInstrumentations := instrWithContainers + instrWithoutContainers
149-
150-
if enabledInstrumentations > 1 && instrumentationWithNoContainers {
151-
return false, fmt.Errorf("incorrect instrumentation configuration - please provide container names for all instrumentations")
152-
}
153-
154112
return true, nil
155113
}
156114

@@ -397,8 +355,6 @@ func (pm *instPodMutator) Mutate(ctx context.Context, ns corev1.Namespace, pod c
397355
if err != nil {
398356
return pod, err
399357
}
400-
401-
// We check if provided annotations and instrumentations are valid
402358
ok, msg := insts.areInstrumentedContainersCorrect()
403359
if !ok {
404360
logger.V(1).Error(msg, "skipping instrumentation injection")

‎pkg/instrumentation/podmutator_test.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -4986,17 +4986,17 @@ func TestContainerNamesConfiguredForMultipleInstrumentations(t *testing.T) {
49864986
Java: instrumentationWithContainers{Instrumentation: &v1alpha1.Instrumentation{}},
49874987
NodeJS: instrumentationWithContainers{Instrumentation: &v1alpha1.Instrumentation{}},
49884988
},
4989-
expectedStatus: false,
4990-
expectedMsg: fmt.Errorf("incorrect instrumentation configuration - please provide container names for all instrumentations"),
4989+
expectedStatus: true,
4990+
expectedMsg: nil,
49914991
},
49924992
{
49934993
name: "Multiple instrumentations enabled with containers for single instrumentation",
49944994
instrumentations: languageInstrumentations{
49954995
Java: instrumentationWithContainers{Instrumentation: &v1alpha1.Instrumentation{}, Containers: []string{"test"}},
49964996
NodeJS: instrumentationWithContainers{Instrumentation: &v1alpha1.Instrumentation{}},
49974997
},
4998-
expectedStatus: false,
4999-
expectedMsg: fmt.Errorf("incorrect instrumentation configuration - please provide container names for all instrumentations"),
4998+
expectedStatus: true,
4999+
expectedMsg: nil,
50005000
},
50015001
{
50025002
name: "Disabled instrumentations",

‎pkg/instrumentation/sdk.go

+25-18
Original file line numberDiff line numberDiff line change
@@ -296,15 +296,19 @@ func (i *sdkInjector) injectCommonSDKConfig(ctx context.Context, otelinst v1alph
296296
configureExporter(otelinst.Spec.Exporter, &pod, container)
297297

298298
// Always retrieve the pod name from the Downward API. Ensure that the OTEL_RESOURCE_ATTRIBUTES_POD_NAME env exists.
299-
container.Env = append(container.Env, corev1.EnvVar{
300-
Name: constants.EnvPodName,
301-
ValueFrom: &corev1.EnvVarSource{
302-
FieldRef: &corev1.ObjectFieldSelector{
303-
FieldPath: "metadata.name",
299+
// Added this check because we add this variable when using sidecar.
300+
idx = getIndexOfEnv(container.Env, constants.EnvPodName)
301+
if idx == -1 {
302+
container.Env = append(container.Env, corev1.EnvVar{
303+
Name: constants.EnvPodName,
304+
ValueFrom: &corev1.EnvVarSource{
305+
FieldRef: &corev1.ObjectFieldSelector{
306+
FieldPath: "metadata.name",
307+
},
304308
},
305-
},
306-
})
307-
resourceMap[string(semconv.K8SPodNameKey)] = fmt.Sprintf("$(%s)", constants.EnvPodName)
309+
})
310+
resourceMap[string(semconv.K8SPodNameKey)] = fmt.Sprintf("$(%s)", constants.EnvPodName)
311+
}
308312

309313
// Some attributes might be empty, we should get them via k8s downward API
310314
if otelinst.Spec.Resource.AddK8sUIDAttributes {
@@ -328,17 +332,20 @@ func (i *sdkInjector) injectCommonSDKConfig(ctx context.Context, otelinst v1alph
328332
resourceMap[string(semconv.ServiceVersionKey)] = vsn
329333
}
330334
}
331-
332-
if resourceMap[string(semconv.K8SNodeNameKey)] == "" {
333-
container.Env = append(container.Env, corev1.EnvVar{
334-
Name: constants.EnvNodeName,
335-
ValueFrom: &corev1.EnvVarSource{
336-
FieldRef: &corev1.ObjectFieldSelector{
337-
FieldPath: "spec.nodeName",
335+
// Added this check because we add this variable when using sidecar.
336+
idx = getIndexOfEnv(container.Env, constants.EnvNodeName)
337+
if idx == -1 {
338+
if resourceMap[string(semconv.K8SNodeNameKey)] == "" {
339+
container.Env = append(container.Env, corev1.EnvVar{
340+
Name: constants.EnvNodeName,
341+
ValueFrom: &corev1.EnvVarSource{
342+
FieldRef: &corev1.ObjectFieldSelector{
343+
FieldPath: "spec.nodeName",
344+
},
338345
},
339-
},
340-
})
341-
resourceMap[string(semconv.K8SNodeNameKey)] = fmt.Sprintf("$(%s)", constants.EnvNodeName)
346+
})
347+
resourceMap[string(semconv.K8SNodeNameKey)] = fmt.Sprintf("$(%s)", constants.EnvNodeName)
348+
}
342349
}
343350

344351
idx = getIndexOfEnv(container.Env, constants.EnvOTELResourceAttrs)

‎tests/e2e-instrumentation/instrumentation-dotnet-multiannotation/00-install-collector.yaml ‎tests/e2e-instrumentation/instrumentation-python-multiannotation/00-install-collector.yaml

+7-4
Original file line numberDiff line numberDiff line change
@@ -2,30 +2,33 @@
22
apiVersion: v1
33
kind: Namespace
44
metadata:
5-
name: my-other-dotnet-ns
5+
name: my-other-python-ns
66
annotations:
77
instrumentation.opentelemetry.io/inject-sdk: "true"
88
---
99
apiVersion: opentelemetry.io/v1beta1
1010
kind: OpenTelemetryCollector
1111
metadata:
12-
name: sidecar
13-
namespace: my-other-dotnet-ns
12+
name: sidecar-teste
13+
namespace: my-other-python-ns
1414
spec:
1515
config:
1616
receivers:
1717
otlp:
1818
protocols:
1919
grpc:
20+
endpoint: 0.0.0.0:4317
2021
http:
22+
endpoint: 0.0.0.0:4318
2123
processors:
2224

2325
exporters:
24-
debug:
26+
debug: {}
2527

2628
service:
2729
pipelines:
2830
traces:
2931
receivers: [otlp]
3032
exporters: [debug]
3133
mode: sidecar
34+
upgradeStrategy: automatic

0 commit comments

Comments
 (0)