Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log warning messages for an invalid service connect config #3412

Closed
wants to merge 1 commit into from
Closed

Log warning messages for an invalid service connect config #3412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions agent/api/serviceconnect/service_connect_validator.go
View file
Open in desktop
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{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of changing all errors to warning, could we simply remove the condition for DNS config to be mandatory? Rest of the validation looks correct to me.

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
Open in desktop
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)
})
}
}
46 changes: 20 additions & 26 deletions agent/api/task/task_attachment_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,34 +94,18 @@ func TestNewAttachmentHandlers(t *testing.T) {

func TestHandleTaskAttachmentsWithServiceConnectAttachment(t *testing.T) {
tt := []struct {
testName string
testServiceConnectConfig string
shouldReturnError bool
testName string
testContainerName string
shouldReturnError bool
}{
{
testName: "AWSVPC IPv6 enabled without error",
testServiceConnectConfig: constructTestServiceConnectConfig(
testIngressPort,
testInboundListener,
testOutboundListener,
testIPv4CIDR,
testIPv6CIDR,
testHost,
testIPv6,
),
testName: "AWSVPC IPv6 enabled without error",
testContainerName: testSCContainerName,
shouldReturnError: false,
},
{
testName: "AWSVPC IPv6 enabled with error",
testServiceConnectConfig: constructTestServiceConnectConfig(
testIngressPort,
testInboundListener,
testOutboundListener,
"",
testIPv6CIDR,
testHost,
testIPv6,
),
testName: "AWSVPC IPv6 enabled with error",
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