From 311c127550e67c22abaaa21d13f7189c806ad13c Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Fri, 23 Aug 2024 17:30:10 +0000 Subject: [PATCH 01/27] enable health monitoring mode for NPD --- launcher/container_runner.go | 14 +--- .../nodeproblemdetector/systemstats_config.go | 56 ++++++++++++-- .../systemstats_config_test.go | 29 +++++++- launcher/launcher/main.go | 14 ++++ launcher/spec/launch_policy.go | 74 +++++++++++++++---- launcher/spec/launch_spec.go | 11 +++ 6 files changed, 167 insertions(+), 31 deletions(-) diff --git a/launcher/container_runner.go b/launcher/container_runner.go index e7ce6d6a1..fb95ae134 100644 --- a/launcher/container_runner.go +++ b/launcher/container_runner.go @@ -29,8 +29,8 @@ import ( "github.com/google/go-tpm-tools/cel" "github.com/google/go-tpm-tools/client" "github.com/google/go-tpm-tools/launcher/agent" + "github.com/google/go-tpm-tools/launcher/internal/healthmonitoring/nodeproblemdetector" "github.com/google/go-tpm-tools/launcher/internal/signaturediscovery" - "github.com/google/go-tpm-tools/launcher/internal/systemctl" "github.com/google/go-tpm-tools/launcher/launcherfile" "github.com/google/go-tpm-tools/launcher/registryauth" "github.com/google/go-tpm-tools/launcher/spec" @@ -524,17 +524,9 @@ func (r *ContainerRunner) Run(ctx context.Context) error { // start node-problem-detector.service to collect memory related metrics. if r.launchSpec.MemoryMonitoringEnabled { r.logger.Println("MemoryMonitoring is enabled by the VM operator") - s, err := systemctl.New() - if err != nil { - return fmt.Errorf("failed to create systemctl client: %v", err) - } - defer s.Close() - - r.logger.Println("Starting a systemctl operation: systemctl start node-problem-detector.service") - if err := s.Start("node-problem-detector.service"); err != nil { - return fmt.Errorf("failed to start node-problem-detector.service: %v", err) + if err := nodeproblemdetector.StartService(r.logger); err != nil { + return err } - r.logger.Println("node-problem-detector.service successfully started.") } else { r.logger.Println("MemoryMonitoring is disabled by the VM operator") } diff --git a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go index 6c97ee294..29fabf883 100644 --- a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go +++ b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go @@ -4,17 +4,22 @@ package nodeproblemdetector import ( "encoding/json" "fmt" + "log" "os" "time" + + "github.com/google/go-tpm-tools/launcher/internal/systemctl" ) +var systemStatsFilePath = "/etc/node_problem_detector/system-stats-monitor.json" + var defaultInvokeIntervalString = (60 * time.Second).String() type metricConfig struct { DisplayName string `json:"displayName"` } -type memoryStatsConfig struct { +type statsConfig struct { MetricsConfigs map[string]metricConfig `json:"metricsConfigs"` } @@ -23,21 +28,44 @@ type memoryStatsConfig struct { // For now we only consider collecting memory related metrics. // View the comprehensive configuration details on https://github.com/kubernetes/node-problem-detector/tree/master/pkg/systemstatsmonitor#detailed-configuration-options type SystemStatsConfig struct { - MemoryStatsConfig memoryStatsConfig `json:"memory"` - InvokeInterval string `json:"invokeInterval"` + Cpu *statsConfig `json:"cpu,omitempty"` + Disk *statsConfig `json:"disk,omitempty"` + Host *statsConfig `json:"host,omitempty"` + Memory *statsConfig `json:"memory,omitempty"` + InvokeInterval string `json:"invokeInterval,omitempty"` } // NewSystemStatsConfig returns a new SystemStatsConfig struct with default configurations. func NewSystemStatsConfig() SystemStatsConfig { return SystemStatsConfig{ - MemoryStatsConfig: memoryStatsConfig{MetricsConfigs: map[string]metricConfig{}}, - InvokeInterval: defaultInvokeIntervalString, + Memory: &statsConfig{MetricsConfigs: map[string]metricConfig{}}, + InvokeInterval: defaultInvokeIntervalString, } } +var healthConfig = &SystemStatsConfig{ + Cpu: &statsConfig{map[string]metricConfig{ + "cpu/load_5m": {"cpu/load_5m"}, + }}, + Disk: &statsConfig{map[string]metricConfig{ + "disk/percent_used": {"disk/percent_used"}, + }}, + Host: &statsConfig{map[string]metricConfig{ + "host/uptime": {"host/uptime"}, + }}, + Memory: &statsConfig{map[string]metricConfig{ + "memory/bytes_used": {"memory/bytes_used"}, + }}, + InvokeInterval: defaultInvokeIntervalString, +} + +func EnableHealthMonitoringConfig() error { + return healthConfig.WriteFile(systemStatsFilePath) +} + // EnableMemoryBytesUsed enables "memory/bytes_used" for memory monitoring. func (ssc *SystemStatsConfig) EnableMemoryBytesUsed() { - ssc.MemoryStatsConfig.MetricsConfigs["memory/bytes_used"] = metricConfig{DisplayName: "memory/bytes_used"} + ssc.Memory.MetricsConfigs["memory/bytes_used"] = metricConfig{DisplayName: "memory/bytes_used"} } // WithInvokeInterval overrides the default invokeInterval. @@ -53,3 +81,19 @@ func (ssc *SystemStatsConfig) WriteFile(path string) error { } return os.WriteFile(path, bytes, 0644) } + +func StartService(logger *log.Logger) error { + s, err := systemctl.New() + if err != nil { + return fmt.Errorf("failed to create systemctl client: %v", err) + } + defer s.Close() + + logger.Printf("Starting node-problem-detector.service") + if err := s.Start("node-problem-detector.service"); err != nil { + return fmt.Errorf("failed to start node-problem-detector.service") + } + + logger.Printf("node-problem-detector.service successfully started") + return nil +} diff --git a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config_test.go b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config_test.go index 2201615aa..6176d5a32 100644 --- a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config_test.go +++ b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config_test.go @@ -2,6 +2,7 @@ package nodeproblemdetector import ( "bytes" + "encoding/json" "io" "os" "path" @@ -11,12 +12,38 @@ import ( "github.com/google/go-cmp/cmp" ) +func TestEnableHealthMonitoringConfig(t *testing.T) { + tmpDir := t.TempDir() + systemStatsFilePath = path.Join(tmpDir, "system-stats-monitor.json") + + wantBytes, err := json.Marshal(healthConfig) + if err != nil { + t.Fatalf("Error marshaling expected config: %v", err) + } + + EnableHealthMonitoringConfig() + + file, err := os.OpenFile(systemStatsFilePath, os.O_RDONLY, 0) + if err != nil { + t.Fatalf("failed to open file %s: %v", systemStatsFilePath, err) + } + + gotBytes, err := io.ReadAll(file) + if err != nil { + t.Fatalf("failed to read from file %s: %v", systemStatsFilePath, err) + } + + if !bytes.Equal(gotBytes, wantBytes) { + t.Errorf("WriteFile() did not write expected contents, got %s, want %s", gotBytes, wantBytes) + } +} + func TestEnableMemoryBytesUsed(t *testing.T) { got := NewSystemStatsConfig() got.EnableMemoryBytesUsed() want := SystemStatsConfig{ - MemoryStatsConfig: memoryStatsConfig{ + Memory: &statsConfig{ MetricsConfigs: map[string]metricConfig{ "memory/bytes_used": {DisplayName: "memory/bytes_used"}, }, diff --git a/launcher/launcher/main.go b/launcher/launcher/main.go index 9181d4e61..b884ed00c 100644 --- a/launcher/launcher/main.go +++ b/launcher/launcher/main.go @@ -18,6 +18,7 @@ import ( "github.com/containerd/containerd/namespaces" "github.com/google/go-tpm-tools/client" "github.com/google/go-tpm-tools/launcher" + "github.com/google/go-tpm-tools/launcher/internal/healthmonitoring/nodeproblemdetector" "github.com/google/go-tpm-tools/launcher/launcherfile" "github.com/google/go-tpm-tools/launcher/registryauth" "github.com/google/go-tpm-tools/launcher/spec" @@ -95,6 +96,19 @@ func main() { return } + if launchSpec.HealthMonitoringEnabled { + logger.Printf("Health Monitoring is enabled by the VM operator") + + if err := nodeproblemdetector.EnableHealthMonitoringConfig(); err != nil { + logger.Printf("failed to enable Health Monitoring config: %v", err) + return + } + + if err := nodeproblemdetector.StartService(logger); err != nil { + logger.Print(err) + } + } + defer func() { // Catch panic to attempt to output to Cloud Logging. if r := recover(); r != nil { diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index 71467eef6..0056dee56 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -14,8 +14,9 @@ type LaunchPolicy struct { AllowedEnvOverride []string AllowedCmdOverride bool AllowedLogRedirect policy - AllowedMemoryMonitoring policy AllowedMountDestinations []string + HardenedMonitoring monitoringType + DebugMonitoring monitoringType } type policy int @@ -26,6 +27,14 @@ const ( never ) +type monitoringType int + +const ( + None monitoringType = iota + MemoryOnly + Health +) + // String returns LaunchPolicy details. func (p policy) String() string { switch p { @@ -57,10 +66,12 @@ func toPolicy(policy, s string) (policy, error) { } const ( - envOverride = "tee.launch_policy.allow_env_override" - cmdOverride = "tee.launch_policy.allow_cmd_override" - logRedirect = "tee.launch_policy.log_redirect" - memoryMonitoring = "tee.launch_policy.monitoring_memory_allow" + envOverride = "tee.launch_policy.allow_env_override" + cmdOverride = "tee.launch_policy.allow_cmd_override" + logRedirect = "tee.launch_policy.log_redirect" + memoryMonitoring = "tee.launch_policy.monitoring_memory_allow" + hardenedMonitoring = "tee.launch_policy.hardened_monitoring" + debugMonitoring = "tee.launch_policy.debug_monitoring" // Values look like a PATH list, with ':' as a separator. // Empty paths will be ignored and relative paths will be interpreted as // relative to "/". @@ -68,6 +79,19 @@ const ( mountDestinations = "tee.launch_policy.allow_mount_destinations" ) +func toMonitoringType(label string) (monitoringType, error) { + switch strings.ToUpper(label) { + case "NONE": + return None, nil + case "MEMORY_ONLY": + return MemoryOnly, nil + case "HEALTH": + return Health, nil + } + + return None, fmt.Errorf("Invalid monitoring type: %v", label) +} + // GetLaunchPolicy takes in a map[string] string which should come from image labels, // and will try to parse it into a LaunchPolicy. Extra fields will be ignored. func GetLaunchPolicy(imageLabels map[string]string) (LaunchPolicy, error) { @@ -97,12 +121,26 @@ func GetLaunchPolicy(imageLabels map[string]string) (LaunchPolicy, error) { } } - // default is debug only for memoryMonitoring - if v, ok := imageLabels[memoryMonitoring]; ok { - launchPolicy.AllowedMemoryMonitoring, err = toPolicy(memoryMonitoring, v) + if _, ok := imageLabels[memoryMonitoring]; ok { + return LaunchPolicy{}, fmt.Errorf("%v label is deprecated - use %v and %v instead", memoryMonitoring, hardenedMonitoring, debugMonitoring) + } + + if v, ok := imageLabels[hardenedMonitoring]; ok { + launchPolicy.HardenedMonitoring, err = toMonitoringType(v) + if err != nil { + return LaunchPolicy{}, fmt.Errorf("invalid monitoring type for hardened image: %v", err) + } + } else { + launchPolicy.HardenedMonitoring = None + } + + if v, ok := imageLabels[debugMonitoring]; ok { + launchPolicy.DebugMonitoring, err = toMonitoringType(v) if err != nil { - return LaunchPolicy{}, fmt.Errorf("invalid image LABEL '%s'; contact the image author", memoryMonitoring) + return LaunchPolicy{}, fmt.Errorf("invalid monitoring type for debug image: %v", err) } + } else { + launchPolicy.DebugMonitoring = Health } if v, ok := imageLabels[mountDestinations]; ok { @@ -140,12 +178,22 @@ func (p LaunchPolicy) Verify(ls LaunchSpec) error { return fmt.Errorf("logging redirection only allowed on debug environment by image") } - if p.AllowedMemoryMonitoring == never && ls.MemoryMonitoringEnabled { - return fmt.Errorf("memory monitoring not allowed by image") + monitoringPolicy := p.DebugMonitoring + if ls.Hardened { + monitoringPolicy = p.HardenedMonitoring } - if p.AllowedMemoryMonitoring == debugOnly && ls.MemoryMonitoringEnabled && ls.Hardened { - return fmt.Errorf("memory monitoring only allowed on debug environment by image") + if ls.HealthMonitoringEnabled { + // Return error if policy does not allow health monitoring. + if monitoringPolicy != Health { + return fmt.Errorf("image does not allow health monitoring") + } + } + + if ls.MemoryMonitoringEnabled { + if monitoringPolicy == None { + return fmt.Errorf("image does not allow any monitoring") + } } var err error diff --git a/launcher/spec/launch_spec.go b/launcher/spec/launch_spec.go index 9c5f13ff1..77a21537b 100644 --- a/launcher/spec/launch_spec.go +++ b/launcher/spec/launch_spec.go @@ -83,6 +83,7 @@ const ( attestationServiceAddrKey = "tee-attestation-service-endpoint" logRedirectKey = "tee-container-log-redirect" memoryMonitoringEnable = "tee-monitoring-memory-enable" + healthMonitoringEnable = "tee-health-monitoring-enable" devShmSizeKey = "tee-dev-shm-size-kb" mountKey = "tee-mount" ) @@ -114,6 +115,7 @@ type LaunchSpec struct { Region string Hardened bool MemoryMonitoringEnabled bool + HealthMonitoringEnabled bool LogRedirect LogRedirectLocation Mounts []launchermount.Mount // DevShmSize is specified in kiB. @@ -154,12 +156,21 @@ func (s *LaunchSpec) UnmarshalJSON(b []byte) error { s.SignedImageRepos = append(s.SignedImageRepos, imageRepos...) } + + memoryProhibited := false if val, ok := unmarshaledMap[memoryMonitoringEnable]; ok && val != "" { if boolValue, err := strconv.ParseBool(val); err == nil { s.MemoryMonitoringEnabled = boolValue + if !boolValue { + memoryProhibited = true + } } } + if val, ok := unmarshaledMap[healthMonitoringEnable]; ok && val != "" { + if memoryProhibited + } + // Populate cmd override. if val, ok := unmarshaledMap[cmdKey]; ok && val != "" { if err := json.Unmarshal([]byte(val), &s.Cmd); err != nil { From 00066e03cffd348c4e8db2b53c41d43c2e832af5 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Fri, 23 Aug 2024 17:48:06 +0000 Subject: [PATCH 02/27] fix bugs in launchspec --- launcher/spec/launch_spec.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/launcher/spec/launch_spec.go b/launcher/spec/launch_spec.go index 77a21537b..2ee016b40 100644 --- a/launcher/spec/launch_spec.go +++ b/launcher/spec/launch_spec.go @@ -156,7 +156,6 @@ func (s *LaunchSpec) UnmarshalJSON(b []byte) error { s.SignedImageRepos = append(s.SignedImageRepos, imageRepos...) } - memoryProhibited := false if val, ok := unmarshaledMap[memoryMonitoringEnable]; ok && val != "" { if boolValue, err := strconv.ParseBool(val); err == nil { @@ -168,7 +167,14 @@ func (s *LaunchSpec) UnmarshalJSON(b []byte) error { } if val, ok := unmarshaledMap[healthMonitoringEnable]; ok && val != "" { - if memoryProhibited + if boolValue, err := strconv.ParseBool(val); err == nil { + // If Health Monitoring is enabled but Memory Monitoring is disabled, this is contradictory. + if boolValue && memoryProhibited { + return fmt.Errorf("Health monitoring is enabled but memory monitoring is disabled.") + } + + s.HealthMonitoringEnabled = boolValue + } } // Populate cmd override. From f3ff5e007e7a86459751f37bd73c2fe6ed75227f Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Fri, 23 Aug 2024 21:42:33 +0000 Subject: [PATCH 03/27] fix test failures --- launcher/spec/launch_policy.go | 16 ++++++------ launcher/spec/launch_policy_test.go | 39 +++++++++++++++++++---------- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index 0056dee56..0cf5238a3 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -15,8 +15,8 @@ type LaunchPolicy struct { AllowedCmdOverride bool AllowedLogRedirect policy AllowedMountDestinations []string - HardenedMonitoring monitoringType - DebugMonitoring monitoringType + HardenedImageMonitoring monitoringType + DebugImageMonitoring monitoringType } type policy int @@ -126,21 +126,21 @@ func GetLaunchPolicy(imageLabels map[string]string) (LaunchPolicy, error) { } if v, ok := imageLabels[hardenedMonitoring]; ok { - launchPolicy.HardenedMonitoring, err = toMonitoringType(v) + launchPolicy.HardenedImageMonitoring, err = toMonitoringType(v) if err != nil { return LaunchPolicy{}, fmt.Errorf("invalid monitoring type for hardened image: %v", err) } } else { - launchPolicy.HardenedMonitoring = None + launchPolicy.HardenedImageMonitoring = None } if v, ok := imageLabels[debugMonitoring]; ok { - launchPolicy.DebugMonitoring, err = toMonitoringType(v) + launchPolicy.DebugImageMonitoring, err = toMonitoringType(v) if err != nil { return LaunchPolicy{}, fmt.Errorf("invalid monitoring type for debug image: %v", err) } } else { - launchPolicy.DebugMonitoring = Health + launchPolicy.DebugImageMonitoring = Health } if v, ok := imageLabels[mountDestinations]; ok { @@ -178,9 +178,9 @@ func (p LaunchPolicy) Verify(ls LaunchSpec) error { return fmt.Errorf("logging redirection only allowed on debug environment by image") } - monitoringPolicy := p.DebugMonitoring + monitoringPolicy := p.DebugImageMonitoring if ls.Hardened { - monitoringPolicy = p.HardenedMonitoring + monitoringPolicy = p.HardenedImageMonitoring } if ls.HealthMonitoringEnabled { diff --git a/launcher/spec/launch_policy_test.go b/launcher/spec/launch_policy_test.go index dc9382724..d568aec36 100644 --- a/launcher/spec/launch_policy_test.go +++ b/launcher/spec/launch_policy_test.go @@ -57,6 +57,10 @@ func TestLaunchPolicy(t *testing.T) { for _, testcase := range testCases { t.Run(testcase.testName, func(t *testing.T) { + // Add default values for policy fields. Not relevant to tested behavior. + testcase.expectedPolicy.HardenedImageMonitoring = None + testcase.expectedPolicy.DebugImageMonitoring = Health + got, err := GetLaunchPolicy(testcase.imageLabels) if err != nil { t.Fatal(err) @@ -82,7 +86,8 @@ func TestVerify(t *testing.T) { AllowedEnvOverride: []string{"foo"}, AllowedCmdOverride: true, AllowedLogRedirect: always, - AllowedMemoryMonitoring: always, + HardenedImageMonitoring: MemoryOnly, + DebugImageMonitoring: MemoryOnly, }, LaunchSpec{ Envs: []EnvVar{{Name: "foo", Value: "foo"}}, @@ -121,7 +126,8 @@ func TestVerify(t *testing.T) { { "memory monitor (never, enable, hardened): err", LaunchPolicy{ - AllowedMemoryMonitoring: never, + HardenedImageMonitoring: None, + DebugImageMonitoring: None, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -133,7 +139,8 @@ func TestVerify(t *testing.T) { { "memory monitor (never, enable, debug): err", LaunchPolicy{ - AllowedMemoryMonitoring: never, + HardenedImageMonitoring: None, + DebugImageMonitoring: None, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -145,7 +152,8 @@ func TestVerify(t *testing.T) { { "memory monitor (never, disable, hardened): noerr", LaunchPolicy{ - AllowedMemoryMonitoring: never, + HardenedImageMonitoring: None, + DebugImageMonitoring: None, }, LaunchSpec{ MemoryMonitoringEnabled: false, @@ -157,7 +165,8 @@ func TestVerify(t *testing.T) { { "memory monitor (never, disable, debug): noerr", LaunchPolicy{ - AllowedMemoryMonitoring: never, + HardenedImageMonitoring: None, + DebugImageMonitoring: None, }, LaunchSpec{ MemoryMonitoringEnabled: false, @@ -169,7 +178,7 @@ func TestVerify(t *testing.T) { { "memory monitor (debugonly, enable, hardened): err", LaunchPolicy{ - AllowedMemoryMonitoring: debugOnly, + DebugImageMonitoring: MemoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -181,7 +190,7 @@ func TestVerify(t *testing.T) { { "memory monitor (debugonly, enable, debug): noerr", LaunchPolicy{ - AllowedMemoryMonitoring: debugOnly, + DebugImageMonitoring: MemoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -193,7 +202,7 @@ func TestVerify(t *testing.T) { { "memory monitor (debugonly, disable, hardened): noerr", LaunchPolicy{ - AllowedMemoryMonitoring: debugOnly, + DebugImageMonitoring: MemoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: false, @@ -205,7 +214,7 @@ func TestVerify(t *testing.T) { { "memory monitor (debugonly, disable, debug): noerr", LaunchPolicy{ - AllowedMemoryMonitoring: debugOnly, + DebugImageMonitoring: MemoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: false, @@ -217,7 +226,8 @@ func TestVerify(t *testing.T) { { "memory monitor (always, enable, hardened): noerr", LaunchPolicy{ - AllowedMemoryMonitoring: always, + HardenedImageMonitoring: MemoryOnly, + DebugImageMonitoring: MemoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -229,7 +239,8 @@ func TestVerify(t *testing.T) { { "memory monitor (always, enable, debug): noerr", LaunchPolicy{ - AllowedMemoryMonitoring: always, + HardenedImageMonitoring: MemoryOnly, + DebugImageMonitoring: MemoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -241,7 +252,8 @@ func TestVerify(t *testing.T) { { "memory monitor (always, disable, hardened): noerr", LaunchPolicy{ - AllowedMemoryMonitoring: always, + HardenedImageMonitoring: MemoryOnly, + DebugImageMonitoring: MemoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: false, @@ -253,7 +265,8 @@ func TestVerify(t *testing.T) { { "memory monitor (always, disable, debug): noerr", LaunchPolicy{ - AllowedMemoryMonitoring: always, + HardenedImageMonitoring: MemoryOnly, + DebugImageMonitoring: MemoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: false, From adac472d04fea555cf2aee3f326ca006412fbc94 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Fri, 23 Aug 2024 21:53:35 +0000 Subject: [PATCH 04/27] fix lint errors --- .../nodeproblemdetector/systemstats_config.go | 2 + launcher/spec/launch_policy.go | 22 ++++----- launcher/spec/launch_policy_test.go | 48 +++++++++---------- 3 files changed, 37 insertions(+), 35 deletions(-) diff --git a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go index 29fabf883..5a9aa3f77 100644 --- a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go +++ b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go @@ -59,6 +59,7 @@ var healthConfig = &SystemStatsConfig{ InvokeInterval: defaultInvokeIntervalString, } +// EnableHealthMonitoringConfig overwrites system stats config with health monitoring config. func EnableHealthMonitoringConfig() error { return healthConfig.WriteFile(systemStatsFilePath) } @@ -82,6 +83,7 @@ func (ssc *SystemStatsConfig) WriteFile(path string) error { return os.WriteFile(path, bytes, 0644) } +// StartService starts Node Problem Detector. func StartService(logger *log.Logger) error { s, err := systemctl.New() if err != nil { diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index 0cf5238a3..55cdd742a 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -30,9 +30,9 @@ const ( type monitoringType int const ( - None monitoringType = iota - MemoryOnly - Health + none monitoringType = iota + memoryOnly + health ) // String returns LaunchPolicy details. @@ -82,14 +82,14 @@ const ( func toMonitoringType(label string) (monitoringType, error) { switch strings.ToUpper(label) { case "NONE": - return None, nil + return none, nil case "MEMORY_ONLY": - return MemoryOnly, nil + return memoryOnly, nil case "HEALTH": - return Health, nil + return health, nil } - return None, fmt.Errorf("Invalid monitoring type: %v", label) + return none, fmt.Errorf("invalid monitoring type: %v", label) } // GetLaunchPolicy takes in a map[string] string which should come from image labels, @@ -131,7 +131,7 @@ func GetLaunchPolicy(imageLabels map[string]string) (LaunchPolicy, error) { return LaunchPolicy{}, fmt.Errorf("invalid monitoring type for hardened image: %v", err) } } else { - launchPolicy.HardenedImageMonitoring = None + launchPolicy.HardenedImageMonitoring = none } if v, ok := imageLabels[debugMonitoring]; ok { @@ -140,7 +140,7 @@ func GetLaunchPolicy(imageLabels map[string]string) (LaunchPolicy, error) { return LaunchPolicy{}, fmt.Errorf("invalid monitoring type for debug image: %v", err) } } else { - launchPolicy.DebugImageMonitoring = Health + launchPolicy.DebugImageMonitoring = health } if v, ok := imageLabels[mountDestinations]; ok { @@ -185,13 +185,13 @@ func (p LaunchPolicy) Verify(ls LaunchSpec) error { if ls.HealthMonitoringEnabled { // Return error if policy does not allow health monitoring. - if monitoringPolicy != Health { + if monitoringPolicy != health { return fmt.Errorf("image does not allow health monitoring") } } if ls.MemoryMonitoringEnabled { - if monitoringPolicy == None { + if monitoringPolicy == none { return fmt.Errorf("image does not allow any monitoring") } } diff --git a/launcher/spec/launch_policy_test.go b/launcher/spec/launch_policy_test.go index d568aec36..99beb282d 100644 --- a/launcher/spec/launch_policy_test.go +++ b/launcher/spec/launch_policy_test.go @@ -58,8 +58,8 @@ func TestLaunchPolicy(t *testing.T) { for _, testcase := range testCases { t.Run(testcase.testName, func(t *testing.T) { // Add default values for policy fields. Not relevant to tested behavior. - testcase.expectedPolicy.HardenedImageMonitoring = None - testcase.expectedPolicy.DebugImageMonitoring = Health + testcase.expectedPolicy.HardenedImageMonitoring = none + testcase.expectedPolicy.DebugImageMonitoring = health got, err := GetLaunchPolicy(testcase.imageLabels) if err != nil { @@ -86,8 +86,8 @@ func TestVerify(t *testing.T) { AllowedEnvOverride: []string{"foo"}, AllowedCmdOverride: true, AllowedLogRedirect: always, - HardenedImageMonitoring: MemoryOnly, - DebugImageMonitoring: MemoryOnly, + HardenedImageMonitoring: memoryOnly, + DebugImageMonitoring: memoryOnly, }, LaunchSpec{ Envs: []EnvVar{{Name: "foo", Value: "foo"}}, @@ -126,8 +126,8 @@ func TestVerify(t *testing.T) { { "memory monitor (never, enable, hardened): err", LaunchPolicy{ - HardenedImageMonitoring: None, - DebugImageMonitoring: None, + HardenedImageMonitoring: none, + DebugImageMonitoring: none, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -139,8 +139,8 @@ func TestVerify(t *testing.T) { { "memory monitor (never, enable, debug): err", LaunchPolicy{ - HardenedImageMonitoring: None, - DebugImageMonitoring: None, + HardenedImageMonitoring: none, + DebugImageMonitoring: none, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -152,8 +152,8 @@ func TestVerify(t *testing.T) { { "memory monitor (never, disable, hardened): noerr", LaunchPolicy{ - HardenedImageMonitoring: None, - DebugImageMonitoring: None, + HardenedImageMonitoring: none, + DebugImageMonitoring: none, }, LaunchSpec{ MemoryMonitoringEnabled: false, @@ -165,8 +165,8 @@ func TestVerify(t *testing.T) { { "memory monitor (never, disable, debug): noerr", LaunchPolicy{ - HardenedImageMonitoring: None, - DebugImageMonitoring: None, + HardenedImageMonitoring: none, + DebugImageMonitoring: none, }, LaunchSpec{ MemoryMonitoringEnabled: false, @@ -178,7 +178,7 @@ func TestVerify(t *testing.T) { { "memory monitor (debugonly, enable, hardened): err", LaunchPolicy{ - DebugImageMonitoring: MemoryOnly, + DebugImageMonitoring: memoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -190,7 +190,7 @@ func TestVerify(t *testing.T) { { "memory monitor (debugonly, enable, debug): noerr", LaunchPolicy{ - DebugImageMonitoring: MemoryOnly, + DebugImageMonitoring: memoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -202,7 +202,7 @@ func TestVerify(t *testing.T) { { "memory monitor (debugonly, disable, hardened): noerr", LaunchPolicy{ - DebugImageMonitoring: MemoryOnly, + DebugImageMonitoring: memoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: false, @@ -214,7 +214,7 @@ func TestVerify(t *testing.T) { { "memory monitor (debugonly, disable, debug): noerr", LaunchPolicy{ - DebugImageMonitoring: MemoryOnly, + DebugImageMonitoring: memoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: false, @@ -226,8 +226,8 @@ func TestVerify(t *testing.T) { { "memory monitor (always, enable, hardened): noerr", LaunchPolicy{ - HardenedImageMonitoring: MemoryOnly, - DebugImageMonitoring: MemoryOnly, + HardenedImageMonitoring: memoryOnly, + DebugImageMonitoring: memoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -239,8 +239,8 @@ func TestVerify(t *testing.T) { { "memory monitor (always, enable, debug): noerr", LaunchPolicy{ - HardenedImageMonitoring: MemoryOnly, - DebugImageMonitoring: MemoryOnly, + HardenedImageMonitoring: memoryOnly, + DebugImageMonitoring: memoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: true, @@ -252,8 +252,8 @@ func TestVerify(t *testing.T) { { "memory monitor (always, disable, hardened): noerr", LaunchPolicy{ - HardenedImageMonitoring: MemoryOnly, - DebugImageMonitoring: MemoryOnly, + HardenedImageMonitoring: memoryOnly, + DebugImageMonitoring: memoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: false, @@ -265,8 +265,8 @@ func TestVerify(t *testing.T) { { "memory monitor (always, disable, debug): noerr", LaunchPolicy{ - HardenedImageMonitoring: MemoryOnly, - DebugImageMonitoring: MemoryOnly, + HardenedImageMonitoring: memoryOnly, + DebugImageMonitoring: memoryOnly, }, LaunchSpec{ MemoryMonitoringEnabled: false, From f4e2c45bbb6ff7bb2c77cdb947e996e0417427b8 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Fri, 23 Aug 2024 21:58:56 +0000 Subject: [PATCH 05/27] more lint fixes --- .../nodeproblemdetector/systemstats_config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go index 5a9aa3f77..58426603f 100644 --- a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go +++ b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go @@ -28,7 +28,7 @@ type statsConfig struct { // For now we only consider collecting memory related metrics. // View the comprehensive configuration details on https://github.com/kubernetes/node-problem-detector/tree/master/pkg/systemstatsmonitor#detailed-configuration-options type SystemStatsConfig struct { - Cpu *statsConfig `json:"cpu,omitempty"` + CPU *statsConfig `json:"cpu,omitempty"` Disk *statsConfig `json:"disk,omitempty"` Host *statsConfig `json:"host,omitempty"` Memory *statsConfig `json:"memory,omitempty"` @@ -44,7 +44,7 @@ func NewSystemStatsConfig() SystemStatsConfig { } var healthConfig = &SystemStatsConfig{ - Cpu: &statsConfig{map[string]metricConfig{ + CPU: &statsConfig{map[string]metricConfig{ "cpu/load_5m": {"cpu/load_5m"}, }}, Disk: &statsConfig{map[string]metricConfig{ From 949dbd1a03b486c9ed69a730d6aea61dab151f5f Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Mon, 26 Aug 2024 22:50:09 +0000 Subject: [PATCH 06/27] add policy and spec tests --- launcher/spec/launch_policy_test.go | 309 ++++++++++++++-------------- launcher/spec/launch_spec.go | 4 +- launcher/spec/launch_spec_test.go | 4 + 3 files changed, 163 insertions(+), 154 deletions(-) diff --git a/launcher/spec/launch_policy_test.go b/launcher/spec/launch_policy_test.go index 99beb282d..ac7ab0bf3 100644 --- a/launcher/spec/launch_policy_test.go +++ b/launcher/spec/launch_policy_test.go @@ -123,158 +123,6 @@ func TestVerify(t *testing.T) { }, true, }, - { - "memory monitor (never, enable, hardened): err", - LaunchPolicy{ - HardenedImageMonitoring: none, - DebugImageMonitoring: none, - }, - LaunchSpec{ - MemoryMonitoringEnabled: true, - Hardened: true, - LogRedirect: Nowhere, - }, - true, - }, - { - "memory monitor (never, enable, debug): err", - LaunchPolicy{ - HardenedImageMonitoring: none, - DebugImageMonitoring: none, - }, - LaunchSpec{ - MemoryMonitoringEnabled: true, - Hardened: false, - LogRedirect: Nowhere, - }, - true, - }, - { - "memory monitor (never, disable, hardened): noerr", - LaunchPolicy{ - HardenedImageMonitoring: none, - DebugImageMonitoring: none, - }, - LaunchSpec{ - MemoryMonitoringEnabled: false, - Hardened: true, - LogRedirect: Nowhere, - }, - false, - }, - { - "memory monitor (never, disable, debug): noerr", - LaunchPolicy{ - HardenedImageMonitoring: none, - DebugImageMonitoring: none, - }, - LaunchSpec{ - MemoryMonitoringEnabled: false, - Hardened: false, - LogRedirect: Nowhere, - }, - false, - }, - { - "memory monitor (debugonly, enable, hardened): err", - LaunchPolicy{ - DebugImageMonitoring: memoryOnly, - }, - LaunchSpec{ - MemoryMonitoringEnabled: true, - Hardened: true, - LogRedirect: Nowhere, - }, - true, - }, - { - "memory monitor (debugonly, enable, debug): noerr", - LaunchPolicy{ - DebugImageMonitoring: memoryOnly, - }, - LaunchSpec{ - MemoryMonitoringEnabled: true, - Hardened: false, - LogRedirect: Nowhere, - }, - false, - }, - { - "memory monitor (debugonly, disable, hardened): noerr", - LaunchPolicy{ - DebugImageMonitoring: memoryOnly, - }, - LaunchSpec{ - MemoryMonitoringEnabled: false, - Hardened: true, - LogRedirect: Nowhere, - }, - false, - }, - { - "memory monitor (debugonly, disable, debug): noerr", - LaunchPolicy{ - DebugImageMonitoring: memoryOnly, - }, - LaunchSpec{ - MemoryMonitoringEnabled: false, - Hardened: false, - LogRedirect: Nowhere, - }, - false, - }, - { - "memory monitor (always, enable, hardened): noerr", - LaunchPolicy{ - HardenedImageMonitoring: memoryOnly, - DebugImageMonitoring: memoryOnly, - }, - LaunchSpec{ - MemoryMonitoringEnabled: true, - Hardened: true, - LogRedirect: Nowhere, - }, - false, - }, - { - "memory monitor (always, enable, debug): noerr", - LaunchPolicy{ - HardenedImageMonitoring: memoryOnly, - DebugImageMonitoring: memoryOnly, - }, - LaunchSpec{ - MemoryMonitoringEnabled: true, - Hardened: false, - LogRedirect: Nowhere, - }, - false, - }, - { - "memory monitor (always, disable, hardened): noerr", - LaunchPolicy{ - HardenedImageMonitoring: memoryOnly, - DebugImageMonitoring: memoryOnly, - }, - LaunchSpec{ - MemoryMonitoringEnabled: false, - Hardened: true, - LogRedirect: Nowhere, - }, - false, - }, - { - "memory monitor (always, disable, debug): noerr", - LaunchPolicy{ - HardenedImageMonitoring: memoryOnly, - DebugImageMonitoring: memoryOnly, - }, - LaunchSpec{ - MemoryMonitoringEnabled: false, - Hardened: false, - LogRedirect: Nowhere, - }, - false, - }, { "log redirect (never, everywhere, hardened): err", LaunchPolicy{ @@ -661,6 +509,163 @@ func TestVerify(t *testing.T) { } } +func TestVerifyMonitoringSettings(t *testing.T) { + testCases := []struct { + testName string + monitoring monitoringType + spec LaunchSpec + }{ + { + "none policy, disabled by spec", + none, + LaunchSpec{ + HealthMonitoringEnabled: false, + MemoryMonitoringEnabled: false, + LogRedirect: Nowhere, + }, + }, + { + "memory-only policy, all disabled by spec", + memoryOnly, + LaunchSpec{ + HealthMonitoringEnabled: false, + MemoryMonitoringEnabled: false, + LogRedirect: Nowhere, + }, + }, + { + "memory-only policy, memory enabled by spec", + memoryOnly, + LaunchSpec{ + MemoryMonitoringEnabled: true, + LogRedirect: Nowhere, + }, + }, + { + "health policy, health enabled by spec", + health, + LaunchSpec{ + HealthMonitoringEnabled: true, + LogRedirect: Nowhere, + }, + }, + { + "health policy, health disabled by spec", + health, + LaunchSpec{ + HealthMonitoringEnabled: false, + LogRedirect: Nowhere, + }, + }, + { + "health policy, memory enabled by spec", + health, + LaunchSpec{ + MemoryMonitoringEnabled: true, + LogRedirect: Nowhere, + }, + }, + { + "health policy, memory disabled by spec", + health, + LaunchSpec{ + MemoryMonitoringEnabled: false, + LogRedirect: Nowhere, + }, + }, + } + + for _, testCase := range testCases { + // Debug. + t.Run("[Debug] "+testCase.testName, func(t *testing.T) { + policy := LaunchPolicy{ + DebugImageMonitoring: testCase.monitoring, + } + if err := policy.Verify(testCase.spec); err != nil { + t.Errorf("expected no error, but got %v", err) + } + }) + + // Hardened. + t.Run("[Hardened] "+testCase.testName, func(t *testing.T) { + policy := LaunchPolicy{ + HardenedImageMonitoring: testCase.monitoring, + } + + // Copy the spec and set Hardened=true. + spec := testCase.spec + spec.Hardened = true + if err := policy.Verify(spec); err != nil { + t.Errorf("expected no error, but got %v", err) + } + }) + } +} + +func TestVerifyMonitoringSettingsErrors(t *testing.T) { + testCases := []struct { + testName string + monitoring monitoringType + spec LaunchSpec + }{ + { + "[Hardened] disabled policy, Health enabled by spec", + none, + LaunchSpec{ + HealthMonitoringEnabled: true, + Hardened: true, + LogRedirect: Nowhere, + }, + }, + { + "[Hardened] disabled policy, Memory enabled by spec", + none, + LaunchSpec{ + MemoryMonitoringEnabled: true, + Hardened: true, + LogRedirect: Nowhere, + }, + }, + { + "[Hardened] memory-only policy, Health enabled by spec", + memoryOnly, + LaunchSpec{ + HealthMonitoringEnabled: true, + Hardened: true, + LogRedirect: Nowhere, + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.testName, func(t *testing.T) { + // Debug. + t.Run("[Debug] "+testCase.testName, func(t *testing.T) { + policy := LaunchPolicy{ + DebugImageMonitoring: testCase.monitoring, + } + if err := policy.Verify(testCase.spec); err == nil { + t.Errorf("expected error, but got nil") + } + }) + + // Hardened. + t.Run("[Hardened] "+testCase.testName, func(t *testing.T) { + policy := LaunchPolicy{ + HardenedImageMonitoring: testCase.monitoring, + } + + // Copy the spec and set Hardened=true. + spec := testCase.spec + spec.Hardened = true + if err := policy.Verify(spec); err == nil { + t.Errorf("expected error, but got nil") + } + }) + }) + } +} + func TestIsHardened(t *testing.T) { testCases := []struct { testName string diff --git a/launcher/spec/launch_spec.go b/launcher/spec/launch_spec.go index 2ee016b40..4a6511714 100644 --- a/launcher/spec/launch_spec.go +++ b/launcher/spec/launch_spec.go @@ -83,7 +83,7 @@ const ( attestationServiceAddrKey = "tee-attestation-service-endpoint" logRedirectKey = "tee-container-log-redirect" memoryMonitoringEnable = "tee-monitoring-memory-enable" - healthMonitoringEnable = "tee-health-monitoring-enable" + healthMonitoringEnable = "tee-monitoring-health-enable" devShmSizeKey = "tee-dev-shm-size-kb" mountKey = "tee-mount" ) @@ -170,7 +170,7 @@ func (s *LaunchSpec) UnmarshalJSON(b []byte) error { if boolValue, err := strconv.ParseBool(val); err == nil { // If Health Monitoring is enabled but Memory Monitoring is disabled, this is contradictory. if boolValue && memoryProhibited { - return fmt.Errorf("Health monitoring is enabled but memory monitoring is disabled.") + return fmt.Errorf("health monitoring is enabled but memory monitoring is disabled.") } s.HealthMonitoringEnabled = boolValue diff --git a/launcher/spec/launch_spec_test.go b/launcher/spec/launch_spec_test.go index 463de3ef6..0a1ae0091 100644 --- a/launcher/spec/launch_spec_test.go +++ b/launcher/spec/launch_spec_test.go @@ -25,6 +25,7 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { "tee-impersonate-service-accounts":"sv1@developer.gserviceaccount.com,sv2@developer.gserviceaccount.com", "tee-container-log-redirect":"true", "tee-monitoring-memory-enable":"true", + "tee-health-monitoring-enable":"false", "tee-dev-shm-size-kb":"234234", "tee-mount":"type=tmpfs,source=tmpfs,destination=/tmpmount;type=tmpfs,source=tmpfs,destination=/sized,size=222" }`, @@ -42,6 +43,7 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { "tee-impersonate-service-accounts":"sv1@developer.gserviceaccount.com,sv2@developer.gserviceaccount.com", "tee-container-log-redirect":"true", "tee-monitoring-memory-enable":"TRUE", + "tee-monitoring-health-enable":"FALSE", "tee-dev-shm-size-kb":"234234", "tee-mount":"type=tmpfs,source=tmpfs,destination=/tmpmount;type=tmpfs,source=tmpfs,destination=/sized,size=222" }`, @@ -57,6 +59,7 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, LogRedirect: Everywhere, MemoryMonitoringEnabled: true, + HealthMonitoringEnabled: false, DevShmSize: 234234, Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, @@ -140,6 +143,7 @@ func TestLaunchSpecUnmarshalJSONWithDefaultValue(t *testing.T) { "tee-container-log-redirect":"", "tee-restart-policy":"", "tee-monitoring-memory-enable":"", + "tee-monitoring-health-enable":"", "tee-mount":"" }` From a6551e0727b27eb4dedc2e7ffe381da61901dca3 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Mon, 26 Aug 2024 22:54:43 +0000 Subject: [PATCH 07/27] fix lint error --- launcher/spec/launch_spec.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/launcher/spec/launch_spec.go b/launcher/spec/launch_spec.go index 4a6511714..d500353cf 100644 --- a/launcher/spec/launch_spec.go +++ b/launcher/spec/launch_spec.go @@ -170,7 +170,7 @@ func (s *LaunchSpec) UnmarshalJSON(b []byte) error { if boolValue, err := strconv.ParseBool(val); err == nil { // If Health Monitoring is enabled but Memory Monitoring is disabled, this is contradictory. if boolValue && memoryProhibited { - return fmt.Errorf("health monitoring is enabled but memory monitoring is disabled.") + return fmt.Errorf("health monitoring is enabled but memory monitoring is disabled") } s.HealthMonitoringEnabled = boolValue From 735962784c5a43e84c544afd672fd4ea39ab65b9 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Tue, 27 Aug 2024 18:04:42 +0000 Subject: [PATCH 08/27] Update cloudbuild test for memory monitoring --- launcher/image/testworkloads/memorymonitoring/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/launcher/image/testworkloads/memorymonitoring/Dockerfile b/launcher/image/testworkloads/memorymonitoring/Dockerfile index 7f8fca0ed..8c2cd0850 100644 --- a/launcher/image/testworkloads/memorymonitoring/Dockerfile +++ b/launcher/image/testworkloads/memorymonitoring/Dockerfile @@ -7,7 +7,7 @@ COPY main / ENV env_bar="val_bar" -LABEL "tee.launch_policy.monitoring_memory_allow"="always" +LABEL "tee.launch_policy.hardened_monitoring"="MEMORY_ONLY" ENTRYPOINT ["/main"] From 217a7100b4ad502a7bd30e7b91fc9d204a430dd2 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Tue, 27 Aug 2024 23:08:18 +0000 Subject: [PATCH 09/27] update policy for test workloads --- launcher/image/testworkloads/memorymonitoring/Dockerfile | 1 + launcher/image/testworkloads/memorymonitoringdebug/Dockerfile | 3 ++- launcher/image/testworkloads/memorymonitoringnever/Dockerfile | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/launcher/image/testworkloads/memorymonitoring/Dockerfile b/launcher/image/testworkloads/memorymonitoring/Dockerfile index 8c2cd0850..2b245ea7e 100644 --- a/launcher/image/testworkloads/memorymonitoring/Dockerfile +++ b/launcher/image/testworkloads/memorymonitoring/Dockerfile @@ -8,6 +8,7 @@ COPY main / ENV env_bar="val_bar" LABEL "tee.launch_policy.hardened_monitoring"="MEMORY_ONLY" +LABEL "tee.launch_policy.debug_monitoring"="NONE" ENTRYPOINT ["/main"] diff --git a/launcher/image/testworkloads/memorymonitoringdebug/Dockerfile b/launcher/image/testworkloads/memorymonitoringdebug/Dockerfile index 8df0840f5..51da2fad4 100644 --- a/launcher/image/testworkloads/memorymonitoringdebug/Dockerfile +++ b/launcher/image/testworkloads/memorymonitoringdebug/Dockerfile @@ -7,7 +7,8 @@ COPY main / ENV env_bar="val_bar" -LABEL "tee.launch_policy.monitoring_memory_allow"="debugonly" +LABEL "tee.launch_policy.hardened_monitoring"="NONE" +LABEL "tee.launch_policy.debug_monitoring"="MEMORY_ONLY" ENTRYPOINT ["/main"] diff --git a/launcher/image/testworkloads/memorymonitoringnever/Dockerfile b/launcher/image/testworkloads/memorymonitoringnever/Dockerfile index 22f7a84dd..d7df4d8d1 100644 --- a/launcher/image/testworkloads/memorymonitoringnever/Dockerfile +++ b/launcher/image/testworkloads/memorymonitoringnever/Dockerfile @@ -7,8 +7,8 @@ COPY main / ENV env_bar="val_bar" -LABEL "tee.launch_policy.monitoring_memory_allow"="never" - +LABEL "tee.launch_policy.hardened_monitoring"="NONE" +LABEL "tee.launch_policy.debug_monitoring"="NONE" ENTRYPOINT ["/main"] CMD ["arg_foo"] From cbd2e200b9f18170efac4baaf03bdd58e7e6a677 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Thu, 29 Aug 2024 23:42:45 +0000 Subject: [PATCH 10/27] add backwards compatibility for memory_monitoring_allow policy: --- launcher/container_runner.go | 2 +- launcher/spec/launch_policy.go | 69 ++++++++++- launcher/spec/launch_policy_test.go | 185 +++++++++++++++++++++++++++- 3 files changed, 249 insertions(+), 7 deletions(-) diff --git a/launcher/container_runner.go b/launcher/container_runner.go index fb95ae134..b130be37f 100644 --- a/launcher/container_runner.go +++ b/launcher/container_runner.go @@ -115,7 +115,7 @@ func NewRunner(ctx context.Context, cdClient *containerd.Client, token oauth2.To } logger.Printf("Image Labels : %v\n", imageConfig.Labels) - launchPolicy, err := spec.GetLaunchPolicy(imageConfig.Labels) + launchPolicy, err := spec.GetLaunchPolicy(imageConfig.Labels, logger) if err != nil { return nil, err } diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index 55cdd742a..dc2f3859b 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -3,6 +3,7 @@ package spec import ( "errors" "fmt" + "log" "path/filepath" "strconv" "strings" @@ -79,13 +80,71 @@ const ( mountDestinations = "tee.launch_policy.allow_mount_destinations" ) +func getMonitoringPolicy(imageLabels map[string]string, launchPolicy *LaunchPolicy, logger *log.Logger) error { + // Old policy. + memVal, memOk := imageLabels[memoryMonitoring] + // New policies. + hardenedVal, hardenedOk := imageLabels[hardenedMonitoring] + debugVal, debugOk := imageLabels[debugMonitoring] + + var err error + + // Return an error if old/new policies are both defined + if memOk && (hardenedOk || debugOk) { + return fmt.Errorf("use either %s or %s/%s in image labels,- not both", memoryMonitoring, hardenedMonitoring, debugMonitoring) + } else if memOk { + policy, err := toPolicy(memoryMonitoring, memVal) + if err != nil { + return fmt.Errorf("invalid image LABEL '%s'", memoryMonitoring) + } + + logger.Printf("%s is deprecated, use %s and %s instead", memoryMonitoring, hardenedMonitoring, debugMonitoring) + + switch policy { + case always: + logger.Printf("%s=always, will be treated as %s=memory_only and %s=memory_only", memoryMonitoring, hardenedMonitoring, debugMonitoring) + launchPolicy.HardenedImageMonitoring = memoryOnly + launchPolicy.DebugImageMonitoring = memoryOnly + case never: + logger.Printf("%s=never, will be treated as %s=none and %s=none", memoryMonitoring, hardenedMonitoring, debugMonitoring) + launchPolicy.HardenedImageMonitoring = none + launchPolicy.DebugImageMonitoring = none + case debugOnly: + logger.Printf("%s=debug_only, will be treated as %s=none and %s=memory_only", memoryMonitoring, hardenedMonitoring, debugMonitoring) + launchPolicy.HardenedImageMonitoring = none + launchPolicy.DebugImageMonitoring = memoryOnly + } + return nil + } else { + if hardenedOk { + launchPolicy.HardenedImageMonitoring, err = toMonitoringType(hardenedVal) + if err != nil { + return fmt.Errorf("invalid monitoring type for hardened image: %v", err) + } + } else { + launchPolicy.HardenedImageMonitoring = none + } + + if debugOk { + launchPolicy.DebugImageMonitoring, err = toMonitoringType(debugVal) + if err != nil { + return fmt.Errorf("invalid monitoring type for debug image: %v", err) + } + } else { + launchPolicy.DebugImageMonitoring = health + } + } + + return nil +} + func toMonitoringType(label string) (monitoringType, error) { - switch strings.ToUpper(label) { - case "NONE": + switch strings.ToLower(label) { + case "none": return none, nil - case "MEMORY_ONLY": + case "memoryonly": return memoryOnly, nil - case "HEALTH": + case "health": return health, nil } @@ -94,7 +153,7 @@ func toMonitoringType(label string) (monitoringType, error) { // GetLaunchPolicy takes in a map[string] string which should come from image labels, // and will try to parse it into a LaunchPolicy. Extra fields will be ignored. -func GetLaunchPolicy(imageLabels map[string]string) (LaunchPolicy, error) { +func GetLaunchPolicy(imageLabels map[string]string, logger *log.Logger) (LaunchPolicy, error) { var err error launchPolicy := LaunchPolicy{} if v, ok := imageLabels[envOverride]; ok { diff --git a/launcher/spec/launch_policy_test.go b/launcher/spec/launch_policy_test.go index ac7ab0bf3..0543869b8 100644 --- a/launcher/spec/launch_policy_test.go +++ b/launcher/spec/launch_policy_test.go @@ -1,6 +1,7 @@ package spec import ( + "log" "testing" "github.com/google/go-cmp/cmp" @@ -61,7 +62,7 @@ func TestLaunchPolicy(t *testing.T) { testcase.expectedPolicy.HardenedImageMonitoring = none testcase.expectedPolicy.DebugImageMonitoring = health - got, err := GetLaunchPolicy(testcase.imageLabels) + got, err := GetLaunchPolicy(testcase.imageLabels, log.Default()) if err != nil { t.Fatal(err) } @@ -703,3 +704,185 @@ func TestIsHardened(t *testing.T) { }) } } + +func TestGetMonitoringPolicy(t *testing.T) { + testcases := []struct { + name string + labels map[string]string + expectedPolicy *LaunchPolicy + }{ + { + name: "memory_monitoring_allow=always", + labels: map[string]string{ + memoryMonitoring: "always", + }, + expectedPolicy: &LaunchPolicy{ + HardenedImageMonitoring: memoryOnly, + DebugImageMonitoring: memoryOnly, + }, + }, + { + name: "memory_monitoring_allow=never", + labels: map[string]string{ + memoryMonitoring: "never", + }, + expectedPolicy: &LaunchPolicy{ + HardenedImageMonitoring: none, + DebugImageMonitoring: none, + }, + }, + { + name: "memory_monitoring_allow=debugonly", + labels: map[string]string{ + memoryMonitoring: "debugonly", + }, + expectedPolicy: &LaunchPolicy{ + HardenedImageMonitoring: none, + DebugImageMonitoring: memoryOnly, + }, + }, + { + name: "HardenedImageMonitoring=none", + labels: map[string]string{ + hardenedMonitoring: "none", + }, + expectedPolicy: &LaunchPolicy{ + HardenedImageMonitoring: none, + DebugImageMonitoring: health, + }, + }, + { + name: "HardenedImageMonitoring=memoryonly", + labels: map[string]string{ + hardenedMonitoring: "memoryonly", + }, + expectedPolicy: &LaunchPolicy{ + HardenedImageMonitoring: memoryOnly, + DebugImageMonitoring: health, + }, + }, + { + name: "HardenedImageMonitoring=health", + labels: map[string]string{ + hardenedMonitoring: "health", + }, + expectedPolicy: &LaunchPolicy{ + HardenedImageMonitoring: health, + DebugImageMonitoring: health, + }, + }, + { + name: "DebugImageMonitoring=none", + labels: map[string]string{ + debugMonitoring: "none", + }, + expectedPolicy: &LaunchPolicy{ + HardenedImageMonitoring: none, + DebugImageMonitoring: none, + }, + }, + { + name: "DebugImageMonitoring=memoryonly", + labels: map[string]string{ + debugMonitoring: "memoryonly", + }, + expectedPolicy: &LaunchPolicy{ + HardenedImageMonitoring: none, + DebugImageMonitoring: memoryOnly, + }, + }, + { + name: "DebugImageMonitoring=health", + labels: map[string]string{ + debugMonitoring: "health", + }, + expectedPolicy: &LaunchPolicy{ + HardenedImageMonitoring: none, + DebugImageMonitoring: health, + }, + }, + // Set both fields to non-default values. + { + name: "HardenedImageMonitoring=health, DebugImageMonitoring=none", + labels: map[string]string{ + hardenedMonitoring: "health", + debugMonitoring: "none", + }, + expectedPolicy: &LaunchPolicy{ + HardenedImageMonitoring: health, + DebugImageMonitoring: none, + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + policy := &LaunchPolicy{} + if err := getMonitoringPolicy(tc.labels, policy, log.Default()); err != nil { + t.Errorf("getMonitoringPolicy returned error: %v", err) + return + } + + if !cmp.Equal(policy, tc.expectedPolicy) { + t.Errorf("getMonitoringPolicy did not return expected policy: got %v, want %v", policy, tc.expectedPolicy) + } + }) + } +} + +func TestGetMonitoringPolicyErrors(t *testing.T) { + testcases := []struct { + name string + labels map[string]string + }{ + { + name: "memory_monitoring_allow and hardened_monitoring specified", + labels: map[string]string{ + memoryMonitoring: "always", + hardenedMonitoring: "health", + }, + }, + { + name: "memory_monitoring_allow and debug_monitoring specified", + labels: map[string]string{ + memoryMonitoring: "always", + debugMonitoring: "health", + }, + }, + { + name: "memory_monitoring_allow, hardened_monitoring, and debug_monitoring specified", + labels: map[string]string{ + memoryMonitoring: "always", + hardenedMonitoring: "health", + debugMonitoring: "memoryOnly", + }, + }, + { + name: "invalid value for memory_monitoring_allow", + labels: map[string]string{ + memoryMonitoring: "this is not valid", + }, + }, + { + name: "invalid value for hardened_monitoring", + labels: map[string]string{ + hardenedMonitoring: "this is not valid", + }, + }, + { + name: "invalid value for debug_monitoring", + labels: map[string]string{ + debugMonitoring: "this is not valid", + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + policy := &LaunchPolicy{} + if err := getMonitoringPolicy(tc.labels, policy, log.Default()); err == nil { + t.Errorf("Expected getMonitoringPolicy to return error, returned successfully with policy %v", policy) + } + }) + } +} From 53973a4cf1fdcb8d0f3e8eb503a20809a90ef147 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Tue, 3 Sep 2024 21:07:11 +0000 Subject: [PATCH 11/27] only one field can be specified in spec --- launcher/spec/launch_spec.go | 24 ++++------ launcher/spec/launch_spec_test.go | 78 +++++++++++++++++++++++++++---- 2 files changed, 79 insertions(+), 23 deletions(-) diff --git a/launcher/spec/launch_spec.go b/launcher/spec/launch_spec.go index d500353cf..6d3551219 100644 --- a/launcher/spec/launch_spec.go +++ b/launcher/spec/launch_spec.go @@ -156,23 +156,17 @@ func (s *LaunchSpec) UnmarshalJSON(b []byte) error { s.SignedImageRepos = append(s.SignedImageRepos, imageRepos...) } - memoryProhibited := false - if val, ok := unmarshaledMap[memoryMonitoringEnable]; ok && val != "" { - if boolValue, err := strconv.ParseBool(val); err == nil { + memVal, memOk := unmarshaledMap[memoryMonitoringEnable] + healthVal, healthOk := unmarshaledMap[healthMonitoringEnable] + + if memOk && healthOk { + return fmt.Errorf("both %v and %v are specified, only one is permitted", memoryMonitoringEnable, healthMonitoringEnable) + } else if memOk && memVal != "" { + if boolValue, err := strconv.ParseBool(memVal); err == nil { s.MemoryMonitoringEnabled = boolValue - if !boolValue { - memoryProhibited = true - } } - } - - if val, ok := unmarshaledMap[healthMonitoringEnable]; ok && val != "" { - if boolValue, err := strconv.ParseBool(val); err == nil { - // If Health Monitoring is enabled but Memory Monitoring is disabled, this is contradictory. - if boolValue && memoryProhibited { - return fmt.Errorf("health monitoring is enabled but memory monitoring is disabled") - } - + } else if healthOk && healthVal != "" { + if boolValue, err := strconv.ParseBool(healthVal); err == nil { s.HealthMonitoringEnabled = boolValue } } diff --git a/launcher/spec/launch_spec_test.go b/launcher/spec/launch_spec_test.go index 0a1ae0091..a753bcfa5 100644 --- a/launcher/spec/launch_spec_test.go +++ b/launcher/spec/launch_spec_test.go @@ -11,11 +11,12 @@ import ( func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { var testCases = []struct { - testName string - mdsJSON string + testName string + mdsJSON string + expectedSpec *LaunchSpec }{ { - "HappyCase", + "HappyCase_MemMonitor", `{ "tee-cmd":"[\"--foo\",\"--bar\",\"--baz\"]", "tee-env-foo":"bar", @@ -25,10 +26,52 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { "tee-impersonate-service-accounts":"sv1@developer.gserviceaccount.com,sv2@developer.gserviceaccount.com", "tee-container-log-redirect":"true", "tee-monitoring-memory-enable":"true", - "tee-health-monitoring-enable":"false", "tee-dev-shm-size-kb":"234234", "tee-mount":"type=tmpfs,source=tmpfs,destination=/tmpmount;type=tmpfs,source=tmpfs,destination=/sized,size=222" }`, + &LaunchSpec{ + ImageRef: "docker.io/library/hello-world:latest", + SignedImageRepos: []string{"docker.io/library/hello-world", "gcr.io/cloudrun/hello"}, + RestartPolicy: Always, + Cmd: []string{"--foo", "--bar", "--baz"}, + Envs: []EnvVar{{"foo", "bar"}}, + ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, + LogRedirect: Everywhere, + MemoryMonitoringEnabled: true, + HealthMonitoringEnabled: false, + DevShmSize: 234234, + Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, + launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, + }, + }, + { + "HappyCase_HealthMonitor", + `{ + "tee-cmd":"[\"--foo\",\"--bar\",\"--baz\"]", + "tee-env-foo":"bar", + "tee-image-reference":"docker.io/library/hello-world:latest", + "tee-signed-image-repos":"docker.io/library/hello-world,gcr.io/cloudrun/hello", + "tee-restart-policy":"Always", + "tee-impersonate-service-accounts":"sv1@developer.gserviceaccount.com,sv2@developer.gserviceaccount.com", + "tee-container-log-redirect":"true", + "tee-monitoring-health-enable":"true", + "tee-dev-shm-size-kb":"234234", + "tee-mount":"type=tmpfs,source=tmpfs,destination=/tmpmount;type=tmpfs,source=tmpfs,destination=/sized,size=222" + }`, + &LaunchSpec{ + ImageRef: "docker.io/library/hello-world:latest", + SignedImageRepos: []string{"docker.io/library/hello-world", "gcr.io/cloudrun/hello"}, + RestartPolicy: Always, + Cmd: []string{"--foo", "--bar", "--baz"}, + Envs: []EnvVar{{"foo", "bar"}}, + ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, + LogRedirect: Everywhere, + MemoryMonitoringEnabled: false, + HealthMonitoringEnabled: true, + DevShmSize: 234234, + Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, + launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, + }, }, { "HappyCaseWithExtraUnknownFields", @@ -43,10 +86,23 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { "tee-impersonate-service-accounts":"sv1@developer.gserviceaccount.com,sv2@developer.gserviceaccount.com", "tee-container-log-redirect":"true", "tee-monitoring-memory-enable":"TRUE", - "tee-monitoring-health-enable":"FALSE", "tee-dev-shm-size-kb":"234234", "tee-mount":"type=tmpfs,source=tmpfs,destination=/tmpmount;type=tmpfs,source=tmpfs,destination=/sized,size=222" }`, + &LaunchSpec{ + ImageRef: "docker.io/library/hello-world:latest", + SignedImageRepos: []string{"docker.io/library/hello-world", "gcr.io/cloudrun/hello"}, + RestartPolicy: Always, + Cmd: []string{"--foo", "--bar", "--baz"}, + Envs: []EnvVar{{"foo", "bar"}}, + ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, + LogRedirect: Everywhere, + MemoryMonitoringEnabled: true, + HealthMonitoringEnabled: false, + DevShmSize: 234234, + Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, + launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, + }, }, } @@ -77,8 +133,8 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { if err := spec.UnmarshalJSON([]byte(testcase.mdsJSON)); err != nil { t.Fatal(err) } - if !cmp.Equal(spec, want) { - t.Errorf("LaunchSpec UnmarshalJSON got %+v, want %+v", spec, want) + if !cmp.Equal(spec, testcase.expectedSpec) { + t.Errorf("LaunchSpec UnmarshalJSON got %+v, want %+v", spec, testcase.expectedSpec) } }) } @@ -123,6 +179,13 @@ func TestLaunchSpecUnmarshalJSONBadInput(t *testing.T) { "tee-container-log-redirect":"badideas", }`, }, + { + "Memory and Health Monitoring both specified", + `{ + "tee-monitoring-memory-enable":"false", + "tee-monitoring-health-enable":"false", + }`, + }, } for _, testcase := range testCases { @@ -143,7 +206,6 @@ func TestLaunchSpecUnmarshalJSONWithDefaultValue(t *testing.T) { "tee-container-log-redirect":"", "tee-restart-policy":"", "tee-monitoring-memory-enable":"", - "tee-monitoring-health-enable":"", "tee-mount":"" }` From bc35595f26698c3322888c5b348f098a543e45d7 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Thu, 5 Sep 2024 22:29:53 +0000 Subject: [PATCH 12/27] fix lint errors --- launcher/container_runner.go | 2 +- launcher/spec/launch_policy.go | 30 ++++++++++++++--------------- launcher/spec/launch_policy_test.go | 2 +- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/launcher/container_runner.go b/launcher/container_runner.go index b130be37f..fb95ae134 100644 --- a/launcher/container_runner.go +++ b/launcher/container_runner.go @@ -115,7 +115,7 @@ func NewRunner(ctx context.Context, cdClient *containerd.Client, token oauth2.To } logger.Printf("Image Labels : %v\n", imageConfig.Labels) - launchPolicy, err := spec.GetLaunchPolicy(imageConfig.Labels, logger) + launchPolicy, err := spec.GetLaunchPolicy(imageConfig.Labels) if err != nil { return nil, err } diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index dc2f3859b..7ef73a0fe 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -115,24 +115,24 @@ func getMonitoringPolicy(imageLabels map[string]string, launchPolicy *LaunchPoli launchPolicy.DebugImageMonitoring = memoryOnly } return nil - } else { - if hardenedOk { - launchPolicy.HardenedImageMonitoring, err = toMonitoringType(hardenedVal) - if err != nil { - return fmt.Errorf("invalid monitoring type for hardened image: %v", err) - } - } else { - launchPolicy.HardenedImageMonitoring = none + } + + if hardenedOk { + launchPolicy.HardenedImageMonitoring, err = toMonitoringType(hardenedVal) + if err != nil { + return fmt.Errorf("invalid monitoring type for hardened image: %v", err) } + } else { + launchPolicy.HardenedImageMonitoring = none + } - if debugOk { - launchPolicy.DebugImageMonitoring, err = toMonitoringType(debugVal) - if err != nil { - return fmt.Errorf("invalid monitoring type for debug image: %v", err) - } - } else { - launchPolicy.DebugImageMonitoring = health + if debugOk { + launchPolicy.DebugImageMonitoring, err = toMonitoringType(debugVal) + if err != nil { + return fmt.Errorf("invalid monitoring type for debug image: %v", err) } + } else { + launchPolicy.DebugImageMonitoring = health } return nil diff --git a/launcher/spec/launch_policy_test.go b/launcher/spec/launch_policy_test.go index 0543869b8..ad814c98e 100644 --- a/launcher/spec/launch_policy_test.go +++ b/launcher/spec/launch_policy_test.go @@ -62,7 +62,7 @@ func TestLaunchPolicy(t *testing.T) { testcase.expectedPolicy.HardenedImageMonitoring = none testcase.expectedPolicy.DebugImageMonitoring = health - got, err := GetLaunchPolicy(testcase.imageLabels, log.Default()) + got, err := GetLaunchPolicy(testcase.imageLabels) if err != nil { t.Fatal(err) } From 11da498b8c54ce8a0373719294fa66f26ecb135a Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Thu, 5 Sep 2024 22:35:26 +0000 Subject: [PATCH 13/27] commit launchpolicy changes --- launcher/spec/launch_policy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index 7ef73a0fe..19527f7c6 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -153,7 +153,7 @@ func toMonitoringType(label string) (monitoringType, error) { // GetLaunchPolicy takes in a map[string] string which should come from image labels, // and will try to parse it into a LaunchPolicy. Extra fields will be ignored. -func GetLaunchPolicy(imageLabels map[string]string, logger *log.Logger) (LaunchPolicy, error) { +func GetLaunchPolicy(imageLabels map[string]string) (LaunchPolicy, error) { var err error launchPolicy := LaunchPolicy{} if v, ok := imageLabels[envOverride]; ok { From 537c87ee13452eb6ba33f8db9fb6df8ab3cc4ebc Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Mon, 9 Sep 2024 22:34:03 +0000 Subject: [PATCH 14/27] use configureMonitoringPolicy to process image labels --- launcher/container_runner.go | 2 +- launcher/spec/launch_policy.go | 28 +++++----------------------- launcher/spec/launch_policy_test.go | 6 +++--- 3 files changed, 9 insertions(+), 27 deletions(-) diff --git a/launcher/container_runner.go b/launcher/container_runner.go index fb95ae134..b130be37f 100644 --- a/launcher/container_runner.go +++ b/launcher/container_runner.go @@ -115,7 +115,7 @@ func NewRunner(ctx context.Context, cdClient *containerd.Client, token oauth2.To } logger.Printf("Image Labels : %v\n", imageConfig.Labels) - launchPolicy, err := spec.GetLaunchPolicy(imageConfig.Labels) + launchPolicy, err := spec.GetLaunchPolicy(imageConfig.Labels, logger) if err != nil { return nil, err } diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index 19527f7c6..73f44d358 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -80,7 +80,7 @@ const ( mountDestinations = "tee.launch_policy.allow_mount_destinations" ) -func getMonitoringPolicy(imageLabels map[string]string, launchPolicy *LaunchPolicy, logger *log.Logger) error { +func configureMonitoringPolicy(imageLabels map[string]string, launchPolicy *LaunchPolicy, logger *log.Logger) error { // Old policy. memVal, memOk := imageLabels[memoryMonitoring] // New policies. @@ -98,7 +98,7 @@ func getMonitoringPolicy(imageLabels map[string]string, launchPolicy *LaunchPoli return fmt.Errorf("invalid image LABEL '%s'", memoryMonitoring) } - logger.Printf("%s is deprecated, use %s and %s instead", memoryMonitoring, hardenedMonitoring, debugMonitoring) + logger.Printf("%s will be deprecated, use %s and %s instead", memoryMonitoring, hardenedMonitoring, debugMonitoring) switch policy { case always: @@ -153,7 +153,7 @@ func toMonitoringType(label string) (monitoringType, error) { // GetLaunchPolicy takes in a map[string] string which should come from image labels, // and will try to parse it into a LaunchPolicy. Extra fields will be ignored. -func GetLaunchPolicy(imageLabels map[string]string) (LaunchPolicy, error) { +func GetLaunchPolicy(imageLabels map[string]string, logger *log.Logger) (LaunchPolicy, error) { var err error launchPolicy := LaunchPolicy{} if v, ok := imageLabels[envOverride]; ok { @@ -180,26 +180,8 @@ func GetLaunchPolicy(imageLabels map[string]string) (LaunchPolicy, error) { } } - if _, ok := imageLabels[memoryMonitoring]; ok { - return LaunchPolicy{}, fmt.Errorf("%v label is deprecated - use %v and %v instead", memoryMonitoring, hardenedMonitoring, debugMonitoring) - } - - if v, ok := imageLabels[hardenedMonitoring]; ok { - launchPolicy.HardenedImageMonitoring, err = toMonitoringType(v) - if err != nil { - return LaunchPolicy{}, fmt.Errorf("invalid monitoring type for hardened image: %v", err) - } - } else { - launchPolicy.HardenedImageMonitoring = none - } - - if v, ok := imageLabels[debugMonitoring]; ok { - launchPolicy.DebugImageMonitoring, err = toMonitoringType(v) - if err != nil { - return LaunchPolicy{}, fmt.Errorf("invalid monitoring type for debug image: %v", err) - } - } else { - launchPolicy.DebugImageMonitoring = health + if err := configureMonitoringPolicy(imageLabels, &launchPolicy, logger); err != nil { + return LaunchPolicy{}, err } if v, ok := imageLabels[mountDestinations]; ok { diff --git a/launcher/spec/launch_policy_test.go b/launcher/spec/launch_policy_test.go index ad814c98e..b3611577b 100644 --- a/launcher/spec/launch_policy_test.go +++ b/launcher/spec/launch_policy_test.go @@ -62,7 +62,7 @@ func TestLaunchPolicy(t *testing.T) { testcase.expectedPolicy.HardenedImageMonitoring = none testcase.expectedPolicy.DebugImageMonitoring = health - got, err := GetLaunchPolicy(testcase.imageLabels) + got, err := GetLaunchPolicy(testcase.imageLabels, log.Default()) if err != nil { t.Fatal(err) } @@ -818,7 +818,7 @@ func TestGetMonitoringPolicy(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { policy := &LaunchPolicy{} - if err := getMonitoringPolicy(tc.labels, policy, log.Default()); err != nil { + if err := configureMonitoringPolicy(tc.labels, policy, log.Default()); err != nil { t.Errorf("getMonitoringPolicy returned error: %v", err) return } @@ -880,7 +880,7 @@ func TestGetMonitoringPolicyErrors(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { policy := &LaunchPolicy{} - if err := getMonitoringPolicy(tc.labels, policy, log.Default()); err == nil { + if err := configureMonitoringPolicy(tc.labels, policy, log.Default()); err == nil { t.Errorf("Expected getMonitoringPolicy to return error, returned successfully with policy %v", policy) } }) From 57a443471f422a437eaec7fe2665d99cfa05faab Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Mon, 9 Sep 2024 23:44:43 +0000 Subject: [PATCH 15/27] use original error messages for consistency/testing --- launcher/spec/launch_policy.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index 73f44d358..efa486a29 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -224,17 +224,20 @@ func (p LaunchPolicy) Verify(ls LaunchSpec) error { monitoringPolicy = p.HardenedImageMonitoring } - if ls.HealthMonitoringEnabled { - // Return error if policy does not allow health monitoring. - if monitoringPolicy != health { - return fmt.Errorf("image does not allow health monitoring") + if ls.HealthMonitoringEnabled && monitoringPolicy != health { + // Check if there is an issue with the image type. + if ls.Hardened && p.DebugImageMonitoring == health { + return fmt.Errorf("health monitoring only allowed on debug environment by image") } + return fmt.Errorf("health monitoring not allowed by image") } - if ls.MemoryMonitoringEnabled { - if monitoringPolicy == none { - return fmt.Errorf("image does not allow any monitoring") + if ls.MemoryMonitoringEnabled && monitoringPolicy == none { + // Check if there is an issue with the image type. + if ls.Hardened && p.DebugImageMonitoring == memoryOnly { + return fmt.Errorf("memory monitoring only allowed on debug environment by image") } + return fmt.Errorf("memory monitoring not allowed by image") } var err error From c925837eb0583fdbfb9661e38b1b1e13ee4ca591 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Tue, 24 Sep 2024 17:55:34 +0000 Subject: [PATCH 16/27] implement now policy/spec format --- launcher/internal/healthmonitoring/config.go | 62 ++++++++++++++++++++ launcher/spec/launch_policy.go | 60 ++++++------------- launcher/spec/launch_spec.go | 20 ++++--- 3 files changed, 93 insertions(+), 49 deletions(-) create mode 100644 launcher/internal/healthmonitoring/config.go diff --git a/launcher/internal/healthmonitoring/config.go b/launcher/internal/healthmonitoring/config.go new file mode 100644 index 000000000..8a978a12e --- /dev/null +++ b/launcher/internal/healthmonitoring/config.go @@ -0,0 +1,62 @@ +package monitoring + +import ( + "fmt" + "strings" +) + +type Config struct { + CPU bool + Disk bool + Host bool + Memory bool +} + +func AllConfig() Config { + return Config{CPU: true, Disk: true, Host: true, Memory: true} +} + +func NoneConfig() Config { + return Config{CPU: false, Disk: false, Host: false, Memory: false} +} + +func ToConfig(data string) (Config, error) { + // We currently only support a single value for this field. + switch strings.ToLower(data) { + case "none": + return NoneConfig(), nil + case "all": + return AllConfig(), nil + case "memory": + return Config{Memory: true}, nil + } + + return Config{}, fmt.Errorf("invalid monitoring type: %v", data) +} + +// Verifies that each monitoring component enabled by spec is also enabled by policy. +func CheckCompliance(policy Config, spec Config) error { + invalidConfigs := []string{} + + if !policy.CPU && spec.CPU { + invalidConfigs = append(invalidConfigs, "CPU") + } + + if !policy.Disk && spec.Disk { + invalidConfigs = append(invalidConfigs, "DISK") + } + + if !policy.Host && spec.Host { + invalidConfigs = append(invalidConfigs, "HOST") + } + + if !policy.Memory && spec.Memory { + invalidConfigs = append(invalidConfigs, "MEMORY") + } + + if len(invalidConfigs) > 0 { + return fmt.Errorf("the following monitoring types were enabled, but disallowed by launch policy: %v", invalidConfigs) + } + + return nil +} diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index efa486a29..dba02e1e8 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -7,6 +7,8 @@ import ( "path/filepath" "strconv" "strings" + + "github.com/google/go-tpm-tools/launcher/internal/healthmonitoring/monitoring" ) // LaunchPolicy contains policies on starting the container. @@ -16,8 +18,8 @@ type LaunchPolicy struct { AllowedCmdOverride bool AllowedLogRedirect policy AllowedMountDestinations []string - HardenedImageMonitoring monitoringType - DebugImageMonitoring monitoringType + HardenedImageMonitoring monitoring.Config + DebugImageMonitoring monitoring.Config } type policy int @@ -100,57 +102,45 @@ func configureMonitoringPolicy(imageLabels map[string]string, launchPolicy *Laun logger.Printf("%s will be deprecated, use %s and %s instead", memoryMonitoring, hardenedMonitoring, debugMonitoring) + memoryOnlyConfig := monitoring.Config{CPU: false, Disk: false, Host: false, Memory: false} switch policy { case always: - logger.Printf("%s=always, will be treated as %s=memory_only and %s=memory_only", memoryMonitoring, hardenedMonitoring, debugMonitoring) - launchPolicy.HardenedImageMonitoring = memoryOnly - launchPolicy.DebugImageMonitoring = memoryOnly + logger.Printf("%s=always, will be treated as %s=memory and %s=memory", memoryMonitoring, hardenedMonitoring, debugMonitoring) + launchPolicy.HardenedImageMonitoring = memoryOnlyConfig + launchPolicy.DebugImageMonitoring = memoryOnlyConfig case never: logger.Printf("%s=never, will be treated as %s=none and %s=none", memoryMonitoring, hardenedMonitoring, debugMonitoring) - launchPolicy.HardenedImageMonitoring = none - launchPolicy.DebugImageMonitoring = none + launchPolicy.HardenedImageMonitoring = memoryOnlyConfig + launchPolicy.DebugImageMonitoring = monitoring.NoneConfig() case debugOnly: - logger.Printf("%s=debug_only, will be treated as %s=none and %s=memory_only", memoryMonitoring, hardenedMonitoring, debugMonitoring) - launchPolicy.HardenedImageMonitoring = none - launchPolicy.DebugImageMonitoring = memoryOnly + logger.Printf("%s=debug_only, will be treated as %s=none and %s=memory", memoryMonitoring, hardenedMonitoring, debugMonitoring) + launchPolicy.HardenedImageMonitoring = monitoring.NoneConfig() + launchPolicy.DebugImageMonitoring = memoryOnlyConfig } return nil } if hardenedOk { - launchPolicy.HardenedImageMonitoring, err = toMonitoringType(hardenedVal) + launchPolicy.HardenedImageMonitoring, err = monitoring.ToConfig(hardenedVal) if err != nil { return fmt.Errorf("invalid monitoring type for hardened image: %v", err) } } else { - launchPolicy.HardenedImageMonitoring = none + launchPolicy.HardenedImageMonitoring = monitoring.NoneConfig() } if debugOk { - launchPolicy.DebugImageMonitoring, err = toMonitoringType(debugVal) + launchPolicy.DebugImageMonitoring, err = monitoring.ToConfig(debugVal) if err != nil { return fmt.Errorf("invalid monitoring type for debug image: %v", err) } } else { - launchPolicy.DebugImageMonitoring = health + launchPolicy.DebugImageMonitoring = monitoring.AllConfig() } return nil } -func toMonitoringType(label string) (monitoringType, error) { - switch strings.ToLower(label) { - case "none": - return none, nil - case "memoryonly": - return memoryOnly, nil - case "health": - return health, nil - } - - return none, fmt.Errorf("invalid monitoring type: %v", label) -} - // GetLaunchPolicy takes in a map[string] string which should come from image labels, // and will try to parse it into a LaunchPolicy. Extra fields will be ignored. func GetLaunchPolicy(imageLabels map[string]string, logger *log.Logger) (LaunchPolicy, error) { @@ -224,20 +214,8 @@ func (p LaunchPolicy) Verify(ls LaunchSpec) error { monitoringPolicy = p.HardenedImageMonitoring } - if ls.HealthMonitoringEnabled && monitoringPolicy != health { - // Check if there is an issue with the image type. - if ls.Hardened && p.DebugImageMonitoring == health { - return fmt.Errorf("health monitoring only allowed on debug environment by image") - } - return fmt.Errorf("health monitoring not allowed by image") - } - - if ls.MemoryMonitoringEnabled && monitoringPolicy == none { - // Check if there is an issue with the image type. - if ls.Hardened && p.DebugImageMonitoring == memoryOnly { - return fmt.Errorf("memory monitoring only allowed on debug environment by image") - } - return fmt.Errorf("memory monitoring not allowed by image") + if err := monitoring.CheckCompliance(monitoringPolicy, ls.MonitoringEnabled); err != nil { + return fmt.Errorf("error verifying monitoring configs: %v", err) } var err error diff --git a/launcher/spec/launch_spec.go b/launcher/spec/launch_spec.go index 6d3551219..879e2085a 100644 --- a/launcher/spec/launch_spec.go +++ b/launcher/spec/launch_spec.go @@ -18,6 +18,7 @@ import ( "github.com/google/go-tpm-tools/cel" "github.com/google/go-tpm-tools/launcher/internal/experiments" + monitoring "github.com/google/go-tpm-tools/launcher/internal/healthmonitoring" "github.com/google/go-tpm-tools/launcher/internal/launchermount" "github.com/google/go-tpm-tools/launcher/launcherfile" "github.com/google/go-tpm-tools/verifier/util" @@ -83,7 +84,7 @@ const ( attestationServiceAddrKey = "tee-attestation-service-endpoint" logRedirectKey = "tee-container-log-redirect" memoryMonitoringEnable = "tee-monitoring-memory-enable" - healthMonitoringEnable = "tee-monitoring-health-enable" + monitoringEnable = "tee-monitoring-enable" devShmSizeKey = "tee-dev-shm-size-kb" mountKey = "tee-mount" ) @@ -115,7 +116,7 @@ type LaunchSpec struct { Region string Hardened bool MemoryMonitoringEnabled bool - HealthMonitoringEnabled bool + MonitoringEnabled monitoring.Config LogRedirect LogRedirectLocation Mounts []launchermount.Mount // DevShmSize is specified in kiB. @@ -157,18 +158,21 @@ func (s *LaunchSpec) UnmarshalJSON(b []byte) error { } memVal, memOk := unmarshaledMap[memoryMonitoringEnable] - healthVal, healthOk := unmarshaledMap[healthMonitoringEnable] + monVal, monOk := unmarshaledMap[monitoringEnable] - if memOk && healthOk { - return fmt.Errorf("both %v and %v are specified, only one is permitted", memoryMonitoringEnable, healthMonitoringEnable) + if memOk && monOk { + return fmt.Errorf("both %v and %v are specified, only one is permitted", memoryMonitoringEnable, monitoringEnable) } else if memOk && memVal != "" { if boolValue, err := strconv.ParseBool(memVal); err == nil { s.MemoryMonitoringEnabled = boolValue } - } else if healthOk && healthVal != "" { - if boolValue, err := strconv.ParseBool(healthVal); err == nil { - s.HealthMonitoringEnabled = boolValue + } else if monOk && monVal != "" { + monCfg, err := monitoring.ToConfig(monVal) + if err != nil { + return err } + + s.MonitoringEnabled = monCfg } // Populate cmd override. From 67f52a4409573526420f1904379ef2fafec21a0d Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Mon, 7 Oct 2024 21:56:06 +0000 Subject: [PATCH 17/27] add ALL config --- launcher/container_runner.go | 11 -- launcher/internal/healthmonitoring/config.go | 2 +- .../nodeproblemdetector/systemstats_config.go | 52 ++++-- .../systemstats_config_test.go | 4 +- launcher/launcher/main.go | 12 +- launcher/spec/launch_policy.go | 70 +++++--- launcher/spec/launch_policy_test.go | 160 ++++++++---------- launcher/spec/launch_spec.go | 15 +- launcher/spec/launch_spec_test.go | 35 +--- 9 files changed, 189 insertions(+), 172 deletions(-) diff --git a/launcher/container_runner.go b/launcher/container_runner.go index b130be37f..d4be30c91 100644 --- a/launcher/container_runner.go +++ b/launcher/container_runner.go @@ -29,7 +29,6 @@ import ( "github.com/google/go-tpm-tools/cel" "github.com/google/go-tpm-tools/client" "github.com/google/go-tpm-tools/launcher/agent" - "github.com/google/go-tpm-tools/launcher/internal/healthmonitoring/nodeproblemdetector" "github.com/google/go-tpm-tools/launcher/internal/signaturediscovery" "github.com/google/go-tpm-tools/launcher/launcherfile" "github.com/google/go-tpm-tools/launcher/registryauth" @@ -521,16 +520,6 @@ func (r *ContainerRunner) Run(ctx context.Context) error { go teeServer.Serve() defer teeServer.Shutdown(ctx) - // start node-problem-detector.service to collect memory related metrics. - if r.launchSpec.MemoryMonitoringEnabled { - r.logger.Println("MemoryMonitoring is enabled by the VM operator") - if err := nodeproblemdetector.StartService(r.logger); err != nil { - return err - } - } else { - r.logger.Println("MemoryMonitoring is disabled by the VM operator") - } - var streamOpt cio.Opt switch r.launchSpec.LogRedirect { case spec.Nowhere: diff --git a/launcher/internal/healthmonitoring/config.go b/launcher/internal/healthmonitoring/config.go index 8a978a12e..25d2d445c 100644 --- a/launcher/internal/healthmonitoring/config.go +++ b/launcher/internal/healthmonitoring/config.go @@ -1,4 +1,4 @@ -package monitoring +package healthmonitoring import ( "fmt" diff --git a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go index 58426603f..cbc87a39b 100644 --- a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go +++ b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go @@ -23,13 +23,20 @@ type statsConfig struct { MetricsConfigs map[string]metricConfig `json:"metricsConfigs"` } +type diskConfig struct { + IncludeAllAttachedBlk bool `json:"includeAllAttachedBlk"` + IncludeRootBlk bool `json:includeRootBlk` + LsblkTimeout string `json:lsblkTimeout` + MetricsConfigs *statsConfig `json:metricsConfigs` +} + // SystemStatsConfig contains configurations for `System Stats Monitor`, // a problem daemon in node-problem-detector that collects pre-defined health-related metrics from different system components. // For now we only consider collecting memory related metrics. // View the comprehensive configuration details on https://github.com/kubernetes/node-problem-detector/tree/master/pkg/systemstatsmonitor#detailed-configuration-options type SystemStatsConfig struct { CPU *statsConfig `json:"cpu,omitempty"` - Disk *statsConfig `json:"disk,omitempty"` + Disk *diskConfig `json:"disk,omitempty"` Host *statsConfig `json:"host,omitempty"` Memory *statsConfig `json:"memory,omitempty"` InvokeInterval string `json:"invokeInterval,omitempty"` @@ -43,25 +50,50 @@ func NewSystemStatsConfig() SystemStatsConfig { } } -var healthConfig = &SystemStatsConfig{ +var allConfig = &SystemStatsConfig{ CPU: &statsConfig{map[string]metricConfig{ - "cpu/load_5m": {"cpu/load_5m"}, - }}, - Disk: &statsConfig{map[string]metricConfig{ - "disk/percent_used": {"disk/percent_used"}, + "cpu/runnable_task_count": {"cpu/runnable_task_count"}, + "cpu/usage_time": {"cpu/usage_time"}, + "cpu/load_1m": {"cpu/load_1m"}, + "cpu/load_5m": {"cpu/load_5m"}, + "cpu/load_15m": {"cpu/load_15m"}, + "system/cpu_stat": {"system/cpu_stat"}, + "system/interrupts_total": {"system/interrupts_total"}, + "system/processes_total": {"system/processes_total"}, + "system/procs_blocked": {"system/procs_blocked"}, + "system/procs_running": {"system/procs_running"}, }}, + Disk: &diskConfig{ + true, true, "5s", + &statsConfig{map[string]metricConfig{ + "disk/avg_queue_len": {"disk/avg_queue_len"}, + "disk/bytes_used": {"disk/bytes_used"}, + "disk/percent_used": {"disk/percent_used"}, + "disk/io_time": {"disk/io_time"}, + "disk/merged_operation_count": {"disk/merged_operation_count"}, + "disk/operation_bytes_count": {"disk/operation_bytes_count"}, + "disk/operation_count": {"disk/operation_count"}, + "disk/operation_time": {"disk/operation_time"}, + "disk/weighted_io": {"disk/weighted_io"}, + }}, + }, Host: &statsConfig{map[string]metricConfig{ "host/uptime": {"host/uptime"}, }}, Memory: &statsConfig{map[string]metricConfig{ - "memory/bytes_used": {"memory/bytes_used"}, + "memory/anonymous_used": {"memory/anonymous_used"}, + "memory/bytes_used": {"memory/bytes_used"}, + "memory/dirty_used": {"memory/dirty_used"}, + "memory/page_cache_used": {"memory/page_cache_used"}, + "memory/unevictable_used": {"memory/unevictable_used"}, + "memory/percent_used": {"memory/percent_used"}, }}, InvokeInterval: defaultInvokeIntervalString, } -// EnableHealthMonitoringConfig overwrites system stats config with health monitoring config. -func EnableHealthMonitoringConfig() error { - return healthConfig.WriteFile(systemStatsFilePath) +// EnableAllConfig overwrites system stats config with health monitoring config. +func EnableAllConfig() error { + return allConfig.WriteFile(systemStatsFilePath) } // EnableMemoryBytesUsed enables "memory/bytes_used" for memory monitoring. diff --git a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config_test.go b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config_test.go index 6176d5a32..71473178e 100644 --- a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config_test.go +++ b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config_test.go @@ -16,12 +16,12 @@ func TestEnableHealthMonitoringConfig(t *testing.T) { tmpDir := t.TempDir() systemStatsFilePath = path.Join(tmpDir, "system-stats-monitor.json") - wantBytes, err := json.Marshal(healthConfig) + wantBytes, err := json.Marshal(allConfig) if err != nil { t.Fatalf("Error marshaling expected config: %v", err) } - EnableHealthMonitoringConfig() + EnableAllConfig() file, err := os.OpenFile(systemStatsFilePath, os.O_RDONLY, 0) if err != nil { diff --git a/launcher/launcher/main.go b/launcher/launcher/main.go index b884ed00c..63db00213 100644 --- a/launcher/launcher/main.go +++ b/launcher/launcher/main.go @@ -96,12 +96,14 @@ func main() { return } - if launchSpec.HealthMonitoringEnabled { - logger.Printf("Health Monitoring is enabled by the VM operator") + if launchSpec.MonitoringEnabled != spec.None { + logger.Printf("Monitoring is enabled by the VM operator") - if err := nodeproblemdetector.EnableHealthMonitoringConfig(); err != nil { - logger.Printf("failed to enable Health Monitoring config: %v", err) - return + if launchSpec.MonitoringEnabled == spec.All { + if err := nodeproblemdetector.EnableAllConfig(); err != nil { + logger.Printf("failed to enable Health Monitoring config: %v", err) + return + } } if err := nodeproblemdetector.StartService(logger); err != nil { diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index dba02e1e8..23e149b2e 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -7,8 +7,6 @@ import ( "path/filepath" "strconv" "strings" - - "github.com/google/go-tpm-tools/launcher/internal/healthmonitoring/monitoring" ) // LaunchPolicy contains policies on starting the container. @@ -18,8 +16,8 @@ type LaunchPolicy struct { AllowedCmdOverride bool AllowedLogRedirect policy AllowedMountDestinations []string - HardenedImageMonitoring monitoring.Config - DebugImageMonitoring monitoring.Config + HardenedImageMonitoring MonitoringType + DebugImageMonitoring MonitoringType } type policy int @@ -30,14 +28,27 @@ const ( never ) -type monitoringType int +type MonitoringType int const ( - none monitoringType = iota - memoryOnly - health + None MonitoringType = iota + MemoryOnly + All ) +func toMonitoringType(s string) (MonitoringType, error) { + switch strings.ToLower(s) { + case "none": + return None, nil + case "memoryonly": + return MemoryOnly, nil + case "all": + return All, nil + } + + return None, fmt.Errorf("invalid monitoring type %v", s) +} + // String returns LaunchPolicy details. func (p policy) String() string { switch p { @@ -102,40 +113,39 @@ func configureMonitoringPolicy(imageLabels map[string]string, launchPolicy *Laun logger.Printf("%s will be deprecated, use %s and %s instead", memoryMonitoring, hardenedMonitoring, debugMonitoring) - memoryOnlyConfig := monitoring.Config{CPU: false, Disk: false, Host: false, Memory: false} switch policy { case always: - logger.Printf("%s=always, will be treated as %s=memory and %s=memory", memoryMonitoring, hardenedMonitoring, debugMonitoring) - launchPolicy.HardenedImageMonitoring = memoryOnlyConfig - launchPolicy.DebugImageMonitoring = memoryOnlyConfig + logger.Printf("%s=always, will be treated as %s=memory_only and %s=memory_only", memoryMonitoring, hardenedMonitoring, debugMonitoring) + launchPolicy.HardenedImageMonitoring = MemoryOnly + launchPolicy.DebugImageMonitoring = MemoryOnly case never: logger.Printf("%s=never, will be treated as %s=none and %s=none", memoryMonitoring, hardenedMonitoring, debugMonitoring) - launchPolicy.HardenedImageMonitoring = memoryOnlyConfig - launchPolicy.DebugImageMonitoring = monitoring.NoneConfig() + launchPolicy.HardenedImageMonitoring = None + launchPolicy.DebugImageMonitoring = None case debugOnly: logger.Printf("%s=debug_only, will be treated as %s=none and %s=memory", memoryMonitoring, hardenedMonitoring, debugMonitoring) - launchPolicy.HardenedImageMonitoring = monitoring.NoneConfig() - launchPolicy.DebugImageMonitoring = memoryOnlyConfig + launchPolicy.HardenedImageMonitoring = None + launchPolicy.DebugImageMonitoring = MemoryOnly } return nil } if hardenedOk { - launchPolicy.HardenedImageMonitoring, err = monitoring.ToConfig(hardenedVal) + launchPolicy.HardenedImageMonitoring, err = toMonitoringType(hardenedVal) if err != nil { return fmt.Errorf("invalid monitoring type for hardened image: %v", err) } } else { - launchPolicy.HardenedImageMonitoring = monitoring.NoneConfig() + launchPolicy.HardenedImageMonitoring = None } if debugOk { - launchPolicy.DebugImageMonitoring, err = monitoring.ToConfig(debugVal) + launchPolicy.DebugImageMonitoring, err = toMonitoringType(debugVal) if err != nil { return fmt.Errorf("invalid monitoring type for debug image: %v", err) } } else { - launchPolicy.DebugImageMonitoring = monitoring.AllConfig() + launchPolicy.DebugImageMonitoring = MemoryOnly } return nil @@ -189,6 +199,22 @@ func GetLaunchPolicy(imageLabels map[string]string, logger *log.Logger) (LaunchP return launchPolicy, nil } +func verifyMonitoringConfig(policy MonitoringType, spec MonitoringType) error { + if policy == None { + if spec != None { + return fmt.Errorf("spec configured for %v but policy is none", spec) + } + + return nil + } else if policy == MemoryOnly { + if spec == All { + return fmt.Errorf("spec configured all monitoring, policy only allows memory") + } + } + + return nil +} + // Verify will use the LaunchPolicy to verify the given LaunchSpec. If the verification passed, will return nil. // If there are multiple violations, the function will return the first error. func (p LaunchPolicy) Verify(ls LaunchSpec) error { @@ -214,8 +240,8 @@ func (p LaunchPolicy) Verify(ls LaunchSpec) error { monitoringPolicy = p.HardenedImageMonitoring } - if err := monitoring.CheckCompliance(monitoringPolicy, ls.MonitoringEnabled); err != nil { - return fmt.Errorf("error verifying monitoring configs: %v", err) + if err := verifyMonitoringConfig(monitoringPolicy, ls.MonitoringEnabled); err != nil { + return fmt.Errorf("error verifying monitoring config: %v", err) } var err error diff --git a/launcher/spec/launch_policy_test.go b/launcher/spec/launch_policy_test.go index b3611577b..b93943979 100644 --- a/launcher/spec/launch_policy_test.go +++ b/launcher/spec/launch_policy_test.go @@ -59,8 +59,8 @@ func TestLaunchPolicy(t *testing.T) { for _, testcase := range testCases { t.Run(testcase.testName, func(t *testing.T) { // Add default values for policy fields. Not relevant to tested behavior. - testcase.expectedPolicy.HardenedImageMonitoring = none - testcase.expectedPolicy.DebugImageMonitoring = health + testcase.expectedPolicy.HardenedImageMonitoring = None + testcase.expectedPolicy.DebugImageMonitoring = MemoryOnly got, err := GetLaunchPolicy(testcase.imageLabels, log.Default()) if err != nil { @@ -87,14 +87,14 @@ func TestVerify(t *testing.T) { AllowedEnvOverride: []string{"foo"}, AllowedCmdOverride: true, AllowedLogRedirect: always, - HardenedImageMonitoring: memoryOnly, - DebugImageMonitoring: memoryOnly, + HardenedImageMonitoring: MemoryOnly, + DebugImageMonitoring: MemoryOnly, }, LaunchSpec{ - Envs: []EnvVar{{Name: "foo", Value: "foo"}}, - Cmd: []string{"foo"}, - LogRedirect: Everywhere, - MemoryMonitoringEnabled: true, + Envs: []EnvVar{{Name: "foo", Value: "foo"}}, + Cmd: []string{"foo"}, + LogRedirect: Everywhere, + MonitoringEnabled: MemoryOnly, }, false, }, @@ -513,65 +513,55 @@ func TestVerify(t *testing.T) { func TestVerifyMonitoringSettings(t *testing.T) { testCases := []struct { testName string - monitoring monitoringType + monitoring MonitoringType spec LaunchSpec }{ { "none policy, disabled by spec", - none, + None, LaunchSpec{ - HealthMonitoringEnabled: false, - MemoryMonitoringEnabled: false, - LogRedirect: Nowhere, + MonitoringEnabled: None, + LogRedirect: Nowhere, }, }, { "memory-only policy, all disabled by spec", - memoryOnly, + MemoryOnly, LaunchSpec{ - HealthMonitoringEnabled: false, - MemoryMonitoringEnabled: false, - LogRedirect: Nowhere, + MonitoringEnabled: None, + LogRedirect: Nowhere, }, }, { "memory-only policy, memory enabled by spec", - memoryOnly, + MemoryOnly, LaunchSpec{ - MemoryMonitoringEnabled: true, - LogRedirect: Nowhere, + MonitoringEnabled: MemoryOnly, + LogRedirect: Nowhere, }, }, { - "health policy, health enabled by spec", - health, + "all enabled by policy, all enabled by spec", + All, LaunchSpec{ - HealthMonitoringEnabled: true, - LogRedirect: Nowhere, + MonitoringEnabled: All, + LogRedirect: Nowhere, }, }, { - "health policy, health disabled by spec", - health, + "all enabled by policy, disabled by spec", + All, LaunchSpec{ - HealthMonitoringEnabled: false, - LogRedirect: Nowhere, + MonitoringEnabled: None, + LogRedirect: Nowhere, }, }, { - "health policy, memory enabled by spec", - health, + "all enabled by policy, memory enabled by spec", + All, LaunchSpec{ - MemoryMonitoringEnabled: true, - LogRedirect: Nowhere, - }, - }, - { - "health policy, memory disabled by spec", - health, - LaunchSpec{ - MemoryMonitoringEnabled: false, - LogRedirect: Nowhere, + MonitoringEnabled: MemoryOnly, + LogRedirect: Nowhere, }, }, } @@ -606,34 +596,34 @@ func TestVerifyMonitoringSettings(t *testing.T) { func TestVerifyMonitoringSettingsErrors(t *testing.T) { testCases := []struct { testName string - monitoring monitoringType + monitoring MonitoringType spec LaunchSpec }{ { - "[Hardened] disabled policy, Health enabled by spec", - none, + "[Hardened] disabled policy, all enabled by spec", + None, LaunchSpec{ - HealthMonitoringEnabled: true, - Hardened: true, - LogRedirect: Nowhere, + MonitoringEnabled: All, + Hardened: true, + LogRedirect: Nowhere, }, }, { - "[Hardened] disabled policy, Memory enabled by spec", - none, + "[Hardened] disabled policy, memory enabled by spec", + None, LaunchSpec{ - MemoryMonitoringEnabled: true, - Hardened: true, - LogRedirect: Nowhere, + MonitoringEnabled: MemoryOnly, + Hardened: true, + LogRedirect: Nowhere, }, }, { - "[Hardened] memory-only policy, Health enabled by spec", - memoryOnly, + "[Hardened] memory-only policy, all enabled by spec", + MemoryOnly, LaunchSpec{ - HealthMonitoringEnabled: true, - Hardened: true, - LogRedirect: Nowhere, + MonitoringEnabled: All, + Hardened: true, + LogRedirect: Nowhere, }, }, } @@ -717,8 +707,8 @@ func TestGetMonitoringPolicy(t *testing.T) { memoryMonitoring: "always", }, expectedPolicy: &LaunchPolicy{ - HardenedImageMonitoring: memoryOnly, - DebugImageMonitoring: memoryOnly, + HardenedImageMonitoring: MemoryOnly, + DebugImageMonitoring: MemoryOnly, }, }, { @@ -727,8 +717,8 @@ func TestGetMonitoringPolicy(t *testing.T) { memoryMonitoring: "never", }, expectedPolicy: &LaunchPolicy{ - HardenedImageMonitoring: none, - DebugImageMonitoring: none, + HardenedImageMonitoring: None, + DebugImageMonitoring: None, }, }, { @@ -737,8 +727,8 @@ func TestGetMonitoringPolicy(t *testing.T) { memoryMonitoring: "debugonly", }, expectedPolicy: &LaunchPolicy{ - HardenedImageMonitoring: none, - DebugImageMonitoring: memoryOnly, + HardenedImageMonitoring: None, + DebugImageMonitoring: MemoryOnly, }, }, { @@ -747,8 +737,8 @@ func TestGetMonitoringPolicy(t *testing.T) { hardenedMonitoring: "none", }, expectedPolicy: &LaunchPolicy{ - HardenedImageMonitoring: none, - DebugImageMonitoring: health, + HardenedImageMonitoring: None, + DebugImageMonitoring: MemoryOnly, }, }, { @@ -757,18 +747,18 @@ func TestGetMonitoringPolicy(t *testing.T) { hardenedMonitoring: "memoryonly", }, expectedPolicy: &LaunchPolicy{ - HardenedImageMonitoring: memoryOnly, - DebugImageMonitoring: health, + HardenedImageMonitoring: MemoryOnly, + DebugImageMonitoring: MemoryOnly, }, }, { - name: "HardenedImageMonitoring=health", + name: "HardenedImageMonitoring=all", labels: map[string]string{ - hardenedMonitoring: "health", + hardenedMonitoring: "all", }, expectedPolicy: &LaunchPolicy{ - HardenedImageMonitoring: health, - DebugImageMonitoring: health, + HardenedImageMonitoring: All, + DebugImageMonitoring: All, }, }, { @@ -777,8 +767,8 @@ func TestGetMonitoringPolicy(t *testing.T) { debugMonitoring: "none", }, expectedPolicy: &LaunchPolicy{ - HardenedImageMonitoring: none, - DebugImageMonitoring: none, + HardenedImageMonitoring: None, + DebugImageMonitoring: None, }, }, { @@ -787,30 +777,30 @@ func TestGetMonitoringPolicy(t *testing.T) { debugMonitoring: "memoryonly", }, expectedPolicy: &LaunchPolicy{ - HardenedImageMonitoring: none, - DebugImageMonitoring: memoryOnly, + HardenedImageMonitoring: None, + DebugImageMonitoring: MemoryOnly, }, }, { - name: "DebugImageMonitoring=health", + name: "DebugImageMonitoring=all", labels: map[string]string{ - debugMonitoring: "health", + debugMonitoring: "all", }, expectedPolicy: &LaunchPolicy{ - HardenedImageMonitoring: none, - DebugImageMonitoring: health, + HardenedImageMonitoring: None, + DebugImageMonitoring: All, }, }, // Set both fields to non-default values. { - name: "HardenedImageMonitoring=health, DebugImageMonitoring=none", + name: "HardenedImageMonitoring=all, DebugImageMonitoring=none", labels: map[string]string{ - hardenedMonitoring: "health", + hardenedMonitoring: "all", debugMonitoring: "none", }, expectedPolicy: &LaunchPolicy{ - HardenedImageMonitoring: health, - DebugImageMonitoring: none, + HardenedImageMonitoring: All, + DebugImageMonitoring: None, }, }, } @@ -839,21 +829,21 @@ func TestGetMonitoringPolicyErrors(t *testing.T) { name: "memory_monitoring_allow and hardened_monitoring specified", labels: map[string]string{ memoryMonitoring: "always", - hardenedMonitoring: "health", + hardenedMonitoring: "all", }, }, { name: "memory_monitoring_allow and debug_monitoring specified", labels: map[string]string{ memoryMonitoring: "always", - debugMonitoring: "health", + debugMonitoring: "all", }, }, { name: "memory_monitoring_allow, hardened_monitoring, and debug_monitoring specified", labels: map[string]string{ memoryMonitoring: "always", - hardenedMonitoring: "health", + hardenedMonitoring: "all", debugMonitoring: "memoryOnly", }, }, diff --git a/launcher/spec/launch_spec.go b/launcher/spec/launch_spec.go index 879e2085a..69a8eac93 100644 --- a/launcher/spec/launch_spec.go +++ b/launcher/spec/launch_spec.go @@ -18,7 +18,6 @@ import ( "github.com/google/go-tpm-tools/cel" "github.com/google/go-tpm-tools/launcher/internal/experiments" - monitoring "github.com/google/go-tpm-tools/launcher/internal/healthmonitoring" "github.com/google/go-tpm-tools/launcher/internal/launchermount" "github.com/google/go-tpm-tools/launcher/launcherfile" "github.com/google/go-tpm-tools/verifier/util" @@ -115,8 +114,7 @@ type LaunchSpec struct { ProjectID string Region string Hardened bool - MemoryMonitoringEnabled bool - MonitoringEnabled monitoring.Config + MonitoringEnabled MonitoringType LogRedirect LogRedirectLocation Mounts []launchermount.Mount // DevShmSize is specified in kiB. @@ -163,16 +161,17 @@ func (s *LaunchSpec) UnmarshalJSON(b []byte) error { if memOk && monOk { return fmt.Errorf("both %v and %v are specified, only one is permitted", memoryMonitoringEnable, monitoringEnable) } else if memOk && memVal != "" { - if boolValue, err := strconv.ParseBool(memVal); err == nil { - s.MemoryMonitoringEnabled = boolValue + if boolValue, err := strconv.ParseBool(memVal); err == nil && boolValue { + s.MonitoringEnabled = MemoryOnly + } else { + s.MonitoringEnabled = None } } else if monOk && monVal != "" { - monCfg, err := monitoring.ToConfig(monVal) + var err error + s.MonitoringEnabled, err = toMonitoringType(monVal) if err != nil { return err } - - s.MonitoringEnabled = monCfg } // Populate cmd override. diff --git a/launcher/spec/launch_spec_test.go b/launcher/spec/launch_spec_test.go index a753bcfa5..83b7f18e2 100644 --- a/launcher/spec/launch_spec_test.go +++ b/launcher/spec/launch_spec_test.go @@ -37,8 +37,7 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { Envs: []EnvVar{{"foo", "bar"}}, ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, LogRedirect: Everywhere, - MemoryMonitoringEnabled: true, - HealthMonitoringEnabled: false, + MonitoringEnabled: MemoryOnly, DevShmSize: 234234, Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, @@ -66,8 +65,7 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { Envs: []EnvVar{{"foo", "bar"}}, ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, LogRedirect: Everywhere, - MemoryMonitoringEnabled: false, - HealthMonitoringEnabled: true, + MonitoringEnabled: None, DevShmSize: 234234, Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, @@ -97,8 +95,7 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { Envs: []EnvVar{{"foo", "bar"}}, ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, LogRedirect: Everywhere, - MemoryMonitoringEnabled: true, - HealthMonitoringEnabled: false, + MonitoringEnabled: MemoryOnly, DevShmSize: 234234, Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, @@ -106,24 +103,6 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { }, } - want := &LaunchSpec{ - ImageRef: "docker.io/library/hello-world:latest", - SignedImageRepos: []string{"docker.io/library/hello-world", "gcr.io/cloudrun/hello"}, - RestartPolicy: Always, - Cmd: []string{"--foo", "--bar", "--baz"}, - Envs: []EnvVar{{"foo", "bar"}}, - ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, - LogRedirect: Everywhere, - MemoryMonitoringEnabled: true, - HealthMonitoringEnabled: false, - DevShmSize: 234234, - Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, - launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, - Experiments: experiments.Experiments{ - EnableTempFSMount: true, - }, - } - for _, testcase := range testCases { t.Run(testcase.testName, func(t *testing.T) { spec := &LaunchSpec{} @@ -215,10 +194,10 @@ func TestLaunchSpecUnmarshalJSONWithDefaultValue(t *testing.T) { } want := &LaunchSpec{ - ImageRef: "docker.io/library/hello-world:latest", - RestartPolicy: Never, - LogRedirect: Nowhere, - MemoryMonitoringEnabled: false, + ImageRef: "docker.io/library/hello-world:latest", + RestartPolicy: Never, + LogRedirect: Nowhere, + MonitoringEnabled: None, } if !cmp.Equal(spec, want) { From c7c090632072f32822b5a0893133bd9d7da52bee Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Mon, 7 Oct 2024 22:30:43 +0000 Subject: [PATCH 18/27] fix spec tests --- launcher/spec/launch_policy_test.go | 2 +- launcher/spec/launch_spec_test.go | 82 ++++++++--------------------- 2 files changed, 23 insertions(+), 61 deletions(-) diff --git a/launcher/spec/launch_policy_test.go b/launcher/spec/launch_policy_test.go index b93943979..9d4fdaf31 100644 --- a/launcher/spec/launch_policy_test.go +++ b/launcher/spec/launch_policy_test.go @@ -758,7 +758,7 @@ func TestGetMonitoringPolicy(t *testing.T) { }, expectedPolicy: &LaunchPolicy{ HardenedImageMonitoring: All, - DebugImageMonitoring: All, + DebugImageMonitoring: MemoryOnly, }, }, { diff --git a/launcher/spec/launch_spec_test.go b/launcher/spec/launch_spec_test.go index 83b7f18e2..ae8acb2f5 100644 --- a/launcher/spec/launch_spec_test.go +++ b/launcher/spec/launch_spec_test.go @@ -11,12 +11,11 @@ import ( func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { var testCases = []struct { - testName string - mdsJSON string - expectedSpec *LaunchSpec + testName string + mdsJSON string }{ { - "HappyCase_MemMonitor", + "HappyCase", `{ "tee-cmd":"[\"--foo\",\"--bar\",\"--baz\"]", "tee-env-foo":"bar", @@ -29,47 +28,6 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { "tee-dev-shm-size-kb":"234234", "tee-mount":"type=tmpfs,source=tmpfs,destination=/tmpmount;type=tmpfs,source=tmpfs,destination=/sized,size=222" }`, - &LaunchSpec{ - ImageRef: "docker.io/library/hello-world:latest", - SignedImageRepos: []string{"docker.io/library/hello-world", "gcr.io/cloudrun/hello"}, - RestartPolicy: Always, - Cmd: []string{"--foo", "--bar", "--baz"}, - Envs: []EnvVar{{"foo", "bar"}}, - ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, - LogRedirect: Everywhere, - MonitoringEnabled: MemoryOnly, - DevShmSize: 234234, - Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, - launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, - }, - }, - { - "HappyCase_HealthMonitor", - `{ - "tee-cmd":"[\"--foo\",\"--bar\",\"--baz\"]", - "tee-env-foo":"bar", - "tee-image-reference":"docker.io/library/hello-world:latest", - "tee-signed-image-repos":"docker.io/library/hello-world,gcr.io/cloudrun/hello", - "tee-restart-policy":"Always", - "tee-impersonate-service-accounts":"sv1@developer.gserviceaccount.com,sv2@developer.gserviceaccount.com", - "tee-container-log-redirect":"true", - "tee-monitoring-health-enable":"true", - "tee-dev-shm-size-kb":"234234", - "tee-mount":"type=tmpfs,source=tmpfs,destination=/tmpmount;type=tmpfs,source=tmpfs,destination=/sized,size=222" - }`, - &LaunchSpec{ - ImageRef: "docker.io/library/hello-world:latest", - SignedImageRepos: []string{"docker.io/library/hello-world", "gcr.io/cloudrun/hello"}, - RestartPolicy: Always, - Cmd: []string{"--foo", "--bar", "--baz"}, - Envs: []EnvVar{{"foo", "bar"}}, - ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, - LogRedirect: Everywhere, - MonitoringEnabled: None, - DevShmSize: 234234, - Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, - launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, - }, }, { "HappyCaseWithExtraUnknownFields", @@ -87,19 +45,23 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { "tee-dev-shm-size-kb":"234234", "tee-mount":"type=tmpfs,source=tmpfs,destination=/tmpmount;type=tmpfs,source=tmpfs,destination=/sized,size=222" }`, - &LaunchSpec{ - ImageRef: "docker.io/library/hello-world:latest", - SignedImageRepos: []string{"docker.io/library/hello-world", "gcr.io/cloudrun/hello"}, - RestartPolicy: Always, - Cmd: []string{"--foo", "--bar", "--baz"}, - Envs: []EnvVar{{"foo", "bar"}}, - ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, - LogRedirect: Everywhere, - MonitoringEnabled: MemoryOnly, - DevShmSize: 234234, - Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, - launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, - }, + }, + } + + want := &LaunchSpec{ + ImageRef: "docker.io/library/hello-world:latest", + SignedImageRepos: []string{"docker.io/library/hello-world", "gcr.io/cloudrun/hello"}, + RestartPolicy: Always, + Cmd: []string{"--foo", "--bar", "--baz"}, + Envs: []EnvVar{{"foo", "bar"}}, + ImpersonateServiceAccounts: []string{"sv1@developer.gserviceaccount.com", "sv2@developer.gserviceaccount.com"}, + LogRedirect: Everywhere, + MonitoringEnabled: MemoryOnly, + DevShmSize: 234234, + Mounts: []launchermount.Mount{launchermount.TmpfsMount{Destination: "/tmpmount", Size: 0}, + launchermount.TmpfsMount{Destination: "/sized", Size: 222}}, + Experiments: experiments.Experiments{ + EnableTempFSMount: true, }, } @@ -112,8 +74,8 @@ func TestLaunchSpecUnmarshalJSONHappyCases(t *testing.T) { if err := spec.UnmarshalJSON([]byte(testcase.mdsJSON)); err != nil { t.Fatal(err) } - if !cmp.Equal(spec, testcase.expectedSpec) { - t.Errorf("LaunchSpec UnmarshalJSON got %+v, want %+v", spec, testcase.expectedSpec) + if !cmp.Equal(spec, want) { + t.Errorf("LaunchSpec UnmarshalJSON got %+v, want %+v", spec, want) } }) } From 1eb3fc187afb3074e0f7a6e0334b7c4eb5a6329d Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Mon, 7 Oct 2024 22:37:17 +0000 Subject: [PATCH 19/27] fix container runner --- launcher/container_runner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/launcher/container_runner.go b/launcher/container_runner.go index d4be30c91..59a78897e 100644 --- a/launcher/container_runner.go +++ b/launcher/container_runner.go @@ -340,7 +340,7 @@ func (r *ContainerRunner) measureContainerClaims(ctx context.Context) error { // eventlog in the AttestationAgent. func (r *ContainerRunner) measureMemoryMonitor() error { var enabled uint8 - if r.launchSpec.MemoryMonitoringEnabled { + if r.launchSpec.MonitoringEnabled == spec.MemoryOnly { enabled = 1 } if err := r.attestAgent.MeasureEvent(cel.CosTlv{EventType: cel.MemoryMonitorType, EventContent: []byte{enabled}}); err != nil { From 5c22516d4cc46938d7995943d44385b1fd0109f2 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Mon, 7 Oct 2024 22:58:30 +0000 Subject: [PATCH 20/27] fix lint issues --- launcher/internal/healthmonitoring/config.go | 62 ------------------- .../nodeproblemdetector/systemstats_config.go | 6 +- 2 files changed, 3 insertions(+), 65 deletions(-) delete mode 100644 launcher/internal/healthmonitoring/config.go diff --git a/launcher/internal/healthmonitoring/config.go b/launcher/internal/healthmonitoring/config.go deleted file mode 100644 index 25d2d445c..000000000 --- a/launcher/internal/healthmonitoring/config.go +++ /dev/null @@ -1,62 +0,0 @@ -package healthmonitoring - -import ( - "fmt" - "strings" -) - -type Config struct { - CPU bool - Disk bool - Host bool - Memory bool -} - -func AllConfig() Config { - return Config{CPU: true, Disk: true, Host: true, Memory: true} -} - -func NoneConfig() Config { - return Config{CPU: false, Disk: false, Host: false, Memory: false} -} - -func ToConfig(data string) (Config, error) { - // We currently only support a single value for this field. - switch strings.ToLower(data) { - case "none": - return NoneConfig(), nil - case "all": - return AllConfig(), nil - case "memory": - return Config{Memory: true}, nil - } - - return Config{}, fmt.Errorf("invalid monitoring type: %v", data) -} - -// Verifies that each monitoring component enabled by spec is also enabled by policy. -func CheckCompliance(policy Config, spec Config) error { - invalidConfigs := []string{} - - if !policy.CPU && spec.CPU { - invalidConfigs = append(invalidConfigs, "CPU") - } - - if !policy.Disk && spec.Disk { - invalidConfigs = append(invalidConfigs, "DISK") - } - - if !policy.Host && spec.Host { - invalidConfigs = append(invalidConfigs, "HOST") - } - - if !policy.Memory && spec.Memory { - invalidConfigs = append(invalidConfigs, "MEMORY") - } - - if len(invalidConfigs) > 0 { - return fmt.Errorf("the following monitoring types were enabled, but disallowed by launch policy: %v", invalidConfigs) - } - - return nil -} diff --git a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go index cbc87a39b..7c97c967b 100644 --- a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go +++ b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go @@ -25,9 +25,9 @@ type statsConfig struct { type diskConfig struct { IncludeAllAttachedBlk bool `json:"includeAllAttachedBlk"` - IncludeRootBlk bool `json:includeRootBlk` - LsblkTimeout string `json:lsblkTimeout` - MetricsConfigs *statsConfig `json:metricsConfigs` + IncludeRootBlk bool `json:"includeRootBlk"` + LsblkTimeout string `json:"lsblkTimeout"` + MetricsConfigs *statsConfig `json:"metricsConfigs"` } // SystemStatsConfig contains configurations for `System Stats Monitor`, From fdfe013bb68f59ce71b557e8e56d396371c22532 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Mon, 7 Oct 2024 23:03:33 +0000 Subject: [PATCH 21/27] fix lint issues --- launcher/spec/launch_policy.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index 23e149b2e..6af7dee5a 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -28,11 +28,15 @@ const ( never ) +// MonitoringType represents the possible health monitoring presets for the client. type MonitoringType int const ( + // None indicates no monitoring enabled. None MonitoringType = iota + // MemoryOnly indicates only memory_bytes_used enabled. MemoryOnly + // All indicates all supported metrics enabled. All ) From a485857f5c44d365d9471111c7abb3aa0df97940 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Tue, 8 Oct 2024 18:02:33 +0000 Subject: [PATCH 22/27] fix logging for backcompat w tests --- launcher/launcher/main.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/launcher/launcher/main.go b/launcher/launcher/main.go index 63db00213..3b5783099 100644 --- a/launcher/launcher/main.go +++ b/launcher/launcher/main.go @@ -109,6 +109,9 @@ func main() { if err := nodeproblemdetector.StartService(logger); err != nil { logger.Print(err) } + } else { + // Avoids breaking existing memory monitoring tests. + logger.Printf("MemoryMonitoring is disabled by the VM operator") } defer func() { From f1fbd727b110f8dd49802dd19a6805ce8bb7bc00 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Wed, 9 Oct 2024 23:01:22 +0000 Subject: [PATCH 23/27] adjust logs for test backcompat --- launcher/container_runner.go | 5 +++++ launcher/launcher/main.go | 3 --- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/launcher/container_runner.go b/launcher/container_runner.go index 59a78897e..a73538966 100644 --- a/launcher/container_runner.go +++ b/launcher/container_runner.go @@ -520,6 +520,11 @@ func (r *ContainerRunner) Run(ctx context.Context) error { go teeServer.Serve() defer teeServer.Shutdown(ctx) + // Avoids breaking existing memory monitoring tests that depend on this log. + if r.launchSpec.MonitoringEnabled == spec.None { + r.logger.Printf("MemoryMonitoring is disabled by the VM operator") + } + var streamOpt cio.Opt switch r.launchSpec.LogRedirect { case spec.Nowhere: diff --git a/launcher/launcher/main.go b/launcher/launcher/main.go index 3b5783099..63db00213 100644 --- a/launcher/launcher/main.go +++ b/launcher/launcher/main.go @@ -109,9 +109,6 @@ func main() { if err := nodeproblemdetector.StartService(logger); err != nil { logger.Print(err) } - } else { - // Avoids breaking existing memory monitoring tests. - logger.Printf("MemoryMonitoring is disabled by the VM operator") } defer func() { From 3714ca0a7f1b1522165f2a550751a7f58671e8a3 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Thu, 10 Oct 2024 00:13:38 +0000 Subject: [PATCH 24/27] adjust mem-monitoring test logs --- launcher/image/test/test_memory_monitoring.yaml | 2 +- launcher/launcher/main.go | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/launcher/image/test/test_memory_monitoring.yaml b/launcher/image/test/test_memory_monitoring.yaml index e4eb49f79..ee496606b 100644 --- a/launcher/image/test/test_memory_monitoring.yaml +++ b/launcher/image/test/test_memory_monitoring.yaml @@ -23,7 +23,7 @@ steps: id: CheckMemoryMonitoringEnabled entrypoint: 'bash' # Search a regex pattern that ensures memory monitoring is enabled and measured into COS event logs. - args: ['scripts/test_memory_monitoring.sh', '${_VM_NAME_PREFIX}-enable-${BUILD_ID}', '${_ZONE}', 'Successfully measured memory monitoring event.*node-problem-detector.service successfully started'] + args: ['scripts/test_memory_monitoring.sh', '${_VM_NAME_PREFIX}-enable-${BUILD_ID}', '${_ZONE}', 'node-problem-detector.service successfully started.*Successfully measured memory monitoring event'] waitFor: ['CreateVMMemoryMemonitorEnabled'] - name: 'gcr.io/cloud-builders/gcloud' id: CleanUpVMMemoryMonitorEnabled diff --git a/launcher/launcher/main.go b/launcher/launcher/main.go index 63db00213..f4d0df4b3 100644 --- a/launcher/launcher/main.go +++ b/launcher/launcher/main.go @@ -97,18 +97,23 @@ func main() { } if launchSpec.MonitoringEnabled != spec.None { - logger.Printf("Monitoring is enabled by the VM operator") + logger.Printf("Health Monitoring is enabled by the VM operator") if launchSpec.MonitoringEnabled == spec.All { + logger.Printf("All health monitoring metrics enabled") if err := nodeproblemdetector.EnableAllConfig(); err != nil { - logger.Printf("failed to enable Health Monitoring config: %v", err) + logger.Printf("failed to enable full monitoring config: %v", err) return } + } else if launchSpec.MonitoringEnabled == spec.MemoryOnly { + logger.Printf("memory/bytes_used enabled") } if err := nodeproblemdetector.StartService(logger); err != nil { logger.Print(err) } + } else { + logger.Printf("Health Monitoring is disabled") } defer func() { From e5a8398276fd63d66c6226fc32afb80508955ca7 Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Thu, 10 Oct 2024 20:24:23 +0000 Subject: [PATCH 25/27] fix launchpolicy tests --- launcher/spec/launch_policy.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index 6af7dee5a..2dbc2dc41 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -119,15 +119,17 @@ func configureMonitoringPolicy(imageLabels map[string]string, launchPolicy *Laun switch policy { case always: - logger.Printf("%s=always, will be treated as %s=memory_only and %s=memory_only", memoryMonitoring, hardenedMonitoring, debugMonitoring) + logger.Printf("%s=always will be treated as %s=memory_only and %s=memory_only", memoryMonitoring, hardenedMonitoring, debugMonitoring) launchPolicy.HardenedImageMonitoring = MemoryOnly launchPolicy.DebugImageMonitoring = MemoryOnly case never: - logger.Printf("%s=never, will be treated as %s=none and %s=none", memoryMonitoring, hardenedMonitoring, debugMonitoring) + logger.Printf("%s=never will be treated as %s=none and %s=none", memoryMonitoring, hardenedMonitoring, debugMonitoring) + logger.Printf("memory monitoring not allowed by image") launchPolicy.HardenedImageMonitoring = None launchPolicy.DebugImageMonitoring = None case debugOnly: - logger.Printf("%s=debug_only, will be treated as %s=none and %s=memory", memoryMonitoring, hardenedMonitoring, debugMonitoring) + logger.Printf("%s=debug_only will be treated as %s=none and %s=memory", memoryMonitoring, hardenedMonitoring, debugMonitoring) + logger.Printf("memory monitoring only allowed on debug environment by image") launchPolicy.HardenedImageMonitoring = None launchPolicy.DebugImageMonitoring = MemoryOnly } From 176775e4387ad1e300f1293b6d241333de67237a Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Mon, 14 Oct 2024 21:07:59 +0000 Subject: [PATCH 26/27] address comments --- .../nodeproblemdetector/systemstats_config.go | 1 - launcher/spec/launch_policy.go | 18 ++++++---- launcher/spec/launch_spec.go | 34 ++++++++++++++----- 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go index 7c97c967b..d32027932 100644 --- a/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go +++ b/launcher/internal/healthmonitoring/nodeproblemdetector/systemstats_config.go @@ -32,7 +32,6 @@ type diskConfig struct { // SystemStatsConfig contains configurations for `System Stats Monitor`, // a problem daemon in node-problem-detector that collects pre-defined health-related metrics from different system components. -// For now we only consider collecting memory related metrics. // View the comprehensive configuration details on https://github.com/kubernetes/node-problem-detector/tree/master/pkg/systemstatsmonitor#detailed-configuration-options type SystemStatsConfig struct { CPU *statsConfig `json:"cpu,omitempty"` diff --git a/launcher/spec/launch_policy.go b/launcher/spec/launch_policy.go index 2dbc2dc41..56117cbe8 100644 --- a/launcher/spec/launch_policy.go +++ b/launcher/spec/launch_policy.go @@ -206,15 +206,19 @@ func GetLaunchPolicy(imageLabels map[string]string, logger *log.Logger) (LaunchP } func verifyMonitoringConfig(policy MonitoringType, spec MonitoringType) error { - if policy == None { - if spec != None { - return fmt.Errorf("spec configured for %v but policy is none", spec) - } - + switch policy { + case All: + // If policy is 'All', spec can be anything. return nil - } else if policy == MemoryOnly { + case MemoryOnly: + // If policy is 'MemoryOnly', spec must be 'None' or 'MemoryOnly'. if spec == All { - return fmt.Errorf("spec configured all monitoring, policy only allows memory") + return fmt.Errorf("spec configured for all monitoring, policy only allows memory") + } + case None: + // If policy is 'None', spec must also be 'None'. + if spec != None { + return fmt.Errorf("spec configured for %v but policy is none", spec) } } diff --git a/launcher/spec/launch_spec.go b/launcher/spec/launch_spec.go index 69a8eac93..c3580cea8 100644 --- a/launcher/spec/launch_spec.go +++ b/launcher/spec/launch_spec.go @@ -160,17 +160,33 @@ func (s *LaunchSpec) UnmarshalJSON(b []byte) error { if memOk && monOk { return fmt.Errorf("both %v and %v are specified, only one is permitted", memoryMonitoringEnable, monitoringEnable) - } else if memOk && memVal != "" { - if boolValue, err := strconv.ParseBool(memVal); err == nil && boolValue { - s.MonitoringEnabled = MemoryOnly - } else { + } else if memOk { + // If value is empty, treat as the default. + if memVal == "" { s.MonitoringEnabled = None + } else { + boolValue, err := strconv.ParseBool(memVal) + if err != nil { + return fmt.Errorf("invalid value for %v (not a boolean): %v", memoryMonitoringEnable, err) + } + + if boolValue { + s.MonitoringEnabled = MemoryOnly + } else { + s.MonitoringEnabled = None + } } - } else if monOk && monVal != "" { - var err error - s.MonitoringEnabled, err = toMonitoringType(monVal) - if err != nil { - return err + } else if monOk { + // If value is empty, treat as the default. + if monVal == "" { + s.MonitoringEnabled = None + } else { + + var err error + s.MonitoringEnabled, err = toMonitoringType(monVal) + if err != nil { + return err + } } } From e0f1601590fce8638bb9c837d7ec3dc41b93930d Mon Sep 17 00:00:00 2001 From: Jessie Liu Date: Tue, 15 Oct 2024 21:59:50 +0000 Subject: [PATCH 27/27] capitalize logging string --- launcher/launcher/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/launcher/launcher/main.go b/launcher/launcher/main.go index f4d0df4b3..6be3edbc1 100644 --- a/launcher/launcher/main.go +++ b/launcher/launcher/main.go @@ -102,7 +102,7 @@ func main() { if launchSpec.MonitoringEnabled == spec.All { logger.Printf("All health monitoring metrics enabled") if err := nodeproblemdetector.EnableAllConfig(); err != nil { - logger.Printf("failed to enable full monitoring config: %v", err) + logger.Printf("Failed to enable full monitoring config: %v", err) return } } else if launchSpec.MonitoringEnabled == spec.MemoryOnly {