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

[CONTP-403] Configure agent sidecar mutator to use config component #29955

Merged
merged 2 commits into from
Oct 8, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func (c *controllerBase) generateWebhooks(wmeta workloadmeta.Component, pa workl
mutatingWebhooks = []Webhook{
configWebhook.NewWebhook(wmeta, injectionFilter, datadogConfig),
tagsfromlabels.NewWebhook(wmeta, injectionFilter),
agentsidecar.NewWebhook(),
agentsidecar.NewWebhook(datadogConfig),
autoscaling.NewWebhook(pa),
}
webhooks = append(webhooks, mutatingWebhooks...)
Expand Down
39 changes: 21 additions & 18 deletions pkg/clusteragent/admission/mutate/agent_sidecar/agent_sidecar.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"k8s.io/client-go/dynamic"

"github.com/DataDog/datadog-agent/cmd/cluster-agent/admission"
"github.com/DataDog/datadog-agent/comp/core/config"
"github.com/DataDog/datadog-agent/pkg/clusteragent/admission/common"
"github.com/DataDog/datadog-agent/pkg/clusteragent/admission/metrics"
mutatecommon "github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/common"
Expand Down Expand Up @@ -52,23 +53,25 @@ type Webhook struct {
namespaceSelector *metav1.LabelSelector
objectSelector *metav1.LabelSelector
containerRegistry string
datadogConfig config.Component
}

// NewWebhook returns a new Webhook
func NewWebhook() *Webhook {
nsSelector, objSelector := labelSelectors()
func NewWebhook(datadogConfig config.Component) *Webhook {
nsSelector, objSelector := labelSelectors(datadogConfig)

containerRegistry := mutatecommon.ContainerRegistry("admission_controller.agent_sidecar.container_registry")

return &Webhook{
name: webhookName,
isEnabled: pkgconfigsetup.Datadog().GetBool("admission_controller.agent_sidecar.enabled"),
endpoint: pkgconfigsetup.Datadog().GetString("admission_controller.agent_sidecar.endpoint"),
isEnabled: datadogConfig.GetBool("admission_controller.agent_sidecar.enabled"),
endpoint: datadogConfig.GetString("admission_controller.agent_sidecar.endpoint"),
resources: []string{"pods"},
operations: []admissionregistrationv1.OperationType{admissionregistrationv1.Create},
namespaceSelector: nsSelector,
objectSelector: objSelector,
containerRegistry: containerRegistry,
datadogConfig: datadogConfig,
Copy link
Contributor

@clamoriniere clamoriniere Oct 8, 2024

Choose a reason for hiding this comment

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

It is great to now have access to the config.Component in the newWebhook() function. However, instead of keeping a "pointer" to the config in the Webhook struct to get config option values later in other functions, all the values should be extracted in this function and be stored in this Webhook struct.

I will give more details in next comments

}
}

Expand Down Expand Up @@ -129,12 +132,12 @@ func (w *Webhook) injectAgentSidecar(pod *corev1.Pod, _ string, _ dynamic.Interf
podUpdated := false

if !agentSidecarExists {
agentSidecarContainer := getDefaultSidecarTemplate(w.containerRegistry)
agentSidecarContainer := getDefaultSidecarTemplate(w.containerRegistry, w.datadogConfig)
Copy link
Contributor

@clamoriniere clamoriniere Oct 8, 2024

Choose a reason for hiding this comment

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

the injectAgentSidecar() is part of the hot execution path: each time we receive a request we run this function. That is why we should not call the config.Component function in this function because retrieving config option value is costly.
For example we made this fix recently because we introduced a function that was using a config.Component function in a hot execution path. (see the fix: #29211)

So my suggestion is to make the getDefaultSidecarTemplate() function attached to the Webhook struct like injectAgentSidecar() and in the NewWebhook() call the the function datadogConfig.GetString("admission_controller.agent_sidecar.image_name") and store the value in Webhook struct.

and do it for any other option value that we get with datadogConfig.GetString(...).

pod.Spec.Containers = append(pod.Spec.Containers, *agentSidecarContainer)
podUpdated = true
}

updated, err := applyProviderOverrides(pod)
updated, err := applyProviderOverrides(pod, w.datadogConfig)
if err != nil {
log.Errorf("Failed to apply provider overrides: %v", err)
return podUpdated, errors.New(metrics.InvalidInput)
Expand All @@ -145,7 +148,7 @@ func (w *Webhook) injectAgentSidecar(pod *corev1.Pod, _ string, _ dynamic.Interf
// highest override-priority. They only apply to the agent sidecar container.
for i := range pod.Spec.Containers {
if pod.Spec.Containers[i].Name == agentSidecarContainerName {
updated, err = applyProfileOverrides(&pod.Spec.Containers[i])
updated, err = applyProfileOverrides(&pod.Spec.Containers[i], w.datadogConfig)
if err != nil {
log.Errorf("Failed to apply profile overrides: %v", err)
return podUpdated, errors.New(metrics.InvalidInput)
Expand All @@ -158,14 +161,14 @@ func (w *Webhook) injectAgentSidecar(pod *corev1.Pod, _ string, _ dynamic.Interf
return podUpdated, nil
}

func getDefaultSidecarTemplate(containerRegistry string) *corev1.Container {
func getDefaultSidecarTemplate(containerRegistry string, datadogConfig config.Component) *corev1.Container {
ddSite := os.Getenv("DD_SITE")
if ddSite == "" {
ddSite = pkgconfigsetup.DefaultSite
}

imageName := pkgconfigsetup.Datadog().GetString("admission_controller.agent_sidecar.image_name")
imageTag := pkgconfigsetup.Datadog().GetString("admission_controller.agent_sidecar.image_tag")
imageName := datadogConfig.GetString("admission_controller.agent_sidecar.image_name")
imageTag := datadogConfig.GetString("admission_controller.agent_sidecar.image_tag")

agentContainer := &corev1.Container{
Env: []corev1.EnvVar{
Expand Down Expand Up @@ -199,7 +202,7 @@ func getDefaultSidecarTemplate(containerRegistry string) *corev1.Container {
},
{
Name: "DD_LANGUAGE_DETECTION_ENABLED",
Value: strconv.FormatBool(pkgconfigsetup.Datadog().GetBool("language_detection.enabled") && pkgconfigsetup.Datadog().GetBool("language_detection.reporting.enabled")),
Value: strconv.FormatBool(datadogConfig.GetBool("language_detection.enabled") && datadogConfig.GetBool("language_detection.reporting.enabled")),
},
},
Image: fmt.Sprintf("%s/%s:%s", containerRegistry, imageName, imageTag),
Expand All @@ -217,11 +220,11 @@ func getDefaultSidecarTemplate(containerRegistry string) *corev1.Container {
},
}

clusterAgentEnabled := pkgconfigsetup.Datadog().GetBool("admission_controller.agent_sidecar.cluster_agent.enabled")
clusterAgentEnabled := datadogConfig.GetBool("admission_controller.agent_sidecar.cluster_agent.enabled")

if clusterAgentEnabled {
clusterAgentCmdPort := pkgconfigsetup.Datadog().GetInt("cluster_agent.cmd_port")
clusterAgentServiceName := pkgconfigsetup.Datadog().GetString("cluster_agent.kubernetes_service_name")
clusterAgentCmdPort := datadogConfig.GetInt("cluster_agent.cmd_port")
clusterAgentServiceName := datadogConfig.GetString("cluster_agent.kubernetes_service_name")

_, _ = withEnvOverrides(agentContainer, corev1.EnvVar{
Name: "DD_CLUSTER_AGENT_ENABLED",
Expand Down Expand Up @@ -249,12 +252,12 @@ func getDefaultSidecarTemplate(containerRegistry string) *corev1.Container {
}

// labelSelectors returns the mutating webhooks object selectors based on the configuration
func labelSelectors() (namespaceSelector, objectSelector *metav1.LabelSelector) {
func labelSelectors(datadogConfig config.Component) (namespaceSelector, objectSelector *metav1.LabelSelector) {
// Read and parse selectors
selectorsJSON := pkgconfigsetup.Datadog().GetString("admission_controller.agent_sidecar.selectors")
selectorsJSON := datadogConfig.GetString("admission_controller.agent_sidecar.selectors")

// Get sidecar profiles
_, err := loadSidecarProfiles()
_, err := loadSidecarProfiles(datadogConfig)
if err != nil {
log.Errorf("encountered issue when loading sidecar profiles: %s", err)
return nil, nil
Expand All @@ -273,7 +276,7 @@ func labelSelectors() (namespaceSelector, objectSelector *metav1.LabelSelector)
return nil, nil
}

provider := pkgconfigsetup.Datadog().GetString("admission_controller.agent_sidecar.provider")
provider := datadogConfig.GetString("admission_controller.agent_sidecar.provider")
if !providerIsSupported(provider) {
log.Errorf("agent sidecar provider is not supported: %v", provider)
return nil, nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@ import (

mutatecommon "github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/common"
configmock "github.com/DataDog/datadog-agent/pkg/config/mock"
"github.com/DataDog/datadog-agent/pkg/config/model"
apicommon "github.com/DataDog/datadog-agent/pkg/util/kubernetes/apiserver/common"
"github.com/DataDog/datadog-agent/pkg/util/pointer"
)

const commonRegistry = "gcr.io/datadoghq"

func TestInjectAgentSidecar(t *testing.T) {
mockConfig := configmock.New(t)
tests := []struct {
Name string
Pod *corev1.Pod
Expand Down Expand Up @@ -68,7 +70,7 @@ func TestInjectAgentSidecar(t *testing.T) {
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{Name: "container-name"},
*getDefaultSidecarTemplate(commonRegistry),
*getDefaultSidecarTemplate(commonRegistry, configmock.New(t)),
},
},
}
Expand Down Expand Up @@ -114,7 +116,7 @@ func TestInjectAgentSidecar(t *testing.T) {
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{Name: "container-name"},
*getDefaultSidecarTemplate(commonRegistry),
*getDefaultSidecarTemplate(commonRegistry, configmock.New(t)),
},
},
},
Expand All @@ -130,7 +132,7 @@ func TestInjectAgentSidecar(t *testing.T) {
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{Name: "container-name"},
*getDefaultSidecarTemplate(commonRegistry),
*getDefaultSidecarTemplate(commonRegistry, mockConfig),
},
},
}
Expand All @@ -153,7 +155,7 @@ func TestInjectAgentSidecar(t *testing.T) {
ExpectError: false,
ExpectInjection: true,
ExpectedPodAfterInjection: func() *corev1.Pod {
sidecar := *getDefaultSidecarTemplate(commonRegistry)
sidecar := *getDefaultSidecarTemplate(commonRegistry, mockConfig)
_, _ = withEnvOverrides(
&sidecar,
corev1.EnvVar{
Expand Down Expand Up @@ -254,7 +256,7 @@ func TestInjectAgentSidecar(t *testing.T) {
ExpectError: false,
ExpectInjection: true,
ExpectedPodAfterInjection: func() *corev1.Pod {
sidecar := *getDefaultSidecarTemplate(commonRegistry)
sidecar := *getDefaultSidecarTemplate(commonRegistry, mockConfig)

_, _ = withEnvOverrides(
&sidecar,
Expand Down Expand Up @@ -350,7 +352,7 @@ func TestInjectAgentSidecar(t *testing.T) {
mockConfig.SetWithoutSource("admission_controller.agent_sidecar.provider", test.provider)
mockConfig.SetWithoutSource("admission_controller.agent_sidecar.profiles", test.profilesJSON)

webhook := NewWebhook()
webhook := NewWebhook(mockConfig)

injected, err := webhook.injectAgentSidecar(test.Pod, "", nil)

Expand Down Expand Up @@ -389,33 +391,34 @@ func TestInjectAgentSidecar(t *testing.T) {
func TestDefaultSidecarTemplateAgentImage(t *testing.T) {
tests := []struct {
name string
setConfig func()
setConfig func() model.Config
containerRegistry string
expectedImage string
}{
{
name: "no configuration set",
setConfig: func() {},
setConfig: func() model.Config { return configmock.New(t) },
containerRegistry: commonRegistry,
expectedImage: fmt.Sprintf("%s/agent:latest", commonRegistry),
},
{
name: "setting custom registry, image and tag",
containerRegistry: "my-registry",
setConfig: func() {
setConfig: func() model.Config {
mockConfig := configmock.New(t)
mockConfig.SetWithoutSource("admission_controller.agent_sidecar.container_registry", "my-registry")
mockConfig.SetWithoutSource("admission_controller.agent_sidecar.image_name", "my-image")
mockConfig.SetWithoutSource("admission_controller.agent_sidecar.image_tag", "my-tag")
return mockConfig
},
expectedImage: "my-registry/my-image:my-tag",
},
}

for _, test := range tests {
t.Run(test.name, func(tt *testing.T) {
test.setConfig()
sidecar := getDefaultSidecarTemplate(test.containerRegistry)
mockConfig := test.setConfig()
sidecar := getDefaultSidecarTemplate(test.containerRegistry, mockConfig)
assert.Equal(tt, test.expectedImage, sidecar.Image)
})
}
Expand Down Expand Up @@ -565,7 +568,7 @@ func TestDefaultSidecarTemplateClusterAgentEnvVars(t *testing.T) {
for _, test := range tests {
t.Run(test.name, func(tt *testing.T) {
test.setConfig()
sidecar := getDefaultSidecarTemplate(commonRegistry)
sidecar := getDefaultSidecarTemplate(commonRegistry, configmock.New(t))
envVarsMap := make(map[string]corev1.EnvVar)
for _, envVar := range sidecar.Env {
envVarsMap[envVar.Name] = envVar
Expand Down
10 changes: 5 additions & 5 deletions pkg/clusteragent/admission/mutate/agent_sidecar/profiles.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (

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

pkgconfigsetup "github.com/DataDog/datadog-agent/pkg/config/setup"
"github.com/DataDog/datadog-agent/comp/core/config"
)

////////////////////////////////
Expand All @@ -32,9 +32,9 @@ type ProfileOverride struct {
// loadSidecarProfiles returns the profile overrides provided by the user
// It returns an error in case of miss-configuration or in case more than
// one profile is configured
func loadSidecarProfiles() ([]ProfileOverride, error) {
func loadSidecarProfiles(datadogConfig config.Component) ([]ProfileOverride, error) {
// Read and parse profiles
profilesJSON := pkgconfigsetup.Datadog().GetString("admission_controller.agent_sidecar.profiles")
profilesJSON := datadogConfig.GetString("admission_controller.agent_sidecar.profiles")

var profiles []ProfileOverride

Expand All @@ -52,12 +52,12 @@ func loadSidecarProfiles() ([]ProfileOverride, error) {

// applyProfileOverrides applies the profile overrides to the container. It
// returns a boolean that indicates if the container was mutated
func applyProfileOverrides(container *corev1.Container) (bool, error) {
func applyProfileOverrides(container *corev1.Container, datadogConfig config.Component) (bool, error) {
if container == nil {
return false, fmt.Errorf("can't apply profile overrides to nil containers")
}

profiles, err := loadSidecarProfiles()
profiles, err := loadSidecarProfiles(datadogConfig)

if err != nil {
return false, err
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func TestLoadSidecarProfiles(t *testing.T) {
for _, test := range tests {
t.Run(test.name, func(tt *testing.T) {
mockConfig.SetWithoutSource("admission_controller.agent_sidecar.profiles", test.profilesJSON)
profiles, err := loadSidecarProfiles()
profiles, err := loadSidecarProfiles(mockConfig)

if test.expectError {
assert.Error(tt, err)
Expand Down Expand Up @@ -361,7 +361,7 @@ func TestApplyProfileOverrides(t *testing.T) {
for _, test := range tests {
t.Run(test.name, func(tt *testing.T) {
mockConfig.SetWithoutSource("admission_controller.agent_sidecar.profiles", test.profilesJSON)
mutated, err := applyProfileOverrides(test.baseContainer)
mutated, err := applyProfileOverrides(test.baseContainer, mockConfig)

assert.Equal(tt, test.expectMutated, mutated)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ import (

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

"github.com/DataDog/datadog-agent/comp/core/config"
"github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/common"
configWebhook "github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/config"
pkgconfigsetup "github.com/DataDog/datadog-agent/pkg/config/setup"
"github.com/DataDog/datadog-agent/pkg/util/pointer"
)

Expand Down Expand Up @@ -55,8 +55,8 @@ func providerIsSupported(provider string) bool {

// applyProviderOverrides applies the necessary overrides for the provider
// configured. It returns a boolean that indicates if the pod was mutated.
func applyProviderOverrides(pod *corev1.Pod) (bool, error) {
provider := pkgconfigsetup.Datadog().GetString("admission_controller.agent_sidecar.provider")
func applyProviderOverrides(pod *corev1.Pod, datadogConfig config.Component) (bool, error) {
provider := datadogConfig.GetString("admission_controller.agent_sidecar.provider")

if !providerIsSupported(provider) {
return false, fmt.Errorf("unsupported provider: %v", provider)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ func TestApplyProviderOverrides(t *testing.T) {
for _, test := range tests {
t.Run(test.name, func(tt *testing.T) {
mockConfig.SetWithoutSource("admission_controller.agent_sidecar.provider", test.provider)
mutated, err := applyProviderOverrides(test.basePod)
mutated, err := applyProviderOverrides(test.basePod, mockConfig)

if test.expectError {
assert.Error(tt, err)
Expand Down
Loading