From 7c69df02814edd6b0bbf12f1cb3cb81aaa5e4785 Mon Sep 17 00:00:00 2001 From: Chien-Han Lin Date: Fri, 30 Sep 2022 09:11:45 -0700 Subject: [PATCH] Log warning messages for invalid egress, ingress and dns configs --- .../service_connect_validator.go | 27 ++++++++++--- .../service_connect_validator_test.go | 33 ++++++++++------ .../api/task/task_attachment_handler_test.go | 38 ++++++++----------- 3 files changed, 59 insertions(+), 39 deletions(-) diff --git a/agent/api/serviceconnect/service_connect_validator.go b/agent/api/serviceconnect/service_connect_validator.go index af158476f47..99fb4e5db14 100644 --- a/agent/api/serviceconnect/service_connect_validator.go +++ b/agent/api/serviceconnect/service_connect_validator.go @@ -20,6 +20,7 @@ import ( "github.com/aws/amazon-ecs-agent/agent/acs/model/ecsacs" "github.com/aws/amazon-ecs-agent/agent/logger" + "github.com/aws/amazon-ecs-agent/agent/logger/field" "github.com/aws/aws-sdk-go/aws" ) @@ -29,7 +30,7 @@ const ( invalidEgressConfigFormat = `no service connect %s in the egress config. %s` portCollisionFormat = `%s port collision detected in the ingress config with the %s port=%d, and listener name=%s` invalidIngressPortFormat = `the %s port=%d in the ingress config is not valid: %w` - warningIngressPortFormat = `Service connect config validation: %s port should not exist for %s mode in the ingress config` + warningIngressPortFormat = `%s port should not exist for %s mode in the ingress config` invalidDnsEntryFormat = `no %s in the DNS config hostname=%s, address=%s` ) @@ -297,6 +298,8 @@ func validatePort(port uint16) error { } // ValidateSCConfig validates service connect container name, config, egress config, and ingress config. +// Only log wanring messages instead of returning an error when ingress, egress and dns configs validations failed +// as the ACS payload from ECS backend is single source of truth to Agent. func ValidateServiceConnectConfig(scConfig *Config, taskContainers []*ecsacs.Container, taskNetworkMode string, @@ -307,19 +310,33 @@ func ValidateServiceConnectConfig(scConfig *Config, // egress config and ingress config should not both be nil/empty if scConfig.EgressConfig == nil && len(scConfig.IngressConfig) == 0 { - return fmt.Errorf("egress config and ingress config should not both be nil/empty") + logger.Warn("Service connect egress config is nil and ingress config is empty. Will proceed anyway.", logger.Fields{ + field.Container: scConfig.ContainerName, + }) } if err := validateEgressConfig(scConfig.EgressConfig, ipv6Enabled); err != nil { - return err + logger.Warn("Service connect egress config validation warnings. Will proceed anyway.", logger.Fields{ + field.Container: scConfig.ContainerName, + "IPv6Enabled": ipv6Enabled, + field.Error: err, + }) } if err := validateDnsConfig(scConfig.DNSConfig, scConfig.EgressConfig, ipv6Enabled); err != nil { - return err + logger.Warn("Service connect DNS config validation warnings. Will proceed anyway.", logger.Fields{ + field.Container: scConfig.ContainerName, + "IPv6Enabled": ipv6Enabled, + field.Error: err, + }) } if err := validateIngressConfig(scConfig.IngressConfig, taskNetworkMode); err != nil { - return err + logger.Warn("Service connect ingress config validation warnings. Will proceed anyway.", logger.Fields{ + field.Container: scConfig.ContainerName, + field.NetworkMode: taskNetworkMode, + field.Error: err, + }) } return nil diff --git a/agent/api/serviceconnect/service_connect_validator_test.go b/agent/api/serviceconnect/service_connect_validator_test.go index 821bb8588be..63867305b97 100644 --- a/agent/api/serviceconnect/service_connect_validator_test.go +++ b/agent/api/serviceconnect/service_connect_validator_test.go @@ -199,7 +199,7 @@ func TestValidateServiceConnectConfig(t *testing.T) { } } -func TestValidateServiceConnectConfigWithWarning(t *testing.T) { +func TestValidateServiceConnectEgressIngressConfigWithWarning(t *testing.T) { tt := []struct { testName string testNetworkMode string @@ -240,6 +240,7 @@ func TestValidateServiceConnectConfigWithWarning(t *testing.T) { ingressConfig, ) err := ValidateServiceConnectConfig(testServiceConnectConfig, testTaskContainers, tc.testNetworkMode, false) + // No error as we only log wanring messages when ingress, egress and dns configs validations failed assert.NoError(t, err) }) } @@ -253,25 +254,21 @@ func TestValidateServiceConnectConfigWithEmptyConfig(t *testing.T) { testName string testEgressConfigIsEmpty bool testIngressConfigIsEmpty bool - shouldError bool }{ { testName: "AWSVPC default case with the empty egress config and dns config", testEgressConfigIsEmpty: true, testIngressConfigIsEmpty: false, - shouldError: false, }, { testName: "AWSVPC default case with the empty ingress config", testEgressConfigIsEmpty: false, testIngressConfigIsEmpty: true, - shouldError: false, }, { testName: "AWSVPC default case with the empty ingress config and engress config", testEgressConfigIsEmpty: true, testIngressConfigIsEmpty: true, - shouldError: true, }, } @@ -298,11 +295,7 @@ func TestValidateServiceConnectConfigWithEmptyConfig(t *testing.T) { testIngressConfig, ) err := ValidateServiceConnectConfig(testServiceConnectConfig, testTaskContainers, AWSVPCNetworkMode, false) - if tc.shouldError { - assert.Error(t, err) - } else { - assert.NoError(t, err) - } + assert.NoError(t, err) }) } } @@ -316,6 +309,7 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testEgressConfig *EgressConfig testDnsConfigEntry DNSConfigEntry testIngressConfigEntry IngressConfigEntry + shouldError bool }{ { testName: "AWSVPC default case with no service connect container name", @@ -325,6 +319,7 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testEgressConfig: getTestEgressConfig(testOutboundListenerName, testIPv4Cidr, ""), testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv4Address), testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, testInboundListenerName, false, uint16(0), testAwsVpcDefaultInterceptPort, aws.Uint16(0)), + shouldError: true, }, { testName: "AWSVPC default case with the service connect container name not exists in the task", @@ -334,6 +329,7 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testEgressConfig: getTestEgressConfig(testOutboundListenerName, testIPv4Cidr, ""), testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv4Address), testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, testInboundListenerName, false, uint16(0), testAwsVpcDefaultInterceptPort, aws.Uint16(0)), + shouldError: true, }, { testName: "AWSVPC default case with no listener name in the egress config", @@ -343,6 +339,7 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testEgressConfig: getTestEgressConfig("", testIPv4Cidr, ""), testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv4Address), testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, testInboundListenerName, false, uint16(0), testAwsVpcDefaultInterceptPort, aws.Uint16(0)), + shouldError: false, }, { testName: "AWSVPC default case with no IPv4 CIDR in the egress config when Ipv6 is not enabled", @@ -352,6 +349,7 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testEgressConfig: getTestEgressConfig(testOutboundListenerName, "", ""), testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv4Address), testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, testInboundListenerName, false, uint16(0), testAwsVpcDefaultInterceptPort, aws.Uint16(0)), + shouldError: false, }, { testName: "AWSVPC default case with no IPv6 CIDR in the egress config when IPv6 is enabled", @@ -361,6 +359,7 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testEgressConfig: getTestEgressConfig(testOutboundListenerName, testIPv4Cidr, ""), testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv6Address), testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, testInboundListenerName, false, uint16(0), testAwsVpcDefaultInterceptPort, aws.Uint16(0)), + shouldError: false, }, { testName: "AWSVPC override case with the invalid IPv4 CIDR in the egress config", @@ -370,6 +369,7 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testEgressConfig: getTestEgressConfig(testOutboundListenerName, "999999999", ""), testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv4Address), testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, "", true, testListenerPort, aws.Uint16(0), aws.Uint16(0)), + shouldError: false, }, { testName: "AWSVPC override case with the invalid IPv6 CIDR in the egress config when IPv6 is enabled", @@ -379,6 +379,7 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testEgressConfig: getTestEgressConfig(testOutboundListenerName, testIPv4Cidr, "999999999"), testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv6Address), testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, "", true, testListenerPort, aws.Uint16(0), aws.Uint16(0)), + shouldError: false, }, { testName: "Bridge default case with the egress config but no dns config", @@ -388,6 +389,7 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testEgressConfig: getTestEgressConfig(testOutboundListenerName, "", testIPv6Cidr), testDnsConfigEntry: DNSConfigEntry{}, testIngressConfigEntry: getTestIngressConfigEntry(BridgeNetworkMode, "", false, testBridgeDefaultListenerPort, aws.Uint16(0), aws.Uint16(0)), + shouldError: false, }, { testName: "Bridge override case with no the invalid address in dns config when IPv6 is enabled", @@ -397,6 +399,7 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testEgressConfig: getTestEgressConfig(testOutboundListenerName, testIPv4Cidr, testIPv6Cidr), testDnsConfigEntry: getTestDnsConfigEntry(testHostName, "999999999"), testIngressConfigEntry: getTestIngressConfigEntry(BridgeNetworkMode, "", true, testListenerPort, aws.Uint16(0), testBridgeOverrideHostPort), + shouldError: false, }, } @@ -414,7 +417,12 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { ingressConfig, ) err := ValidateServiceConnectConfig(testServiceConnectConfig, testTaskContainers, tc.testNetworkMode, tc.testIsIPv6Enabled) - assert.Error(t, err) + if tc.shouldError { + assert.Error(t, err) + } else { + // No error as we only log wanring messages when ingress, egress and dns configs validations failed + assert.NoError(t, err) + } }) } } @@ -467,7 +475,8 @@ func TestValidateServiceConnectConfigWithPortCollision(t *testing.T) { ingressConfig, ) err := ValidateServiceConnectConfig(testServiceConnectConfig, testTaskContainers, tc.testNetworkMode, false) - assert.Error(t, err) + // No error as we only log wanring messages when ingress, egress and dns configs validations failed + assert.NoError(t, err) }) } } diff --git a/agent/api/task/task_attachment_handler_test.go b/agent/api/task/task_attachment_handler_test.go index fccc2358159..7df7d5c308d 100644 --- a/agent/api/task/task_attachment_handler_test.go +++ b/agent/api/task/task_attachment_handler_test.go @@ -95,33 +95,17 @@ func TestNewAttachmentHandlers(t *testing.T) { func TestHandleTaskAttachmentsWithServiceConnectAttachment(t *testing.T) { tt := []struct { testName string - testServiceConnectConfig string + testContainerName string shouldReturnError bool }{ { testName: "AWSVPC IPv6 enabled without error", - testServiceConnectConfig: constructTestServiceConnectConfig( - testIngressPort, - testInboundListener, - testOutboundListener, - testIPv4CIDR, - testIPv6CIDR, - testHost, - testIPv6, - ), + testContainerName: testSCContainerName, shouldReturnError: false, }, { testName: "AWSVPC IPv6 enabled with error", - testServiceConnectConfig: constructTestServiceConnectConfig( - testIngressPort, - testInboundListener, - testOutboundListener, - "", - testIPv6CIDR, - testHost, - testIPv6, - ), + testContainerName: "", shouldReturnError: true, }, } @@ -149,12 +133,22 @@ func TestHandleTaskAttachmentsWithServiceConnectAttachment(t *testing.T) { }, } + testServiceConnectConfig := constructTestServiceConnectConfig( + testIngressPort, + testInboundListener, + testOutboundListener, + testIPv4CIDR, + testIPv6CIDR, + testHost, + testIPv6, + ) + for _, tc := range tt { t.Run(tc.testName, func(t *testing.T) { testAcsTask := &ecsacs.Task{ ElasticNetworkInterfaces: []*ecsacs.ElasticNetworkInterface{testIpv6ElasticNetworkInterface}, Containers: []*ecsacs.Container{ - getTestcontainerFromACS(testSCContainerName, AWSVPCNetworkMode), + getTestcontainerFromACS(tc., AWSVPCNetworkMode), }, Attachments: []*ecsacs.Attachment{ { @@ -162,11 +156,11 @@ func TestHandleTaskAttachmentsWithServiceConnectAttachment(t *testing.T) { AttachmentProperties: []*ecsacs.AttachmentProperty{ { Name: stringToPointer(serviceconnect.GetServiceConnectConfigKey()), - Value: stringToPointer(tc.testServiceConnectConfig), + Value: stringToPointer(testServiceConnectConfig), }, { Name: stringToPointer(serviceconnect.GetServiceConnectContainerNameKey()), - Value: stringToPointer(testSCContainerName), + Value: stringToPointer(tc.testContainerName), }, }, AttachmentType: stringToPointer(serviceConnectAttachmentType),