Skip to content

Commit

Permalink
Not ignore the container-names annotation when the multi-instrumentat…
Browse files Browse the repository at this point in the history 
…ion is enabled

Signed-off-by: Israel Blancas <iblancasa@gmail.com>
  • Loading branch information
@iblancasa
iblancasa committed Aug 21, 2024
1 parent a42426d commit 8989fc3
Show file tree
Hide file tree
Showing 12 changed files with 455 additions and 982 deletions.
16 changes: 16 additions & 0 deletions .chloggen/feature_3090.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: 'enhancement'

# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
component: auto-instrumentation

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Not ignore the `instrumentation.opentelemetry.io/container-names` annotation when the multi-instrumentation is enabled"

# One or more tracking issues related to the change
issues: [3090]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:
39 changes: 30 additions & 9 deletions pkg/instrumentation/helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@ package instrumentation

import (
"fmt"
"regexp"
"sort"
"strings"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/strings/slices"

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

// Look for duplicates in the provided containers.
func findDuplicatedContainers(ctrs []string) error {
// Merge is needed because of multiple containers can be provided for single instrumentation.
mergedContainers := strings.Join(ctrs, ",")

// Split all containers.
splitContainers := strings.Split(mergedContainers, ",")

countMap := make(map[string]int)
var duplicates []string
for _, str := range splitContainers {
for _, str := range ctrs {
countMap[str]++
}

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

// Return positive for instrumentation with defined containers.
func isInstrWithContainers(inst instrumentationWithContainers) int {
if inst.Containers != "" {
if len(inst.Containers) > 0 {
return 1
}

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

// Return positive for instrumentation without defined containers.
func isInstrWithoutContainers(inst instrumentationWithContainers) int {
if inst.Containers == "" {
if len(inst.Containers) == 0 {
return 1
}

Expand All @@ -133,3 +129,28 @@ func volumeSize(quantity *resource.Quantity) *resource.Quantity {
}
return quantity
}

func isValidContainersAnnotation(containersAnnotation string) error {
if containersAnnotation == "" {
return nil
}

matched, err := regexp.MatchString("^[a-zA-Z0-9-,]+$", containersAnnotation)
if err != nil {
return fmt.Errorf("error while checking for instrumentation container annotations %w", err)
}
if !matched {
return fmt.Errorf("not valid characters included in the instrumentation container annotation %s", containersAnnotation)
}
return nil
}

func configureLanguageContainers(inst *instrumentationWithContainers, annotation string, ns metav1.ObjectMeta, pod metav1.ObjectMeta) error {
annotationValue := annotationValue(ns, pod, annotation)
if err := isValidContainersAnnotation(annotationValue); err != nil {
return err
}
languageContainers := strings.Split(annotationValue, ",")
inst.Containers = append(inst.Containers, languageContainers...)
return nil
}
12 changes: 6 additions & 6 deletions pkg/instrumentation/helper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,12 @@ func TestDuplicatedContainers(t *testing.T) {
}{
{
name: "No duplicates",
containers: []string{"app1,app2", "app3", "app4,app5"},
containers: []string{"app1", "app2", "app3", "app4", "app5"},
expectedDuplicates: nil,
},
{
name: "Duplicates in containers",
containers: []string{"app1,app2", "app1", "app1,app3,app4", "app4"},
containers: []string{"app1", "app2", "app1", "app1", "app3", "app4", "app4"},
expectedDuplicates: fmt.Errorf("duplicated container names detected: [app1 app4]"),
},
}
Expand All @@ -196,12 +196,12 @@ func TestInstrWithContainers(t *testing.T) {
}{
{
name: "No containers",
containers: instrumentationWithContainers{Containers: ""},
containers: instrumentationWithContainers{Containers: []string{}},
expectedResult: 0,
},
{
name: "With containers",
containers: instrumentationWithContainers{Containers: "ct1"},
containers: instrumentationWithContainers{Containers: []string{"ct1"}},
expectedResult: 1,
},
}
Expand All @@ -222,12 +222,12 @@ func TestInstrWithoutContainers(t *testing.T) {
}{
{
name: "No containers",
containers: instrumentationWithContainers{Containers: ""},
containers: instrumentationWithContainers{Containers: []string{}},
expectedResult: 1,
},
{
name: "With containers",
containers: instrumentationWithContainers{Containers: "ct1"},
containers: instrumentationWithContainers{Containers: []string{"ct1"}},
expectedResult: 0,
},
}
Expand Down
125 changes: 62 additions & 63 deletions pkg/instrumentation/podmutator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

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

type instrumentationWithContainers struct {
Instrumentation *v1alpha1.Instrumentation
Containers string
Containers []string
AdditionalAnnotations map[string]string
}

Expand All @@ -61,38 +62,6 @@ type languageInstrumentations struct {
Sdk instrumentationWithContainers
}

// Check if single instrumentation is configured for Pod and return which is configured.
func (langInsts languageInstrumentations) isSingleInstrumentationEnabled() bool {
count := 0

if langInsts.Java.Instrumentation != nil {
count++
}
if langInsts.NodeJS.Instrumentation != nil {
count++
}
if langInsts.Python.Instrumentation != nil {
count++
}
if langInsts.DotNet.Instrumentation != nil {
count++
}
if langInsts.ApacheHttpd.Instrumentation != nil {
count++
}
if langInsts.Nginx.Instrumentation != nil {
count++
}
if langInsts.Go.Instrumentation != nil {
count++
}
if langInsts.Sdk.Instrumentation != nil {
count++
}

return count == 1
}

// Check if specific containers are provided for configured instrumentation.
func (langInsts languageInstrumentations) areContainerNamesConfiguredForMultipleInstrumentations() (bool, error) {
var instrWithoutContainers int
Expand All @@ -103,42 +72,42 @@ func (langInsts languageInstrumentations) areContainerNamesConfiguredForMultiple
if langInsts.Java.Instrumentation != nil {
instrWithContainers += isInstrWithContainers(langInsts.Java)
instrWithoutContainers += isInstrWithoutContainers(langInsts.Java)
allContainers = append(allContainers, langInsts.Java.Containers)
allContainers = append(allContainers, langInsts.Java.Containers...)
}
if langInsts.NodeJS.Instrumentation != nil {
instrWithContainers += isInstrWithContainers(langInsts.NodeJS)
instrWithoutContainers += isInstrWithoutContainers(langInsts.NodeJS)
allContainers = append(allContainers, langInsts.NodeJS.Containers)
allContainers = append(allContainers, langInsts.NodeJS.Containers...)
}
if langInsts.Python.Instrumentation != nil {
instrWithContainers += isInstrWithContainers(langInsts.Python)
instrWithoutContainers += isInstrWithoutContainers(langInsts.Python)
allContainers = append(allContainers, langInsts.Python.Containers)
allContainers = append(allContainers, langInsts.Python.Containers...)
}
if langInsts.DotNet.Instrumentation != nil {
instrWithContainers += isInstrWithContainers(langInsts.DotNet)
instrWithoutContainers += isInstrWithoutContainers(langInsts.DotNet)
allContainers = append(allContainers, langInsts.DotNet.Containers)
allContainers = append(allContainers, langInsts.DotNet.Containers...)
}
if langInsts.ApacheHttpd.Instrumentation != nil {
instrWithContainers += isInstrWithContainers(langInsts.ApacheHttpd)
instrWithoutContainers += isInstrWithoutContainers(langInsts.ApacheHttpd)
allContainers = append(allContainers, langInsts.ApacheHttpd.Containers)
allContainers = append(allContainers, langInsts.ApacheHttpd.Containers...)
}
if langInsts.Nginx.Instrumentation != nil {
instrWithContainers += isInstrWithContainers(langInsts.Nginx)
instrWithoutContainers += isInstrWithoutContainers(langInsts.Nginx)
allContainers = append(allContainers, langInsts.Nginx.Containers)
allContainers = append(allContainers, langInsts.Nginx.Containers...)
}
if langInsts.Go.Instrumentation != nil {
instrWithContainers += isInstrWithContainers(langInsts.Go)
instrWithoutContainers += isInstrWithoutContainers(langInsts.Go)
allContainers = append(allContainers, langInsts.Go.Containers)
allContainers = append(allContainers, langInsts.Go.Containers...)
}
if langInsts.Sdk.Instrumentation != nil {
instrWithContainers += isInstrWithContainers(langInsts.Sdk)
instrWithoutContainers += isInstrWithoutContainers(langInsts.Sdk)
allContainers = append(allContainers, langInsts.Sdk.Containers)
allContainers = append(allContainers, langInsts.Sdk.Containers...)
}

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

// Set containers for configured instrumentation.
func (langInsts *languageInstrumentations) setInstrumentationLanguageContainers(containers string) {
func (langInsts *languageInstrumentations) setCommonInstrumentedContainers(ns corev1.Namespace, pod corev1.Pod) error {
containersAnnotation := annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectContainerName)
if err := isValidContainersAnnotation(containersAnnotation); err != nil {
return err
}

var containers []string
if containersAnnotation == "" {
return nil
} else {
containers = strings.Split(containersAnnotation, ",")
}

if langInsts.Java.Instrumentation != nil {
langInsts.Java.Containers = containers
}

if langInsts.NodeJS.Instrumentation != nil {
langInsts.NodeJS.Containers = containers
}
Expand All @@ -190,6 +172,35 @@ func (langInsts *languageInstrumentations) setInstrumentationLanguageContainers(
if langInsts.Sdk.Instrumentation != nil {
langInsts.Sdk.Containers = containers
}
return nil
}

func (langInsts *languageInstrumentations) setLanguageSpecificContainers(ns metav1.ObjectMeta, pod metav1.ObjectMeta) error {
if err := configureLanguageContainers(&langInsts.Java, annotationInjectJavaContainersName, ns, pod); err != nil {
return err
}
if err := configureLanguageContainers(&langInsts.NodeJS, annotationInjectNodeJSContainersName, ns, pod); err != nil {
return err
}
if err := configureLanguageContainers(&langInsts.Python, annotationInjectPythonContainersName, ns, pod); err != nil {
return err
}
if err := configureLanguageContainers(&langInsts.DotNet, annotationInjectDotnetContainersName, ns, pod); err != nil {
return err
}
if err := configureLanguageContainers(&langInsts.Go, annotationInjectGoContainersName, ns, pod); err != nil {
return err
}
if err := configureLanguageContainers(&langInsts.ApacheHttpd, annotationInjectNginxContainersName, ns, pod); err != nil {
return err
}
if err := configureLanguageContainers(&langInsts.Nginx, annotationInjectJavaContainersName, ns, pod); err != nil {
return err
}
if err := configureLanguageContainers(&langInsts.Sdk, annotationInjectSdkContainersName, ns, pod); err != nil {
return err
}
return nil
}

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

err = insts.setCommonInstrumentedContainers(ns, pod)
if err != nil {
return pod, err
}

// We retrieve the annotation for podname
if pm.config.EnableMultiInstrumentation() {
// We use annotations specific for instrumentation language
insts.Java.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectJavaContainersName)
insts.NodeJS.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectNodeJSContainersName)
insts.Python.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectPythonContainersName)
insts.DotNet.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectDotnetContainersName)
insts.Go.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectGoContainersName)
insts.ApacheHttpd.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectApacheHttpdContainersName)
insts.Nginx.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectNginxContainersName)
insts.Sdk.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectSdkContainersName)
err = insts.setLanguageSpecificContainers(ns.ObjectMeta, pod.ObjectMeta)
if err != nil {
return pod, err
}

// We check if provided annotations and instrumentations are valid
ok, msg := insts.areContainerNamesConfiguredForMultipleInstrumentations()
if !ok {
logger.V(1).Error(msg, "skipping instrumentation injection")
return pod, nil
}
} else {
// We use general annotation for container names
// only when multi instrumentation is disabled
singleInstrEnabled := insts.isSingleInstrumentationEnabled()
if singleInstrEnabled {
generalContainerNames := annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectContainerName)
insts.setInstrumentationLanguageContainers(generalContainerNames)
} else {
logger.V(1).Error(fmt.Errorf("multiple injection annotations present"), "skipping instrumentation injection")
return pod, nil
}

}

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

0 comments on commit 8989fc3

Please sign in to comment.