Merge pull request #2921 from Colstuwjx/common-flag-env-metadata-whitelist

Common flag env metadata whitelist

Author: bobbypage

build/integration.sh

@@ -27,7 +27,7 @@ function start {
   set +e  # We want to handle errors if cAdvisor crashes.
   echo ">> starting cAdvisor locally"
   # This cpuset, percpu, memory, disk, diskIO, network, perf_event metrics should be enabled.
-  GORACE="halt_on_error=1" ./cadvisor --enable_metrics="cpuset,percpu,memory,disk,diskIO,network,perf_event" --docker_env_metadata_whitelist=TEST_VAR --v=6 --logtostderr $CADVISOR_ARGS &> "$log_file"
+  GORACE="halt_on_error=1" ./cadvisor --enable_metrics="cpuset,percpu,memory,disk,diskIO,network,perf_event" --env_metadata_whitelist=TEST_VAR --v=6 --logtostderr $CADVISOR_ARGS &> "$log_file"
   if [ $? != 0 ]; then
     echo "!! cAdvisor exited unexpectedly with Exit $?"
     kill $TEST_PID # cAdvisor crashed: abort testing.

cmd/cadvisor.go

@@ -65,6 +65,8 @@ var collectorKey = flag.String("collector_key", "", "Key for the collector's cer
 var storeContainerLabels = flag.Bool("store_container_labels", true, "convert container labels and environment variables into labels on prometheus metrics for each container. If flag set to false, then only metrics exported are container name, first alias, and image name")
 var whitelistedContainerLabels = flag.String("whitelisted_container_labels", "", "comma separated list of container labels to be converted to labels on prometheus metrics for each container. store_container_labels must be set to false for this to take effect.")
 
+var envMetadataWhiteList = flag.String("env_metadata_whitelist", "", "a comma-separated list of environment variable keys matched with specified prefix that needs to be collected for containers, only support containerd and docker runtime for now.")
+
 var urlBasePrefix = flag.String("url_base_prefix", "", "prefix path that will be prepended to all paths to support some reverse proxies")
 
 var rawCgroupPrefixWhiteList = flag.String("raw_cgroup_prefix_whitelist", "", "A comma-separated list of cgroup path prefix that needs to be collected even when -docker_only is specified")
@@ -129,7 +131,7 @@ func main() {
 
 	collectorHttpClient := createCollectorHttpClient(*collectorCert, *collectorKey)
 
-	resourceManager, err := manager.New(memoryStorage, sysFs, manager.HousekeepingConfigFlags, includedMetrics, &collectorHttpClient, strings.Split(*rawCgroupPrefixWhiteList, ","), *perfEvents)
+	resourceManager, err := manager.New(memoryStorage, sysFs, manager.HousekeepingConfigFlags, includedMetrics, &collectorHttpClient, strings.Split(*rawCgroupPrefixWhiteList, ","), strings.Split(*envMetadataWhiteList, ","), *perfEvents)
 	if err != nil {
 		klog.Fatalf("Failed to create a manager: %s", err)
 	}

cmd/internal/container/mesos/factory.go

@@ -59,7 +59,7 @@ func (f *mesosFactory) String() string {
 	return MesosNamespace
 }
 
-func (f *mesosFactory) NewContainerHandler(name string, inHostNamespace bool) (container.ContainerHandler, error) {
+func (f *mesosFactory) NewContainerHandler(name string, metadataEnvAllowList []string, inHostNamespace bool) (container.ContainerHandler, error) {
 	client, err := Client()
 	if err != nil {
 		return nil, err
@@ -72,6 +72,7 @@ func (f *mesosFactory) NewContainerHandler(name string, inHostNamespace bool) (c
 		f.fsInfo,
 		f.includedMetrics,
 		inHostNamespace,
+		metadataEnvAllowList,
 		client,
 	)
 }

cmd/internal/container/mesos/handler.go

@@ -57,6 +57,7 @@ func newMesosContainerHandler(
 	fsInfo fs.FsInfo,
 	includedMetrics container.MetricSet,
 	inHostNamespace bool,
+	metadataEnvAllowList []string,
 	client mesosAgentClient,
 ) (container.ContainerHandler, error) {
 	cgroupPaths := common.MakeCgroupPaths(cgroupSubsystems.MountPoints, name)

cmd/internal/container/mesos/handler_test.go

@@ -37,13 +37,14 @@ func PopulateContainer() *mContainer {
 func TestContainerReference(t *testing.T) {
 	as := assert.New(t)
 	type testCase struct {
-		client             mesosAgentClient
-		name               string
-		machineInfoFactory info.MachineInfoFactory
-		fsInfo             fs.FsInfo
-		cgroupSubsystems   *containerlibcontainer.CgroupSubsystems
-		inHostNamespace    bool
-		includedMetrics    container.MetricSet
+		client               mesosAgentClient
+		name                 string
+		machineInfoFactory   info.MachineInfoFactory
+		fsInfo               fs.FsInfo
+		cgroupSubsystems     *containerlibcontainer.CgroupSubsystems
+		inHostNamespace      bool
+		metadataEnvAllowList []string
+		includedMetrics      container.MetricSet
 
 		hasErr         bool
 		errContains    string
@@ -57,6 +58,7 @@ func TestContainerReference(t *testing.T) {
 			nil,
 			&containerlibcontainer.CgroupSubsystems{},
 			false,
+			[]string{},
 			nil,
 
 			true,
@@ -70,6 +72,7 @@ func TestContainerReference(t *testing.T) {
 			nil,
 			&containerlibcontainer.CgroupSubsystems{},
 			false,
+			[]string{},
 			nil,
 
 			true,
@@ -83,6 +86,7 @@ func TestContainerReference(t *testing.T) {
 			nil,
 			&containerlibcontainer.CgroupSubsystems{},
 			false,
+			[]string{},
 			nil,
 
 			false,
@@ -95,7 +99,7 @@ func TestContainerReference(t *testing.T) {
 			},
 		},
 	} {
-		handler, err := newMesosContainerHandler(ts.name, ts.cgroupSubsystems, ts.machineInfoFactory, ts.fsInfo, ts.includedMetrics, ts.inHostNamespace, ts.client)
+		handler, err := newMesosContainerHandler(ts.name, ts.cgroupSubsystems, ts.machineInfoFactory, ts.fsInfo, ts.includedMetrics, ts.inHostNamespace, ts.metadataEnvAllowList, ts.client)
 		if ts.hasErr {
 			as.NotNil(err)
 			if ts.errContains != "" {

container/containerd/factory.go

@@ -34,15 +34,15 @@ import (
 var ArgContainerdEndpoint = flag.String("containerd", "/run/containerd/containerd.sock", "containerd endpoint")
 var ArgContainerdNamespace = flag.String("containerd-namespace", "k8s.io", "containerd namespace")
 
+var containerdEnvMetadataWhiteList = flag.String("containerd_env_metadata_whitelist", "", "DEPRECATED: this flag will be removed, please use `env_metadata_whitelist`. A comma-separated list of environment variable keys matched with specified prefix that needs to be collected for containerd containers")
+
 // The namespace under which containerd aliases are unique.
 const k8sContainerdNamespace = "containerd"
 
 // Regexp that identifies containerd cgroups, containers started with
 // --cgroup-parent have another prefix than 'containerd'
 var containerdCgroupRegexp = regexp.MustCompile(`([a-z0-9]{64})`)
 
-var containerdEnvWhitelist = flag.String("containerd_env_metadata_whitelist", "", "a comma-separated list of environment variable keys matched with specified prefix that needs to be collected for containerd containers")
-
 type containerdFactory struct {
 	machineInfoFactory info.MachineInfoFactory
 	client             ContainerdClient
@@ -58,13 +58,18 @@ func (f *containerdFactory) String() string {
 	return k8sContainerdNamespace
 }
 
-func (f *containerdFactory) NewContainerHandler(name string, inHostNamespace bool) (handler container.ContainerHandler, err error) {
+func (f *containerdFactory) NewContainerHandler(name string, metadataEnvAllowList []string, inHostNamespace bool) (handler container.ContainerHandler, err error) {
 	client, err := Client(*ArgContainerdEndpoint, *ArgContainerdNamespace)
 	if err != nil {
 		return
 	}
 
-	metadataEnvs := strings.Split(*containerdEnvWhitelist, ",")
+	containerdMetadataEnvAllowList := strings.Split(*containerdEnvMetadataWhiteList, ",")
+
+	// prefer using the unified metadataEnvAllowList
+	if len(metadataEnvAllowList) != 0 {
+		containerdMetadataEnvAllowList = metadataEnvAllowList
+	}
 
 	return newContainerdContainerHandler(
 		client,
@@ -73,7 +78,7 @@ func (f *containerdFactory) NewContainerHandler(name string, inHostNamespace boo
 		f.fsInfo,
 		&f.cgroupSubsystems,
 		inHostNamespace,
-		metadataEnvs,
+		containerdMetadataEnvAllowList,
 		f.includedMetrics,
 	)
 }

container/containerd/handler.go

@@ -60,7 +60,7 @@ func newContainerdContainerHandler(
 	fsInfo fs.FsInfo,
 	cgroupSubsystems *containerlibcontainer.CgroupSubsystems,
 	inHostNamespace bool,
-	metadataEnvs []string,
+	metadataEnvAllowList []string,
 	includedMetrics container.MetricSet,
 ) (container.ContainerHandler, error) {
 	// Create the cgroup paths.
@@ -134,9 +134,9 @@ func newContainerdContainerHandler(
 	// Add the name and bare ID as aliases of the container.
 	handler.image = cntr.Image
 
-	for _, exposedEnv := range metadataEnvs {
+	for _, exposedEnv := range metadataEnvAllowList {
 		if exposedEnv == "" {
-			// if no containerdEnvWhitelist provided, len(metadataEnvs) == 1, metadataEnvs[0] == ""
+			// if no containerdEnvWhitelist provided, len(metadataEnvAllowList) == 1, metadataEnvAllowList[0] == ""
 			continue
 		}
 

container/containerd/handler_test.go

@@ -45,14 +45,14 @@ func (m *mockedMachineInfo) GetVersionInfo() (*info.VersionInfo, error) {
 func TestHandler(t *testing.T) {
 	as := assert.New(t)
 	type testCase struct {
-		client             ContainerdClient
-		name               string
-		machineInfoFactory info.MachineInfoFactory
-		fsInfo             fs.FsInfo
-		cgroupSubsystems   *containerlibcontainer.CgroupSubsystems
-		inHostNamespace    bool
-		metadataEnvs       []string
-		includedMetrics    container.MetricSet
+		client               ContainerdClient
+		name                 string
+		machineInfoFactory   info.MachineInfoFactory
+		fsInfo               fs.FsInfo
+		cgroupSubsystems     *containerlibcontainer.CgroupSubsystems
+		inHostNamespace      bool
+		metadataEnvAllowList []string
+		includedMetrics      container.MetricSet
 
 		hasErr         bool
 		errContains    string
@@ -121,7 +121,7 @@ func TestHandler(t *testing.T) {
 			map[string]string{"TEST_REGION": "FRA", "TEST_ZONE": "A"},
 		},
 	} {
-		handler, err := newContainerdContainerHandler(ts.client, ts.name, ts.machineInfoFactory, ts.fsInfo, ts.cgroupSubsystems, ts.inHostNamespace, ts.metadataEnvs, ts.includedMetrics)
+		handler, err := newContainerdContainerHandler(ts.client, ts.name, ts.machineInfoFactory, ts.fsInfo, ts.cgroupSubsystems, ts.inHostNamespace, ts.metadataEnvAllowList, ts.includedMetrics)
 		if ts.hasErr {
 			as.NotNil(err)
 			if ts.errContains != "" {

container/crio/factory.go

@@ -64,13 +64,11 @@ func (f *crioFactory) String() string {
 	return CrioNamespace
 }
 
-func (f *crioFactory) NewContainerHandler(name string, inHostNamespace bool) (handler container.ContainerHandler, err error) {
+func (f *crioFactory) NewContainerHandler(name string, metadataEnvAllowList []string, inHostNamespace bool) (handler container.ContainerHandler, err error) {
 	client, err := Client()
 	if err != nil {
 		return
 	}
-	// TODO are there any env vars we need to white list, if so, do it here...
-	metadataEnvs := []string{}
 	handler, err = newCrioContainerHandler(
 		client,
 		name,
@@ -80,7 +78,7 @@ func (f *crioFactory) NewContainerHandler(name string, inHostNamespace bool) (ha
 		f.storageDir,
 		&f.cgroupSubsystems,
 		inHostNamespace,
-		metadataEnvs,
+		metadataEnvAllowList,
 		f.includedMetrics,
 	)
 	return

container/crio/handler.go

@@ -85,7 +85,7 @@ func newCrioContainerHandler(
 	storageDir string,
 	cgroupSubsystems *containerlibcontainer.CgroupSubsystems,
 	inHostNamespace bool,
-	metadataEnvs []string,
+	metadataEnvAllowList []string,
 	includedMetrics container.MetricSet,
 ) (container.ContainerHandler, error) {
 	// Create the cgroup paths.
@@ -186,7 +186,7 @@ func newCrioContainerHandler(
 		handler.fsHandler = common.NewFsHandler(common.DefaultPeriod, rootfsStorageDir, storageLogDir, fsInfo)
 	}
 	// TODO for env vars we wanted to show from container.Config.Env from whitelist
-	//for _, exposedEnv := range metadataEnvs {
+	//for _, exposedEnv := range metadataEnvAllowList {
 	//klog.V(4).Infof("TODO env whitelist: %v", exposedEnv)
 	//}