Skip to content

Commit 8989fc3

Browse files
iblancasaiblancasa
committed
Not ignore the container-names annotation when the multi-instrumentation is enabled
Signed-off-by: Israel Blancas <[email protected]>
1 parent a42426d commit 8989fc3

File tree

12 files changed

+455
-982
lines changed

12 files changed

+455
-982
lines changed

‎.chloggen/feature_3090.yaml

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
2+
change_type: 'enhancement'
3+
4+
# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
5+
component: auto-instrumentation
6+
7+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
8+
note: "Not ignore the `instrumentation.opentelemetry.io/container-names` annotation when the multi-instrumentation is enabled"
9+
10+
# One or more tracking issues related to the change
11+
issues: [3090]
12+
13+
# (Optional) One or more lines of additional information to render under the primary note.
14+
# These lines will be padded with 2 spaces and then inserted directly into the document.
15+
# Use pipe (|) for multiline entries.
16+
subtext:

‎pkg/instrumentation/helper.go

+30-9
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,13 @@ package instrumentation
1616

1717
import (
1818
"fmt"
19+
"regexp"
1920
"sort"
2021
"strings"
2122

2223
corev1 "k8s.io/api/core/v1"
2324
"k8s.io/apimachinery/pkg/api/resource"
25+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2426
"k8s.io/utils/strings/slices"
2527

2628
"github.com/open-telemetry/opentelemetry-operator/internal/naming"
@@ -77,15 +79,9 @@ func isAutoInstrumentationInjected(pod corev1.Pod) bool {
7779

7880
// Look for duplicates in the provided containers.
7981
func findDuplicatedContainers(ctrs []string) error {
80-
// Merge is needed because of multiple containers can be provided for single instrumentation.
81-
mergedContainers := strings.Join(ctrs, ",")
82-
83-
// Split all containers.
84-
splitContainers := strings.Split(mergedContainers, ",")
85-
8682
countMap := make(map[string]int)
8783
var duplicates []string
88-
for _, str := range splitContainers {
84+
for _, str := range ctrs {
8985
countMap[str]++
9086
}
9187

@@ -111,7 +107,7 @@ func findDuplicatedContainers(ctrs []string) error {
111107

112108
// Return positive for instrumentation with defined containers.
113109
func isInstrWithContainers(inst instrumentationWithContainers) int {
114-
if inst.Containers != "" {
110+
if len(inst.Containers) > 0 {
115111
return 1
116112
}
117113

@@ -120,7 +116,7 @@ func isInstrWithContainers(inst instrumentationWithContainers) int {
120116

121117
// Return positive for instrumentation without defined containers.
122118
func isInstrWithoutContainers(inst instrumentationWithContainers) int {
123-
if inst.Containers == "" {
119+
if len(inst.Containers) == 0 {
124120
return 1
125121
}
126122

@@ -133,3 +129,28 @@ func volumeSize(quantity *resource.Quantity) *resource.Quantity {
133129
}
134130
return quantity
135131
}
132+
133+
func isValidContainersAnnotation(containersAnnotation string) error {
134+
if containersAnnotation == "" {
135+
return nil
136+
}
137+
138+
matched, err := regexp.MatchString("^[a-zA-Z0-9-,]+$", containersAnnotation)
139+
if err != nil {
140+
return fmt.Errorf("error while checking for instrumentation container annotations %w", err)
141+
}
142+
if !matched {
143+
return fmt.Errorf("not valid characters included in the instrumentation container annotation %s", containersAnnotation)
144+
}
145+
return nil
146+
}
147+
148+
func configureLanguageContainers(inst *instrumentationWithContainers, annotation string, ns metav1.ObjectMeta, pod metav1.ObjectMeta) error {
149+
annotationValue := annotationValue(ns, pod, annotation)
150+
if err := isValidContainersAnnotation(annotationValue); err != nil {
151+
return err
152+
}
153+
languageContainers := strings.Split(annotationValue, ",")
154+
inst.Containers = append(inst.Containers, languageContainers...)
155+
return nil
156+
}

‎pkg/instrumentation/helper_test.go

+6-6
Original file line numberDiff line numberDiff line change
@@ -170,12 +170,12 @@ func TestDuplicatedContainers(t *testing.T) {
170170
}{
171171
{
172172
name: "No duplicates",
173-
containers: []string{"app1,app2", "app3", "app4,app5"},
173+
containers: []string{"app1", "app2", "app3", "app4", "app5"},
174174
expectedDuplicates: nil,
175175
},
176176
{
177177
name: "Duplicates in containers",
178-
containers: []string{"app1,app2", "app1", "app1,app3,app4", "app4"},
178+
containers: []string{"app1", "app2", "app1", "app1", "app3", "app4", "app4"},
179179
expectedDuplicates: fmt.Errorf("duplicated container names detected: [app1 app4]"),
180180
},
181181
}
@@ -196,12 +196,12 @@ func TestInstrWithContainers(t *testing.T) {
196196
}{
197197
{
198198
name: "No containers",
199-
containers: instrumentationWithContainers{Containers: ""},
199+
containers: instrumentationWithContainers{Containers: []string{}},
200200
expectedResult: 0,
201201
},
202202
{
203203
name: "With containers",
204-
containers: instrumentationWithContainers{Containers: "ct1"},
204+
containers: instrumentationWithContainers{Containers: []string{"ct1"}},
205205
expectedResult: 1,
206206
},
207207
}
@@ -222,12 +222,12 @@ func TestInstrWithoutContainers(t *testing.T) {
222222
}{
223223
{
224224
name: "No containers",
225-
containers: instrumentationWithContainers{Containers: ""},
225+
containers: instrumentationWithContainers{Containers: []string{}},
226226
expectedResult: 1,
227227
},
228228
{
229229
name: "With containers",
230-
containers: instrumentationWithContainers{Containers: "ct1"},
230+
containers: instrumentationWithContainers{Containers: []string{"ct1"}},
231231
expectedResult: 0,
232232
},
233233
}

‎pkg/instrumentation/podmutator.go

+62-63
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222

2323
"github.com/go-logr/logr"
2424
corev1 "k8s.io/api/core/v1"
25+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2526
"k8s.io/apimachinery/pkg/types"
2627
"k8s.io/client-go/tools/record"
2728
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -46,7 +47,7 @@ type instPodMutator struct {
4647

4748
type instrumentationWithContainers struct {
4849
Instrumentation *v1alpha1.Instrumentation
49-
Containers string
50+
Containers []string
5051
AdditionalAnnotations map[string]string
5152
}
5253

@@ -61,38 +62,6 @@ type languageInstrumentations struct {
6162
Sdk instrumentationWithContainers
6263
}
6364

64-
// Check if single instrumentation is configured for Pod and return which is configured.
65-
func (langInsts languageInstrumentations) isSingleInstrumentationEnabled() bool {
66-
count := 0
67-
68-
if langInsts.Java.Instrumentation != nil {
69-
count++
70-
}
71-
if langInsts.NodeJS.Instrumentation != nil {
72-
count++
73-
}
74-
if langInsts.Python.Instrumentation != nil {
75-
count++
76-
}
77-
if langInsts.DotNet.Instrumentation != nil {
78-
count++
79-
}
80-
if langInsts.ApacheHttpd.Instrumentation != nil {
81-
count++
82-
}
83-
if langInsts.Nginx.Instrumentation != nil {
84-
count++
85-
}
86-
if langInsts.Go.Instrumentation != nil {
87-
count++
88-
}
89-
if langInsts.Sdk.Instrumentation != nil {
90-
count++
91-
}
92-
93-
return count == 1
94-
}
95-
9665
// Check if specific containers are provided for configured instrumentation.
9766
func (langInsts languageInstrumentations) areContainerNamesConfiguredForMultipleInstrumentations() (bool, error) {
9867
var instrWithoutContainers int
@@ -103,42 +72,42 @@ func (langInsts languageInstrumentations) areContainerNamesConfiguredForMultiple
10372
if langInsts.Java.Instrumentation != nil {
10473
instrWithContainers += isInstrWithContainers(langInsts.Java)
10574
instrWithoutContainers += isInstrWithoutContainers(langInsts.Java)
106-
allContainers = append(allContainers, langInsts.Java.Containers)
75+
allContainers = append(allContainers, langInsts.Java.Containers...)
10776
}
10877
if langInsts.NodeJS.Instrumentation != nil {
10978
instrWithContainers += isInstrWithContainers(langInsts.NodeJS)
11079
instrWithoutContainers += isInstrWithoutContainers(langInsts.NodeJS)
111-
allContainers = append(allContainers, langInsts.NodeJS.Containers)
80+
allContainers = append(allContainers, langInsts.NodeJS.Containers...)
11281
}
11382
if langInsts.Python.Instrumentation != nil {
11483
instrWithContainers += isInstrWithContainers(langInsts.Python)
11584
instrWithoutContainers += isInstrWithoutContainers(langInsts.Python)
116-
allContainers = append(allContainers, langInsts.Python.Containers)
85+
allContainers = append(allContainers, langInsts.Python.Containers...)
11786
}
11887
if langInsts.DotNet.Instrumentation != nil {
11988
instrWithContainers += isInstrWithContainers(langInsts.DotNet)
12089
instrWithoutContainers += isInstrWithoutContainers(langInsts.DotNet)
121-
allContainers = append(allContainers, langInsts.DotNet.Containers)
90+
allContainers = append(allContainers, langInsts.DotNet.Containers...)
12291
}
12392
if langInsts.ApacheHttpd.Instrumentation != nil {
12493
instrWithContainers += isInstrWithContainers(langInsts.ApacheHttpd)
12594
instrWithoutContainers += isInstrWithoutContainers(langInsts.ApacheHttpd)
126-
allContainers = append(allContainers, langInsts.ApacheHttpd.Containers)
95+
allContainers = append(allContainers, langInsts.ApacheHttpd.Containers...)
12796
}
12897
if langInsts.Nginx.Instrumentation != nil {
12998
instrWithContainers += isInstrWithContainers(langInsts.Nginx)
13099
instrWithoutContainers += isInstrWithoutContainers(langInsts.Nginx)
131-
allContainers = append(allContainers, langInsts.Nginx.Containers)
100+
allContainers = append(allContainers, langInsts.Nginx.Containers...)
132101
}
133102
if langInsts.Go.Instrumentation != nil {
134103
instrWithContainers += isInstrWithContainers(langInsts.Go)
135104
instrWithoutContainers += isInstrWithoutContainers(langInsts.Go)
136-
allContainers = append(allContainers, langInsts.Go.Containers)
105+
allContainers = append(allContainers, langInsts.Go.Containers...)
137106
}
138107
if langInsts.Sdk.Instrumentation != nil {
139108
instrWithContainers += isInstrWithContainers(langInsts.Sdk)
140109
instrWithoutContainers += isInstrWithoutContainers(langInsts.Sdk)
141-
allContainers = append(allContainers, langInsts.Sdk.Containers)
110+
allContainers = append(allContainers, langInsts.Sdk.Containers...)
142111
}
143112

144113
// Look for duplicated containers.
@@ -165,10 +134,23 @@ func (langInsts languageInstrumentations) areContainerNamesConfiguredForMultiple
165134
}
166135

167136
// Set containers for configured instrumentation.
168-
func (langInsts *languageInstrumentations) setInstrumentationLanguageContainers(containers string) {
137+
func (langInsts *languageInstrumentations) setCommonInstrumentedContainers(ns corev1.Namespace, pod corev1.Pod) error {
138+
containersAnnotation := annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectContainerName)
139+
if err := isValidContainersAnnotation(containersAnnotation); err != nil {
140+
return err
141+
}
142+
143+
var containers []string
144+
if containersAnnotation == "" {
145+
return nil
146+
} else {
147+
containers = strings.Split(containersAnnotation, ",")
148+
}
149+
169150
if langInsts.Java.Instrumentation != nil {
170151
langInsts.Java.Containers = containers
171152
}
153+
172154
if langInsts.NodeJS.Instrumentation != nil {
173155
langInsts.NodeJS.Containers = containers
174156
}
@@ -190,6 +172,35 @@ func (langInsts *languageInstrumentations) setInstrumentationLanguageContainers(
190172
if langInsts.Sdk.Instrumentation != nil {
191173
langInsts.Sdk.Containers = containers
192174
}
175+
return nil
176+
}
177+
178+
func (langInsts *languageInstrumentations) setLanguageSpecificContainers(ns metav1.ObjectMeta, pod metav1.ObjectMeta) error {
179+
if err := configureLanguageContainers(&langInsts.Java, annotationInjectJavaContainersName, ns, pod); err != nil {
180+
return err
181+
}
182+
if err := configureLanguageContainers(&langInsts.NodeJS, annotationInjectNodeJSContainersName, ns, pod); err != nil {
183+
return err
184+
}
185+
if err := configureLanguageContainers(&langInsts.Python, annotationInjectPythonContainersName, ns, pod); err != nil {
186+
return err
187+
}
188+
if err := configureLanguageContainers(&langInsts.DotNet, annotationInjectDotnetContainersName, ns, pod); err != nil {
189+
return err
190+
}
191+
if err := configureLanguageContainers(&langInsts.Go, annotationInjectGoContainersName, ns, pod); err != nil {
192+
return err
193+
}
194+
if err := configureLanguageContainers(&langInsts.ApacheHttpd, annotationInjectNginxContainersName, ns, pod); err != nil {
195+
return err
196+
}
197+
if err := configureLanguageContainers(&langInsts.Nginx, annotationInjectJavaContainersName, ns, pod); err != nil {
198+
return err
199+
}
200+
if err := configureLanguageContainers(&langInsts.Sdk, annotationInjectSdkContainersName, ns, pod); err != nil {
201+
return err
202+
}
203+
return nil
193204
}
194205

195206
var _ podmutation.PodMutator = (*instPodMutator)(nil)
@@ -329,36 +340,24 @@ func (pm *instPodMutator) Mutate(ctx context.Context, ns corev1.Namespace, pod c
329340
return pod, nil
330341
}
331342

343+
err = insts.setCommonInstrumentedContainers(ns, pod)
344+
if err != nil {
345+
return pod, err
346+
}
347+
332348
// We retrieve the annotation for podname
333349
if pm.config.EnableMultiInstrumentation() {
334-
// We use annotations specific for instrumentation language
335-
insts.Java.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectJavaContainersName)
336-
insts.NodeJS.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectNodeJSContainersName)
337-
insts.Python.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectPythonContainersName)
338-
insts.DotNet.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectDotnetContainersName)
339-
insts.Go.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectGoContainersName)
340-
insts.ApacheHttpd.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectApacheHttpdContainersName)
341-
insts.Nginx.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectNginxContainersName)
342-
insts.Sdk.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectSdkContainersName)
350+
err = insts.setLanguageSpecificContainers(ns.ObjectMeta, pod.ObjectMeta)
351+
if err != nil {
352+
return pod, err
353+
}
343354

344355
// We check if provided annotations and instrumentations are valid
345356
ok, msg := insts.areContainerNamesConfiguredForMultipleInstrumentations()
346357
if !ok {
347358
logger.V(1).Error(msg, "skipping instrumentation injection")
348359
return pod, nil
349360
}
350-
} else {
351-
// We use general annotation for container names
352-
// only when multi instrumentation is disabled
353-
singleInstrEnabled := insts.isSingleInstrumentationEnabled()
354-
if singleInstrEnabled {
355-
generalContainerNames := annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectContainerName)
356-
insts.setInstrumentationLanguageContainers(generalContainerNames)
357-
} else {
358-
logger.V(1).Error(fmt.Errorf("multiple injection annotations present"), "skipping instrumentation injection")
359-
return pod, nil
360-
}
361-
362361
}
363362

364363
// once it's been determined that instrumentation is desired, none exists yet, and we know which instance it should talk to,

0 commit comments

Comments
 (0)