Skip to content

Commit

Permalink
Log warning messages for invalid egress, ingress and dns configs
Browse files Browse the repository at this point in the history
  • Loading branch information
@chienhanlin
chienhanlin committed Sep 30, 2022
1 parent 99eda24 commit b3675d9
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 39 deletions.
27 changes: 22 additions & 5 deletions agent/api/serviceconnect/service_connect_validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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`
)

Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down
33 changes: 21 additions & 12 deletions agent/api/serviceconnect/service_connect_validator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
})
}
Expand All @@ -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,
},
}

Expand All @@ -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)
})
}
}
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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,
},
}

Expand All @@ -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)
}
})
}
}
Expand Down Expand Up @@ -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)
})
}
}
38 changes: 16 additions & 22 deletions agent/api/task/task_attachment_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}
Expand Down Expand Up @@ -149,24 +133,34 @@ 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.testContainerName, AWSVPCNetworkMode),
},
Attachments: []*ecsacs.Attachment{
{
AttachmentArn: stringToPointer("attachmentArn"),
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),
Expand Down

0 comments on commit b3675d9

Please sign in to comment.