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(ssi): decouple admission controller webhooks #33599

Merged
merged 23 commits into from
Feb 6, 2025
Merged

fix(ssi): decouple admission controller webhooks #33599

Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
90ad7fe
fix(ssi): migrate injection filter to mutate common
betterengineering Jan 30, 2025
ebfb6cd
fix(ssi): remove config from injection filter
betterengineering Jan 30, 2025
a867941
fix(ssi): update mutating webhooks to use their own config
betterengineering Jan 30, 2025
020cafd
Fixup injection_filter
betterengineering Jan 30, 2025
15954b1
Fixup namespace filter naming
betterengineering Jan 30, 2025
b971c92
Fixup log line
betterengineering Jan 30, 2025
5b47e56
Update pkg/clusteragent/admission/mutate/tagsfromlabels/tags.go
betterengineering Jan 31, 2025
26c9b5e
Address PR feedback
betterengineering Jan 31, 2025
b7e1d69
fix(ssi): create injector interface and refactor webhooks
betterengineering Jan 31, 2025
dd18502
fix(ssi): rename internal instruementation pod injector
betterengineering Feb 3, 2025
5ae9cf8
fix(ssi): update auto instrumentation to new interface
betterengineering Feb 3, 2025
f617640
fix(ssi): refactor to mutator and mutation filter
betterengineering Feb 3, 2025
5fff25d
fix(ssi): cleanup mutation filter interface
betterengineering Feb 3, 2025
d8e1220
fix(ssi): add a multi mutator implementation
betterengineering Feb 3, 2025
a364c84
fix(ssi): wire in multi mutator for apm
betterengineering Feb 3, 2025
f7413c3
Fix label selector issue
betterengineering Feb 4, 2025
c2c57a6
Address PR feedback
betterengineering Feb 4, 2025
7d6826a
Add release note
betterengineering Feb 4, 2025
f2319e9
Merge branch 'main' into mark.spicer/INPLAT-467-seperate-usages-of-in…
betterengineering Feb 4, 2025
66efdc1
Apply suggestions from code review
betterengineering Feb 5, 2025
828da01
Fix linter issues
betterengineering Feb 5, 2025
048ad98
Fix tests
betterengineering Feb 5, 2025
931f3af
Merge branch 'main' into mark.spicer/INPLAT-467-seperate-usages-of-in…
betterengineering Feb 5, 2025
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
6 changes: 5 additions & 1 deletion pkg/clusteragent/admission/common/label_selectors.go
View file
Original file line number Diff line number Diff line change
@@ -19,7 +19,11 @@ func DefaultLabelSelectors(useNamespaceSelector bool) (namespaceSelector, object

if pkgconfigsetup.Datadog().GetBool("admission_controller.mutate_unlabelled") ||
pkgconfigsetup.Datadog().GetBool("apm_config.instrumentation.enabled") ||
len(pkgconfigsetup.Datadog().GetStringSlice("apm_config.instrumentation.enabled_namespaces")) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

A note for those reading this, this is used across all webhooks and isn't very optimized because of the dependencies across all of them (which we are trying to decouple).

@betterengineering Add a TODO here to create different label selectors and use them for the different webhooks (or make one webhook?). There are options.

cc @davidor @adel121 this definitely needs a follow up from your team.

pkgconfigsetup.Datadog().GetBool("admission_controller.inject_config.enabled") ||
pkgconfigsetup.Datadog().GetBool("admission_controller.inject_tags.enabled") ||
len(pkgconfigsetup.Datadog().GetStringSlice("apm_config.instrumentation.enabled_namespaces")) > 0 ||
len(pkgconfigsetup.Datadog().GetStringSlice("admission_controller.inject_config.enabled_namespaces")) > 0 ||
len(pkgconfigsetup.Datadog().GetStringSlice("admission_controller.inject_tags.enabled_namespaces")) > 0 {
// Accept all, ignore pods if they're explicitly filtered-out
labelSelector = metav1.LabelSelector{
MatchExpressions: []metav1.LabelSelectorRequirement{
113 changes: 82 additions & 31 deletions pkg/clusteragent/admission/controllers/webhook/controller_base.go
View file
Original file line number Diff line number Diff line change
@@ -30,6 +30,7 @@ import (
agentsidecar "github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/agent_sidecar"
"github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/autoinstrumentation"
"github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/autoscaling"
mutatecommon "github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/common"
configWebhook "github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/config"
"github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/cwsinstrumentation"
"github.com/DataDog/datadog-agent/pkg/clusteragent/admission/mutate/tagsfromlabels"
@@ -98,16 +99,8 @@ type Webhook interface {
// doesn't always work on Fargate (one of the envs where we use an agent sidecar), and
// the agent sidecar webhook needs to remove it.
func (c *controllerBase) generateWebhooks(wmeta workloadmeta.Component, pa workload.PodPatcher, datadogConfig config.Component, demultiplexer demultiplexer.Component) []Webhook {
// Note: the auto_instrumentation pod injection filter is used across
// multiple mutating webhooks, so we add it as a hard dependency to each
// of the components that use it via the injectionFilter parameter.
// TODO: for now we ignore the error returned by NewInjectionFilter, but we should not and surface it
Copy link
Member Author

Choose a reason for hiding this comment

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

This tiny TODO is the reason for this entire change 😭

// in the admission controller section in agent status.
injectionFilter, _ := autoinstrumentation.NewInjectionFilter(datadogConfig)

var webhooks []Webhook
var validatingWebhooks []Webhook
var mutatingWebhooks []Webhook

// Add Validating webhooks.
if c.config.isValidationEnabled() {
@@ -118,38 +111,96 @@ func (c *controllerBase) generateWebhooks(wmeta workloadmeta.Component, pa workl
webhooks = append(webhooks, validatingWebhooks...)
}

// Add Mutating webhooks.
if c.config.isMutationEnabled() {
mutatingWebhooks = []Webhook{
configWebhook.NewWebhook(wmeta, injectionFilter, datadogConfig),
tagsfromlabels.NewWebhook(wmeta, datadogConfig, injectionFilter),
agentsidecar.NewWebhook(datadogConfig),
autoscaling.NewWebhook(pa, datadogConfig),
}
webhooks = append(webhooks, mutatingWebhooks...)
// Skip mutating webhooks if the mutation feature is disabled.
if !c.config.isMutationEnabled() {
return webhooks
}

// Setup config webhook.
configWebhook, err := generateConfigWebhook(wmeta, datadogConfig)
if err != nil {
log.Errorf("failed to register config webhook: %v", err)
} else {
webhooks = append(webhooks, configWebhook)
}

// Setup tags from labels webhook.
tagsWebhook, err := generateTagsFromLabelsWebhook(wmeta, datadogConfig)
if err != nil {
log.Errorf("failed to register tags from labels webhook: %v", err)
} else {
webhooks = append(webhooks, tagsWebhook)
}

// Setup agents sidecar webhook.
agentsWebhook := agentsidecar.NewWebhook(datadogConfig)
webhooks = append(webhooks, agentsWebhook)

// Setup autoscaling webhook.
autoscalingWebhook := autoscaling.NewWebhook(pa, datadogConfig)
webhooks = append(webhooks, autoscalingWebhook)

// Setup APM Instrumentation webhook. APM Instrumentation webhook needs to be registered after the config webhook.
apmWebhook, err := generateAutoInstrumentationWebhook(wmeta, datadogConfig)
if err != nil {
log.Errorf("failed to register APM Instrumentation webhook: %v", err)
} else {
webhooks = append(webhooks, apmWebhook)
}

// APM Instrumentation webhook needs to be registered after the configWebhook webhook.
apm, err := autoinstrumentation.NewWebhook(wmeta, datadogConfig, injectionFilter)
isCWSInstrumentationEnabled := datadogConfig.GetBool("admission_controller.cws_instrumentation.enabled")
if isCWSInstrumentationEnabled {
cws, err := cwsinstrumentation.NewCWSInstrumentation(wmeta, datadogConfig)
if err == nil {
webhooks = append(webhooks, apm)
webhooks = append(webhooks, cws.WebhookForPods(), cws.WebhookForCommands())
} else {
log.Errorf("failed to register APM Instrumentation webhook: %v", err)
}

isCWSInstrumentationEnabled := datadogConfig.GetBool("admission_controller.cws_instrumentation.enabled")
if isCWSInstrumentationEnabled {
cws, err := cwsinstrumentation.NewCWSInstrumentation(wmeta, datadogConfig)
if err == nil {
webhooks = append(webhooks, cws.WebhookForPods(), cws.WebhookForCommands())
} else {
log.Errorf("failed to register CWS Instrumentation webhook: %v", err)
}
log.Errorf("failed to register CWS Instrumentation webhook: %v", err)
}
}

return webhooks
}

func generateConfigWebhook(wmeta workloadmeta.Component, datadogConfig config.Component) (*configWebhook.Webhook, error) {
filter, err := configWebhook.NewFilter(configWebhook.NewFilterConfig(datadogConfig))
if err != nil {
return nil, fmt.Errorf("failed to create config filter: %v", err)
}
mutatorCfg := configWebhook.NewMutatorConfig(datadogConfig)
mutator := configWebhook.NewMutator(mutatorCfg, filter)
return configWebhook.NewWebhook(wmeta, datadogConfig, mutator), nil
Copy link
Member

Choose a reason for hiding this comment

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

This might be out of scope for this change, but do the NewWebhok functions need to have the datadogConfig passed in anymore?

}

func generateTagsFromLabelsWebhook(wmeta workloadmeta.Component, datadogConfig config.Component) (*tagsfromlabels.Webhook, error) {
filter, err := tagsfromlabels.NewFilter(tagsfromlabels.NewFilterConfig(datadogConfig))
if err != nil {
return nil, fmt.Errorf("failed to create tags from labels filter: %v", err)
}
mutator := tagsfromlabels.NewMutator(tagsfromlabels.NewMutatorConfig(datadogConfig), filter)
return tagsfromlabels.NewWebhook(wmeta, datadogConfig, mutator), nil
}

func generateAutoInstrumentationWebhook(wmeta workloadmeta.Component, datadogConfig config.Component) (*autoinstrumentation.Webhook, error) {
filter, err := autoinstrumentation.NewFilter(autoinstrumentation.NewFilterConfig(datadogConfig))
if err != nil {
return nil, fmt.Errorf("failed to create auto instrumentation filter: %v", err)
}
apmMutatorCfg, err := autoinstrumentation.NewMutatorConfig(datadogConfig)
if err != nil {
return nil, fmt.Errorf("failed to create auto instrumentation mutator config: %v", err)
}

// For auto instrumentation, we need all the mutators to be applied for SSI to function. Specifically, we need
// things like the Datadog socket to be mounted from the config webhook and the DD_ENV, DD_SERVICE, and DD_VERSION
// env vars to be set from labels if they are available..
mutator := mutatecommon.NewMutators(
tagsfromlabels.NewMutator(tagsfromlabels.NewMutatorConfig(datadogConfig), filter),
configWebhook.NewMutator(configWebhook.NewMutatorConfig(datadogConfig), filter),
autoinstrumentation.NewMutator(apmMutatorCfg, filter, wmeta),
)
return autoinstrumentation.NewWebhook(wmeta, datadogConfig, mutator)
}

// controllerBase acts as a base class for ControllerV1 and ControllerV1beta1.
// It contains the shared fields and provides shared methods.
// For the nolint:structcheck see https://github.com/golangci/golangci-lint/issues/537
48 changes: 36 additions & 12 deletions pkg/clusteragent/admission/controllers/webhook/controller_v1_test.go
View file
Original file line number Diff line number Diff line change
@@ -282,8 +282,12 @@ func TestGenerateTemplatesV1(t *testing.T) {
configFunc: func(mockConfig model.Config) Config { return NewConfig(false, false, false, mockConfig) },
want: func() []admiv1.MutatingWebhook {
webhook := webhook("datadog.webhook.agent.config", "/injectconfig", &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, nil, []admiv1.MatchCondition{}, []admiv1.OperationType{admiv1.Create}, []string{"pods"})
return []admiv1.MutatingWebhook{webhook}
@@ -324,8 +328,12 @@ func TestGenerateTemplatesV1(t *testing.T) {
configFunc: func(mockConfig model.Config) Config { return NewConfig(false, false, false, mockConfig) },
want: func() []admiv1.MutatingWebhook {
webhook := webhook("datadog.webhook.standard.tags", "/injecttags", &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, nil, []admiv1.MatchCondition{}, []admiv1.OperationType{admiv1.Create}, []string{"pods"})
return []admiv1.MutatingWebhook{webhook}
@@ -384,13 +392,21 @@ func TestGenerateTemplatesV1(t *testing.T) {
configFunc: func(mockConfig model.Config) Config { return NewConfig(false, false, false, mockConfig) },
want: func() []admiv1.MutatingWebhook {
webhookConfig := webhook("datadog.webhook.agent.config", "/injectconfig", &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, nil, []admiv1.MatchCondition{}, []admiv1.OperationType{admiv1.Create}, []string{"pods"})
webhookTags := webhook("datadog.webhook.standard.tags", "/injecttags", &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, nil, []admiv1.MatchCondition{}, []admiv1.OperationType{admiv1.Create}, []string{"pods"})
return []admiv1.MutatingWebhook{webhookConfig, webhookTags}
@@ -441,13 +457,21 @@ func TestGenerateTemplatesV1(t *testing.T) {
configFunc: func(mockConfig model.Config) Config { return NewConfig(false, true, false, mockConfig) },
want: func() []admiv1.MutatingWebhook {
webhookConfig := webhook("datadog.webhook.agent.config", "/injectconfig", nil, &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, []admiv1.MatchCondition{}, []admiv1.OperationType{admiv1.Create}, []string{"pods"})
webhookTags := webhook("datadog.webhook.standard.tags", "/injecttags", nil, &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, []admiv1.MatchCondition{}, []admiv1.OperationType{admiv1.Create}, []string{"pods"})
return []admiv1.MutatingWebhook{webhookConfig, webhookTags}
48 changes: 36 additions & 12 deletions pkg/clusteragent/admission/controllers/webhook/controller_v1beta1_test.go
View file
Original file line number Diff line number Diff line change
@@ -278,8 +278,12 @@ func TestGenerateTemplatesV1beta1(t *testing.T) {
configFunc: func(mockConfig model.Config) Config { return NewConfig(false, false, false, mockConfig) },
want: func() []admiv1beta1.MutatingWebhook {
webhook := webhook("datadog.webhook.agent.config", "/injectconfig", &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, nil, []admiv1beta1.MatchCondition{}, []admiv1beta1.OperationType{admiv1beta1.Create}, []string{"pods"})
return []admiv1beta1.MutatingWebhook{webhook}
@@ -320,8 +324,12 @@ func TestGenerateTemplatesV1beta1(t *testing.T) {
configFunc: func(mockConfig model.Config) Config { return NewConfig(false, false, false, mockConfig) },
want: func() []admiv1beta1.MutatingWebhook {
webhook := webhook("datadog.webhook.standard.tags", "/injecttags", &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, nil, []admiv1beta1.MatchCondition{}, []admiv1beta1.OperationType{admiv1beta1.Create}, []string{"pods"})
return []admiv1beta1.MutatingWebhook{webhook}
@@ -380,13 +388,21 @@ func TestGenerateTemplatesV1beta1(t *testing.T) {
configFunc: func(mockConfig model.Config) Config { return NewConfig(false, false, false, mockConfig) },
want: func() []admiv1beta1.MutatingWebhook {
webhookConfig := webhook("datadog.webhook.agent.config", "/injectconfig", &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, nil, []admiv1beta1.MatchCondition{}, []admiv1beta1.OperationType{admiv1beta1.Create}, []string{"pods"})
webhookTags := webhook("datadog.webhook.standard.tags", "/injecttags", &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, nil, []admiv1beta1.MatchCondition{}, []admiv1beta1.OperationType{admiv1beta1.Create}, []string{"pods"})
return []admiv1beta1.MutatingWebhook{webhookConfig, webhookTags}
@@ -437,13 +453,21 @@ func TestGenerateTemplatesV1beta1(t *testing.T) {
configFunc: func(mockConfig model.Config) Config { return NewConfig(false, true, false, mockConfig) },
want: func() []admiv1beta1.MutatingWebhook {
webhookConfig := webhook("datadog.webhook.agent.config", "/injectconfig", nil, &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, []admiv1beta1.MatchCondition{}, []admiv1beta1.OperationType{admiv1beta1.Create}, []string{"pods"})
webhookTags := webhook("datadog.webhook.standard.tags", "/injecttags", nil, &metav1.LabelSelector{
MatchLabels: map[string]string{
"admission.datadoghq.com/enabled": "true",
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "admission.datadoghq.com/enabled",
Operator: metav1.LabelSelectorOpNotIn,
Values: []string{"false"},
},
},
}, []admiv1beta1.MatchCondition{}, []admiv1beta1.OperationType{admiv1beta1.Create}, []string{"pods"})
return []admiv1beta1.MutatingWebhook{webhookConfig, webhookTags}
Loading
Loading