From 856f458993220ef30a09bbf72f3e571915f4df79 Mon Sep 17 00:00:00 2001 From: Gabriel Dos Santos Date: Tue, 25 Feb 2025 20:49:55 +0000 Subject: [PATCH 1/6] Kubelet ignoring unknown envvars --- .../collectors/internal/kubelet/kubelet.go | 11 +- comp/core/workloadmeta/def/merge_test.go | 11 + .../third_party/golang/expansion/expand.go | 18 +- .../golang/expansion/expand_test.go | 275 ++++++++++-------- pkg/util/kubernetes/kubelet/kubelet.go | 16 + pkg/util/kubernetes/kubelet/kubelet_test.go | 45 +++ .../kubelet/testdata/podlist_1.8-2.json | 263 +++++++++-------- pkg/util/kubernetes/kubelet/types_kubelet.go | 2 + 8 files changed, 392 insertions(+), 249 deletions(-) diff --git a/comp/core/workloadmeta/collectors/internal/kubelet/kubelet.go b/comp/core/workloadmeta/collectors/internal/kubelet/kubelet.go index 25440989430f96..c3d1b55a974d05 100644 --- a/comp/core/workloadmeta/collectors/internal/kubelet/kubelet.go +++ b/comp/core/workloadmeta/collectors/internal/kubelet/kubelet.go @@ -417,9 +417,18 @@ func extractEnvFromSpec(envSpec []kubelet.EnvVar) map[string]string { continue } + ok := true runtimeVal := e.Value if runtimeVal != "" { - runtimeVal = expansion.Expand(runtimeVal, mappingFunc) + runtimeVal, ok = expansion.Expand(runtimeVal, mappingFunc) + } + + // Ignore environment variables that failed to expand + // This occurs when the env var references another env var + // that has its value sourced from an external source + // (eg. ConfigMap, Secret, DownwardAPI) + if !ok { + continue } env[e.Name] = runtimeVal diff --git a/comp/core/workloadmeta/def/merge_test.go b/comp/core/workloadmeta/def/merge_test.go index 866c2f58f451d2..ea3cbbc0bc5f89 100644 --- a/comp/core/workloadmeta/def/merge_test.go +++ b/comp/core/workloadmeta/def/merge_test.go @@ -49,6 +49,9 @@ func container1(testTime time.Time) Container { Health: ContainerHealthUnknown, }, CollectorTags: []string{"tag1", "tag2"}, + EnvVars: map[string]string{ + "DD_SERVICE-partial": "my-svc", + }, } } @@ -91,6 +94,10 @@ func container2() Container { Health: ContainerHealthHealthy, }, CollectorTags: []string{"tag3"}, + EnvVars: map[string]string{ + "DD_SERVICE-partial": "my-svc", + "DD_ENV-extra": "prod", + }, } } @@ -114,6 +121,10 @@ func TestMerge(t *testing.T) { ExitCode: pointer.Ptr(int64(100)), Health: ContainerHealthHealthy, }, + EnvVars: map[string]string{ + "DD_SERVICE-partial": "my-svc", + "DD_ENV-extra": "prod", + }, } expectedPorts := []ContainerPort{ diff --git a/internal/third_party/golang/expansion/expand.go b/internal/third_party/golang/expansion/expand.go index 6bf0ea8ce09f63..f0928a1ad603d6 100644 --- a/internal/third_party/golang/expansion/expand.go +++ b/internal/third_party/golang/expansion/expand.go @@ -19,23 +19,23 @@ func syntaxWrap(input string) string { // implements the expansion semantics defined in the expansion spec; it // returns the input string wrapped in the expansion syntax if no mapping // for the input is found. -func MappingFuncFor(context ...map[string]string) func(string) string { - return func(input string) string { +func MappingFuncFor(context ...map[string]string) func(string) (string, bool) { + return func(input string) (string, bool) { for _, vars := range context { val, ok := vars[input] if ok { - return val + return val, true } } - return syntaxWrap(input) + return syntaxWrap(input), false } } // Expand replaces variable references in the input string according to // the expansion spec using the given mapping function to resolve the // values of variables. -func Expand(input string, mapping func(string) string) string { +func Expand(input string, mapping func(string) (string, bool)) (string, bool) { var buf bytes.Buffer checkpoint := 0 for cursor := 0; cursor < len(input); cursor++ { @@ -52,7 +52,11 @@ func Expand(input string, mapping func(string) string) string { // We were able to read a variable name correctly; // apply the mapping to the variable name and copy the // bytes into the buffer - buf.WriteString(mapping(read)) + mappedValue, found := mapping(read) + if !found { + return "", false + } + buf.WriteString(mappedValue) } else { // Not a variable name; copy the read bytes into the buffer buf.WriteString(read) @@ -69,7 +73,7 @@ func Expand(input string, mapping func(string) string) string { // Return the buffer and any remaining unwritten bytes in the // input string. - return buf.String() + input[checkpoint:] + return buf.String() + input[checkpoint:], true } // tryReadVariableName attempts to read a variable name from the input diff --git a/internal/third_party/golang/expansion/expand_test.go b/internal/third_party/golang/expansion/expand_test.go index 2c98658f0b23a7..dc7fa31f565d26 100644 --- a/internal/third_party/golang/expansion/expand_test.go +++ b/internal/third_party/golang/expansion/expand_test.go @@ -33,7 +33,7 @@ func TestMapReference(t *testing.T) { mapping := MappingFuncFor(declaredEnv, serviceEnv) for _, env := range envs { - declaredEnv[env.Name] = Expand(env.Value, mapping) + declaredEnv[env.Name], _ = Expand(env.Value, mapping) } expectedEnv := map[string]string{ @@ -83,203 +83,244 @@ func TestMappingDual(t *testing.T) { doExpansionTest(t, mapping) } -func doExpansionTest(t *testing.T, mapping func(string) string) { +func doExpansionTest(t *testing.T, mapping func(string) (string, bool)) { cases := []struct { - name string - input string - expected string + name string + input string + expected string + expectedStatus bool }{ { - name: "whole string", - input: "$(VAR_A)", - expected: "A", + name: "whole string", + input: "$(VAR_A)", + expected: "A", + expectedStatus: true, }, { - name: "repeat", - input: "$(VAR_A)-$(VAR_A)", - expected: "A-A", + name: "repeat", + input: "$(VAR_A)-$(VAR_A)", + expected: "A-A", + expectedStatus: true, }, { - name: "beginning", - input: "$(VAR_A)-1", - expected: "A-1", + name: "beginning", + input: "$(VAR_A)-1", + expected: "A-1", + expectedStatus: true, }, { - name: "middle", - input: "___$(VAR_B)___", - expected: "___B___", + name: "middle", + input: "___$(VAR_B)___", + expected: "___B___", + expectedStatus: true, }, { - name: "end", - input: "___$(VAR_C)", - expected: "___C", + name: "end", + input: "___$(VAR_C)", + expected: "___C", + expectedStatus: true, }, { - name: "compound", - input: "$(VAR_A)_$(VAR_B)_$(VAR_C)", - expected: "A_B_C", + name: "compound", + input: "$(VAR_A)_$(VAR_B)_$(VAR_C)", + expected: "A_B_C", + expectedStatus: true, }, { - name: "escape & expand", - input: "$$(VAR_B)_$(VAR_A)", - expected: "$(VAR_B)_A", + name: "escape & expand", + input: "$$(VAR_B)_$(VAR_A)", + expected: "$(VAR_B)_A", + expectedStatus: true, }, { - name: "compound escape", - input: "$$(VAR_A)_$$(VAR_B)", - expected: "$(VAR_A)_$(VAR_B)", + name: "compound escape", + input: "$$(VAR_A)_$$(VAR_B)", + expected: "$(VAR_A)_$(VAR_B)", + expectedStatus: true, }, { - name: "mixed in escapes", - input: "f000-$$VAR_A", - expected: "f000-$VAR_A", + name: "mixed in escapes", + input: "f000-$$VAR_A", + expected: "f000-$VAR_A", + expectedStatus: true, }, { - name: "backslash escape ignored", - input: "foo\\$(VAR_C)bar", - expected: "foo\\Cbar", + name: "backslash escape ignored", + input: "foo\\$(VAR_C)bar", + expected: "foo\\Cbar", + expectedStatus: true, }, { - name: "backslash escape ignored", - input: "foo\\\\$(VAR_C)bar", - expected: "foo\\\\Cbar", + name: "backslash escape ignored", + input: "foo\\\\$(VAR_C)bar", + expected: "foo\\\\Cbar", + expectedStatus: true, }, { - name: "lots of backslashes", - input: "foo\\\\\\\\$(VAR_A)bar", - expected: "foo\\\\\\\\Abar", + name: "lots of backslashes", + input: "foo\\\\\\\\$(VAR_A)bar", + expected: "foo\\\\\\\\Abar", + expectedStatus: true, }, { - name: "nested var references", - input: "$(VAR_A$(VAR_B))", - expected: "$(VAR_A$(VAR_B))", + name: "nested var references", + input: "$(VAR_A$(VAR_B))", + expected: "", + expectedStatus: false, }, { - name: "nested var references second type", - input: "$(VAR_A$(VAR_B)", - expected: "$(VAR_A$(VAR_B)", + name: "nested var references second type", + input: "$(VAR_A$(VAR_B)", + expected: "", + expectedStatus: false, }, { - name: "value is a reference", - input: "$(VAR_REF)", - expected: "$(VAR_A)", + name: "value is a reference", + input: "$(VAR_REF)", + expected: "$(VAR_A)", + expectedStatus: true, }, { - name: "value is a reference x 2", - input: "%%$(VAR_REF)--$(VAR_REF)%%", - expected: "%%$(VAR_A)--$(VAR_A)%%", + name: "value is a reference x 2", + input: "%%$(VAR_REF)--$(VAR_REF)%%", + expected: "%%$(VAR_A)--$(VAR_A)%%", + expectedStatus: true, }, { - name: "empty var", - input: "foo$(VAR_EMPTY)bar", - expected: "foobar", + name: "empty var", + input: "foo$(VAR_EMPTY)bar", + expected: "foobar", + expectedStatus: true, }, { - name: "unterminated expression", - input: "foo$(VAR_Awhoops!", - expected: "foo$(VAR_Awhoops!", + name: "unterminated expression", + input: "foo$(VAR_Awhoops!", + expected: "foo$(VAR_Awhoops!", + expectedStatus: true, }, { - name: "expression without operator", - input: "f00__(VAR_A)__", - expected: "f00__(VAR_A)__", + name: "expression without operator", + input: "f00__(VAR_A)__", + expected: "f00__(VAR_A)__", + expectedStatus: true, }, { - name: "shell special vars pass through", - input: "$?_boo_$!", - expected: "$?_boo_$!", + name: "shell special vars pass through", + input: "$?_boo_$!", + expected: "$?_boo_$!", + expectedStatus: true, }, { - name: "bare operators are ignored", - input: "$VAR_A", - expected: "$VAR_A", + name: "bare operators are ignored", + input: "$VAR_A", + expected: "$VAR_A", + expectedStatus: true, }, { - name: "undefined vars are passed through", - input: "$(VAR_DNE)", - expected: "$(VAR_DNE)", + name: "undefined vars are passed through", + input: "$(VAR_DNE)", + expected: "", + expectedStatus: false, }, { - name: "multiple (even) operators, var undefined", - input: "$$$$$$(BIG_MONEY)", - expected: "$$$(BIG_MONEY)", + name: "multiple (even) operators, var undefined", + input: "$$$$$$(BIG_MONEY)", + expected: "$$$(BIG_MONEY)", + expectedStatus: true, }, { - name: "multiple (even) operators, var defined", - input: "$$$$$$(VAR_A)", - expected: "$$$(VAR_A)", + name: "multiple (even) operators, var defined", + input: "$$$$$$(VAR_A)", + expected: "$$$(VAR_A)", + expectedStatus: true, }, { - name: "multiple (odd) operators, var undefined", - input: "$$$$$$$(GOOD_ODDS)", - expected: "$$$$(GOOD_ODDS)", + name: "multiple (odd) operators, var undefined", + input: "$$$$$$$(GOOD_ODDS)", + expected: "", + expectedStatus: false, }, { - name: "multiple (odd) operators, var defined", - input: "$$$$$$$(VAR_A)", - expected: "$$$A", + name: "multiple (odd) operators, var defined", + input: "$$$$$$$(VAR_A)", + expected: "$$$A", + expectedStatus: true, }, { - name: "missing open expression", - input: "$VAR_A)", - expected: "$VAR_A)", + name: "missing open expression", + input: "$VAR_A)", + expected: "$VAR_A)", + expectedStatus: true, }, { - name: "shell syntax ignored", - input: "${VAR_A}", - expected: "${VAR_A}", + name: "shell syntax ignored", + input: "${VAR_A}", + expected: "${VAR_A}", + expectedStatus: true, }, { - name: "trailing incomplete expression not consumed", - input: "$(VAR_B)_______$(A", - expected: "B_______$(A", + name: "trailing incomplete expression not consumed", + input: "$(VAR_B)_______$(A", + expected: "B_______$(A", + expectedStatus: true, }, { - name: "trailing incomplete expression, no content, is not consumed", - input: "$(VAR_C)_______$(", - expected: "C_______$(", + name: "trailing incomplete expression, no content, is not consumed", + input: "$(VAR_C)_______$(", + expected: "C_______$(", + expectedStatus: true, }, { - name: "operator at end of input string is preserved", - input: "$(VAR_A)foobarzab$", - expected: "Afoobarzab$", + name: "operator at end of input string is preserved", + input: "$(VAR_A)foobarzab$", + expected: "Afoobarzab$", + expectedStatus: true, }, { - name: "shell escaped incomplete expr", - input: "foo-\\$(VAR_A", - expected: "foo-\\$(VAR_A", + name: "shell escaped incomplete expr", + input: "foo-\\$(VAR_A", + expected: "foo-\\$(VAR_A", + expectedStatus: true, }, { - name: "lots of $( in middle", - input: "--$($($($($--", - expected: "--$($($($($--", + name: "lots of $( in middle", + input: "--$($($($($--", + expected: "--$($($($($--", + expectedStatus: true, }, { - name: "lots of $( in beginning", - input: "$($($($($--foo$(", - expected: "$($($($($--foo$(", + name: "lots of $( in beginning", + input: "$($($($($--foo$(", + expected: "$($($($($--foo$(", + expectedStatus: true, }, { - name: "lots of $( at end", - input: "foo0--$($($($(", - expected: "foo0--$($($($(", + name: "lots of $( at end", + input: "foo0--$($($($(", + expected: "foo0--$($($($(", + expectedStatus: true, }, { - name: "escaped operators in variable names are not escaped", - input: "$(foo$$var)", - expected: "$(foo$$var)", + name: "escaped operators in variable names are not escaped", + input: "$(foo$$var)", + expected: "", + expectedStatus: false, }, { - name: "newline not expanded", - input: "\n", - expected: "\n", + name: "newline not expanded", + input: "\n", + expected: "\n", + expectedStatus: true, }, } for _, tc := range cases { - expanded := Expand(tc.input, mapping) + expanded, success := Expand(tc.input, mapping) if e, a := tc.expected, expanded; e != a { t.Errorf("%v: expected %q, got %q", tc.name, e, a) } + if e, a := tc.expectedStatus, success; e != a { + t.Errorf("%v: expected success=%v, got %v", tc.name, e, a) + } } } diff --git a/pkg/util/kubernetes/kubelet/kubelet.go b/pkg/util/kubernetes/kubelet/kubelet.go index d44e8074be9369..5db5ac5a5d0a42 100644 --- a/pkg/util/kubernetes/kubelet/kubelet.go +++ b/pkg/util/kubernetes/kubelet/kubelet.go @@ -236,6 +236,8 @@ func (ku *KubeUtil) getLocalPodList(ctx context.Context) (*PodList, error) { allContainers = append(allContainers, pod.Status.InitContainers...) allContainers = append(allContainers, pod.Status.Containers...) pod.Status.AllContainers = allContainers + filterEnvVars(pod.Spec.Containers) + filterEnvVars(pod.Spec.InitContainers) tmpSlice = append(tmpSlice, pod) } } @@ -486,3 +488,17 @@ func isPodStatic(pod *Pod) bool { } return false } + +// filterEnvVars removes unsupported env var sources (eg. ConfigMap, Secrets, etc.) +func filterEnvVars(containers []ContainerSpec) { + for i := range containers { + cleanEnvVars := make([]EnvVar, 0) + for _, envVar := range containers[i].Env { + if envVar.ValueFrom != nil { + continue + } + cleanEnvVars = append(cleanEnvVars, envVar) + } + containers[i].Env = cleanEnvVars + } +} diff --git a/pkg/util/kubernetes/kubelet/kubelet_test.go b/pkg/util/kubernetes/kubelet/kubelet_test.go index 5ced2ecf98817a..5b330fb274d125 100644 --- a/pkg/util/kubernetes/kubelet/kubelet_test.go +++ b/pkg/util/kubernetes/kubelet/kubelet_test.go @@ -843,6 +843,51 @@ func TestKubeletTestSuite(t *testing.T) { suite.Run(t, new(KubeletTestSuite)) } +func (suite *KubeletTestSuite) TestContainerEnvVars() { + ctx := context.Background() + mockConfig := configmock.New(suite.T()) + + kubelet, err := newDummyKubelet("./testdata/podlist_1.8-2.json") + require.Nil(suite.T(), err) + ts, kubeletPort, err := kubelet.Start() + require.Nil(suite.T(), err) + defer ts.Close() + + mockConfig.SetWithoutSource("kubernetes_kubelet_host", "localhost") + mockConfig.SetWithoutSource("kubernetes_http_kubelet_port", kubeletPort) + mockConfig.SetWithoutSource("kubernetes_https_kubelet_port", -1) + + kubeutil := suite.getCustomKubeUtil() + kubelet.dropRequests() // Throwing away first GETs + + pods, err := kubeutil.ForceGetLocalPodList(ctx) + require.Nil(suite.T(), err) + require.NotNil(suite.T(), pods) + + var nginxPod *Pod + for _, pod := range pods.Items { + if pod.Metadata.Name == "nginx-99d8b564-4r4vq" { + nginxPod = pod + break + } + } + require.NotNil(suite.T(), nginxPod) + + var nginxContainer *ContainerSpec + for _, container := range nginxPod.Spec.Containers { + if container.Name == "nginx" { + nginxContainer = &container + break + } + } + require.NotNil(suite.T(), nginxContainer) + + expectedEnvVars := []EnvVar{ + {Name: "DEFINED_VAR", Value: "true"}, + } + assert.ElementsMatch(suite.T(), nginxContainer.Env, expectedEnvVars) +} + func (suite *KubeletTestSuite) TestPodListWithNullPod() { ctx := context.Background() mockConfig := configmock.New(suite.T()) diff --git a/pkg/util/kubernetes/kubelet/testdata/podlist_1.8-2.json b/pkg/util/kubernetes/kubelet/testdata/podlist_1.8-2.json index c1223d00cf84c5..5070e0fda6f332 100644 --- a/pkg/util/kubernetes/kubelet/testdata/podlist_1.8-2.json +++ b/pkg/util/kubernetes/kubelet/testdata/podlist_1.8-2.json @@ -618,6 +618,21 @@ "successThreshold": 1, "failureThreshold": 3 }, + "env": [ + { + "name": "UNDEFINED_VAR", + "valueFrom": { + "secretKeyRef": { + "name": "nginx", + "key": "password" + } + } + }, + { + "name": "DEFINED_VAR", + "value": "true" + } + ], "terminationMessagePath": "/dev/termination-log", "terminationMessagePolicy": "File", "imagePullPolicy": "IfNotPresent" @@ -788,147 +803,147 @@ }, { "status": { - "phase": "Pending", - "conditions": [ - { - "status": "True", - "lastProbeTime": null, - "type": "PodScheduled", - "lastTransitionTime": "2018-02-13T16:10:24Z" - } - ] + "phase": "Pending", + "conditions": [ + { + "status": "True", + "lastProbeTime": null, + "type": "PodScheduled", + "lastTransitionTime": "2018-02-13T16:10:24Z" + } + ] }, "spec": { - "dnsPolicy": "ClusterFirst", - "securityContext": {}, - "nodeName": "gke-haissam-default-pool-be5066f1-wnvn", - "schedulerName": "default-scheduler", - "hostNetwork": true, - "terminationGracePeriodSeconds": 30, - "restartPolicy": "Always", - "volumes": [ - { - "hostPath": { - "path": "/usr/share/ca-certificates", - "type": "" - }, - "name": "usr-ca-certs" - }, + "dnsPolicy": "ClusterFirst", + "securityContext": {}, + "nodeName": "gke-haissam-default-pool-be5066f1-wnvn", + "schedulerName": "default-scheduler", + "hostNetwork": true, + "terminationGracePeriodSeconds": 30, + "restartPolicy": "Always", + "volumes": [ + { + "hostPath": { + "path": "/usr/share/ca-certificates", + "type": "" + }, + "name": "usr-ca-certs" + }, + { + "hostPath": { + "path": "/etc/ssl/certs", + "type": "" + }, + "name": "etc-ssl-certs" + }, + { + "hostPath": { + "path": "/var/lib/kube-proxy/kubeconfig", + "type": "FileOrCreate" + }, + "name": "kubeconfig" + }, + { + "hostPath": { + "path": "/var/log", + "type": "" + }, + "name": "varlog" + }, + { + "hostPath": { + "path": "/run/xtables.lock", + "type": "FileOrCreate" + }, + "name": "iptableslock" + }, + { + "hostPath": { + "path": "/lib/modules", + "type": "" + }, + "name": "lib-modules" + } + ], + "tolerations": [ + { + "operator": "Exists", + "effect": "NoExecute" + }, + { + "operator": "Exists", + "effect": "NoSchedule" + } + ], + "containers": [ + { + "terminationMessagePath": "/dev/termination-log", + "name": "kube-proxy", + "image": "gcr.io/google_containers/kube-proxy:v1.9.2-gke.1", + "volumeMounts": [ { - "hostPath": { - "path": "/etc/ssl/certs", - "type": "" - }, - "name": "etc-ssl-certs" + "readOnly": true, + "mountPath": "/etc/ssl/certs", + "name": "etc-ssl-certs" }, { - "hostPath": { - "path": "/var/lib/kube-proxy/kubeconfig", - "type": "FileOrCreate" - }, - "name": "kubeconfig" + "readOnly": true, + "mountPath": "/usr/share/ca-certificates", + "name": "usr-ca-certs" }, { - "hostPath": { - "path": "/var/log", - "type": "" - }, - "name": "varlog" + "mountPath": "/var/log", + "name": "varlog" }, { - "hostPath": { - "path": "/run/xtables.lock", - "type": "FileOrCreate" - }, - "name": "iptableslock" + "mountPath": "/var/lib/kube-proxy/kubeconfig", + "name": "kubeconfig" }, { - "hostPath": { - "path": "/lib/modules", - "type": "" - }, - "name": "lib-modules" - } - ], - "tolerations": [ - { - "operator": "Exists", - "effect": "NoExecute" + "mountPath": "/run/xtables.lock", + "name": "iptableslock" }, { - "operator": "Exists", - "effect": "NoSchedule" + "readOnly": true, + "mountPath": "/lib/modules", + "name": "lib-modules" } - ], - "containers": [ - { - "terminationMessagePath": "/dev/termination-log", - "name": "kube-proxy", - "image": "gcr.io/google_containers/kube-proxy:v1.9.2-gke.1", - "volumeMounts": [ - { - "readOnly": true, - "mountPath": "/etc/ssl/certs", - "name": "etc-ssl-certs" - }, - { - "readOnly": true, - "mountPath": "/usr/share/ca-certificates", - "name": "usr-ca-certs" - }, - { - "mountPath": "/var/log", - "name": "varlog" - }, - { - "mountPath": "/var/lib/kube-proxy/kubeconfig", - "name": "kubeconfig" - }, - { - "mountPath": "/run/xtables.lock", - "name": "iptableslock" - }, - { - "readOnly": true, - "mountPath": "/lib/modules", - "name": "lib-modules" - } - ], - "terminationMessagePolicy": "File", - "command": [ - "/bin/sh", - "-c", - "exec kube-proxy --master=https://35.189.248.80 --kubeconfig=/var/lib/kube-proxy/kubeconfig --cluster-cidr=10.8.0.0/14 --resource-container=\"\" --oom-score-adj=-998 --v=2 --feature-gates=ExperimentalCriticalPodAnnotation=true --iptables-sync-period=1m --iptables-min-sync-period=10s --ipvs-sync-period=1m --ipvs-min-sync-period=10s 1>>/var/log/kube-proxy.log 2>&1" - ], - "imagePullPolicy": "IfNotPresent", - "securityContext": { - "privileged": true - }, - "resources": { - "requests": { - "cpu": "100m" - } - } + ], + "terminationMessagePolicy": "File", + "command": [ + "/bin/sh", + "-c", + "exec kube-proxy --master=https://35.189.248.80 --kubeconfig=/var/lib/kube-proxy/kubeconfig --cluster-cidr=10.8.0.0/14 --resource-container=\"\" --oom-score-adj=-998 --v=2 --feature-gates=ExperimentalCriticalPodAnnotation=true --iptables-sync-period=1m --iptables-min-sync-period=10s --ipvs-sync-period=1m --ipvs-min-sync-period=10s 1>>/var/log/kube-proxy.log 2>&1" + ], + "imagePullPolicy": "IfNotPresent", + "securityContext": { + "privileged": true + }, + "resources": { + "requests": { + "cpu": "100m" } - ] + } + } + ] }, "metadata": { - "name": "kube-proxy-gke-haissam-default-pool-be5066f1-wnvn", - "labels": { - "tier": "node", - "component": "kube-proxy" - }, - "namespace": "kube-system", - "creationTimestamp": null, - "annotations": { - "scheduler.alpha.kubernetes.io/critical-pod": "", - "kubernetes.io/config.hash": "260c2b1d43b094af6d6b4ccba082c2db", - "kubernetes.io/config.source": "http", - "kubernetes.io/config.seen": "2018-02-13T16:10:19.507814572Z" - }, - "selfLink": "/api/v1/namespaces/kube-system/pods/kube-proxy-gke-haissam-default-pool-be5066f1-wnvn", - "uid": "260c2b1d43b094af6d6b4ccba082c2db" + "name": "kube-proxy-gke-haissam-default-pool-be5066f1-wnvn", + "labels": { + "tier": "node", + "component": "kube-proxy" + }, + "namespace": "kube-system", + "creationTimestamp": null, + "annotations": { + "scheduler.alpha.kubernetes.io/critical-pod": "", + "kubernetes.io/config.hash": "260c2b1d43b094af6d6b4ccba082c2db", + "kubernetes.io/config.source": "http", + "kubernetes.io/config.seen": "2018-02-13T16:10:19.507814572Z" + }, + "selfLink": "/api/v1/namespaces/kube-system/pods/kube-proxy-gke-haissam-default-pool-be5066f1-wnvn", + "uid": "260c2b1d43b094af6d6b4ccba082c2db" } } ] -} +} \ No newline at end of file diff --git a/pkg/util/kubernetes/kubelet/types_kubelet.go b/pkg/util/kubernetes/kubelet/types_kubelet.go index 3ec5149266c09c..8adab5a410fcbe 100644 --- a/pkg/util/kubernetes/kubelet/types_kubelet.go +++ b/pkg/util/kubernetes/kubelet/types_kubelet.go @@ -166,6 +166,8 @@ type EnvVar struct { Name string `json:"name"` // Value of the environment variable. Value string `json:"value,omitempty"` + // Source of the environment variable. + ValueFrom *struct{} `json:"valueFrom,omitempty"` } // VolumeSpec contains fields for unmarshalling a Pod.Spec.Volumes From 481ddc76222c3ee56653608abf3e7f025b579668 Mon Sep 17 00:00:00 2001 From: Gabriel Dos Santos Date: Tue, 25 Feb 2025 21:07:05 +0000 Subject: [PATCH 2/6] changelog --- .../kubelet-envvar-handling-55657ebfaa0702b8.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 releasenotes/notes/kubelet-envvar-handling-55657ebfaa0702b8.yaml diff --git a/releasenotes/notes/kubelet-envvar-handling-55657ebfaa0702b8.yaml b/releasenotes/notes/kubelet-envvar-handling-55657ebfaa0702b8.yaml new file mode 100644 index 00000000000000..ffa3a1801a5f57 --- /dev/null +++ b/releasenotes/notes/kubelet-envvar-handling-55657ebfaa0702b8.yaml @@ -0,0 +1,12 @@ +# Each section from every release note are combined when the +# CHANGELOG.rst is rendered. So the text needs to be worded so that +# it does not depend on any information only available in another +# section. This may mean repeating some details, but each section +# must be readable independently of the other. +# +# Each section note must be formatted as reStructuredText. +--- +fixes: + - | + Environment variables that the kubelet collector recieves that are + not explicitly defined in plaintext will now be completely ignored. From ccb9b5a38939a6c951a197e6d953de098f4f6415 Mon Sep 17 00:00:00 2001 From: Gabriel Dos Santos <91925154+gabedos@users.noreply.github.com> Date: Tue, 25 Feb 2025 16:13:42 -0500 Subject: [PATCH 3/6] Update releasenotes/notes/kubelet-envvar-handling-55657ebfaa0702b8.yaml Co-authored-by: May Lee --- .../notes/kubelet-envvar-handling-55657ebfaa0702b8.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/releasenotes/notes/kubelet-envvar-handling-55657ebfaa0702b8.yaml b/releasenotes/notes/kubelet-envvar-handling-55657ebfaa0702b8.yaml index ffa3a1801a5f57..c1b8783d84186b 100644 --- a/releasenotes/notes/kubelet-envvar-handling-55657ebfaa0702b8.yaml +++ b/releasenotes/notes/kubelet-envvar-handling-55657ebfaa0702b8.yaml @@ -8,5 +8,5 @@ --- fixes: - | - Environment variables that the kubelet collector recieves that are - not explicitly defined in plaintext will now be completely ignored. + Environment variables that the kubelet collector receives and are + not explicitly defined in plaintext will now be ignored. From 407ff22a85a8bd3ab5a506b67a00ac9c0b7f39b4 Mon Sep 17 00:00:00 2001 From: Gabriel Dos Santos Date: Wed, 26 Feb 2025 18:33:14 +0000 Subject: [PATCH 4/6] Refactor envvar filtering to only kubelet collector --- .../collectors/internal/kubelet/kubelet.go | 14 ++++++++++++++ internal/third_party/golang/expansion/expand.go | 17 ++++++++++++++--- .../third_party/golang/expansion/expand_test.go | 10 +++++----- pkg/util/kubernetes/kubelet/kubelet.go | 16 ---------------- 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/comp/core/workloadmeta/collectors/internal/kubelet/kubelet.go b/comp/core/workloadmeta/collectors/internal/kubelet/kubelet.go index 764ca0fe851080..af9e608a545d33 100644 --- a/comp/core/workloadmeta/collectors/internal/kubelet/kubelet.go +++ b/comp/core/workloadmeta/collectors/internal/kubelet/kubelet.go @@ -402,6 +402,7 @@ func extractContainerSecurityContext(spec *kubelet.ContainerSpec) *workloadmeta. } func extractEnvFromSpec(envSpec []kubelet.EnvVar) map[string]string { + filterEnvVars(&envSpec) env := make(map[string]string) mappingFunc := expansion.MappingFuncFor(env) @@ -436,6 +437,19 @@ func extractEnvFromSpec(envSpec []kubelet.EnvVar) map[string]string { return env } +// filterEnvVars removes unsupported env var sources (eg. ConfigMap, Secrets, etc.) +func filterEnvVars(envSpec *[]kubelet.EnvVar) { + j := 0 // Position for the next valid element + for _, envVar := range *envSpec { + if envVar.ValueFrom != nil { + continue + } + (*envSpec)[j] = envVar + j++ + } + *envSpec = (*envSpec)[:j] +} + func extractResources(spec *kubelet.ContainerSpec) workloadmeta.ContainerResources { resources := workloadmeta.ContainerResources{} if cpuReq, found := spec.Resources.Requests[kubelet.ResourceCPU]; found { diff --git a/internal/third_party/golang/expansion/expand.go b/internal/third_party/golang/expansion/expand.go index f0928a1ad603d6..3cca16afcfbf9f 100644 --- a/internal/third_party/golang/expansion/expand.go +++ b/internal/third_party/golang/expansion/expand.go @@ -19,6 +19,9 @@ func syntaxWrap(input string) string { // implements the expansion semantics defined in the expansion spec; it // returns the input string wrapped in the expansion syntax if no mapping // for the input is found. +// +// Additionally, it returns a boolean indicating whether the variable was +// found in the context. func MappingFuncFor(context ...map[string]string) func(string) (string, bool) { return func(input string) (string, bool) { for _, vars := range context { @@ -35,9 +38,13 @@ func MappingFuncFor(context ...map[string]string) func(string) (string, bool) { // Expand replaces variable references in the input string according to // the expansion spec using the given mapping function to resolve the // values of variables. +// +// Additionally, it returns the status of whether all nested variables +// have a defined mapping value in the environment. func Expand(input string, mapping func(string) (string, bool)) (string, bool) { var buf bytes.Buffer checkpoint := 0 + allMappingsFound := true for cursor := 0; cursor < len(input); cursor++ { if input[cursor] == operator && cursor+1 < len(input) { // Copy the portion of the input string since the last @@ -53,9 +60,12 @@ func Expand(input string, mapping func(string) (string, bool)) (string, bool) { // apply the mapping to the variable name and copy the // bytes into the buffer mappedValue, found := mapping(read) + + // Record that the read variable is not mapped in the environment if !found { - return "", false + allMappingsFound = false } + buf.WriteString(mappedValue) } else { // Not a variable name; copy the read bytes into the buffer @@ -72,8 +82,9 @@ func Expand(input string, mapping func(string) (string, bool)) (string, bool) { } // Return the buffer and any remaining unwritten bytes in the - // input string. - return buf.String() + input[checkpoint:], true + // input string. Also return whether any nested variables in + // the input string were not found in the environment. + return buf.String() + input[checkpoint:], allMappingsFound } // tryReadVariableName attempts to read a variable name from the input diff --git a/internal/third_party/golang/expansion/expand_test.go b/internal/third_party/golang/expansion/expand_test.go index dc7fa31f565d26..c6a8174abfeee1 100644 --- a/internal/third_party/golang/expansion/expand_test.go +++ b/internal/third_party/golang/expansion/expand_test.go @@ -165,13 +165,13 @@ func doExpansionTest(t *testing.T, mapping func(string) (string, bool)) { { name: "nested var references", input: "$(VAR_A$(VAR_B))", - expected: "", + expected: "$(VAR_A$(VAR_B))", expectedStatus: false, }, { name: "nested var references second type", input: "$(VAR_A$(VAR_B)", - expected: "", + expected: "$(VAR_A$(VAR_B)", expectedStatus: false, }, { @@ -219,7 +219,7 @@ func doExpansionTest(t *testing.T, mapping func(string) (string, bool)) { { name: "undefined vars are passed through", input: "$(VAR_DNE)", - expected: "", + expected: "$(VAR_DNE)", expectedStatus: false, }, { @@ -237,7 +237,7 @@ func doExpansionTest(t *testing.T, mapping func(string) (string, bool)) { { name: "multiple (odd) operators, var undefined", input: "$$$$$$$(GOOD_ODDS)", - expected: "", + expected: "$$$$(GOOD_ODDS)", expectedStatus: false, }, { @@ -303,7 +303,7 @@ func doExpansionTest(t *testing.T, mapping func(string) (string, bool)) { { name: "escaped operators in variable names are not escaped", input: "$(foo$$var)", - expected: "", + expected: "$(foo$$var)", expectedStatus: false, }, { diff --git a/pkg/util/kubernetes/kubelet/kubelet.go b/pkg/util/kubernetes/kubelet/kubelet.go index 34ab1390c9ba25..bcfc961206f672 100644 --- a/pkg/util/kubernetes/kubelet/kubelet.go +++ b/pkg/util/kubernetes/kubelet/kubelet.go @@ -230,8 +230,6 @@ func (ku *KubeUtil) getLocalPodList(ctx context.Context) (*PodList, error) { allContainers = append(allContainers, pod.Status.InitContainers...) allContainers = append(allContainers, pod.Status.Containers...) pod.Status.AllContainers = allContainers - filterEnvVars(pod.Spec.Containers) - filterEnvVars(pod.Spec.InitContainers) tmpSlice = append(tmpSlice, pod) } } @@ -482,17 +480,3 @@ func isPodStatic(pod *Pod) bool { } return false } - -// filterEnvVars removes unsupported env var sources (eg. ConfigMap, Secrets, etc.) -func filterEnvVars(containers []ContainerSpec) { - for i := range containers { - cleanEnvVars := make([]EnvVar, 0) - for _, envVar := range containers[i].Env { - if envVar.ValueFrom != nil { - continue - } - cleanEnvVars = append(cleanEnvVars, envVar) - } - containers[i].Env = cleanEnvVars - } -} From 6ed7532cc80a797e78d290056906cc2d0326a237 Mon Sep 17 00:00:00 2001 From: Gabriel Dos Santos Date: Wed, 26 Feb 2025 20:28:27 +0000 Subject: [PATCH 5/6] More kubelet tests --- .../collectors/internal/kubelet/kubelet_test.go | 17 +++++++++++++++-- pkg/util/kubernetes/kubelet/kubelet_test.go | 3 +++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/comp/core/workloadmeta/collectors/internal/kubelet/kubelet_test.go b/comp/core/workloadmeta/collectors/internal/kubelet/kubelet_test.go index 96ebb1a7abe67d..0990c29a787827 100644 --- a/comp/core/workloadmeta/collectors/internal/kubelet/kubelet_test.go +++ b/comp/core/workloadmeta/collectors/internal/kubelet/kubelet_test.go @@ -65,6 +65,17 @@ func TestPodParser(t *testing.T) { "cpu": resource.MustParse("100m"), }, }, + Env: []kubelet.EnvVar{ + { + Name: "ExplicitEnvVar", + Value: "true", + }, + { + Name: "ImplicitEnvVar", + Value: "", + ValueFrom: &struct{}{}, + }, + }, }, }, }, @@ -121,8 +132,10 @@ func TestPodParser(t *testing.T) { Kind: "kubernetes_pod", ID: "uniqueIdentifier", }, - Ports: []workloadmeta.ContainerPort{}, - EnvVars: map[string]string{}, + Ports: []workloadmeta.ContainerPort{}, + EnvVars: map[string]string{ + "ExplicitEnvVar": "true", + }, State: workloadmeta.ContainerState{ Health: "healthy", }, diff --git a/pkg/util/kubernetes/kubelet/kubelet_test.go b/pkg/util/kubernetes/kubelet/kubelet_test.go index 5b330fb274d125..848f3f2412a635 100644 --- a/pkg/util/kubernetes/kubelet/kubelet_test.go +++ b/pkg/util/kubernetes/kubelet/kubelet_test.go @@ -883,7 +883,10 @@ func (suite *KubeletTestSuite) TestContainerEnvVars() { require.NotNil(suite.T(), nginxContainer) expectedEnvVars := []EnvVar{ + // Variable explicitly defined in the pod spec {Name: "DEFINED_VAR", Value: "true"}, + // Variable from an external source (eg. ConfigMap, Secret, Downward API, etc.) + {Name: "UNDEFINED_VAR", Value: "", ValueFrom: &struct{}{}}, } assert.ElementsMatch(suite.T(), nginxContainer.Env, expectedEnvVars) } From bbe4a9454cf8ebd3ca6f41edcbbdae0307111240 Mon Sep 17 00:00:00 2001 From: Gabriel Dos Santos Date: Wed, 26 Feb 2025 23:00:46 +0000 Subject: [PATCH 6/6] Config kubelet tests --- .../collectors/internal/kubelet/kubelet_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/comp/core/workloadmeta/collectors/internal/kubelet/kubelet_test.go b/comp/core/workloadmeta/collectors/internal/kubelet/kubelet_test.go index 0990c29a787827..a3ed6707476b08 100644 --- a/comp/core/workloadmeta/collectors/internal/kubelet/kubelet_test.go +++ b/comp/core/workloadmeta/collectors/internal/kubelet/kubelet_test.go @@ -67,11 +67,15 @@ func TestPodParser(t *testing.T) { }, Env: []kubelet.EnvVar{ { - Name: "ExplicitEnvVar", - Value: "true", + Name: "DD_ENV", + Value: "prod", }, { - Name: "ImplicitEnvVar", + Name: "OTEL_SERVICE_NAME", + Value: "$(DD_ENV)-$(DD_EXCLUDED_VAR)", + }, + { + Name: "DD_EXCLUDED_VAR", Value: "", ValueFrom: &struct{}{}, }, @@ -134,7 +138,7 @@ func TestPodParser(t *testing.T) { }, Ports: []workloadmeta.ContainerPort{}, EnvVars: map[string]string{ - "ExplicitEnvVar": "true", + "DD_ENV": "prod", }, State: workloadmeta.ContainerState{ Health: "healthy",