From c2feaf67d6a9bd41aa2ecd880259729388f53dac Mon Sep 17 00:00:00 2001 From: Chien-Han Lin Date: Sun, 9 Oct 2022 19:34:07 -0700 Subject: [PATCH] Update service connect config validator to validate: 1. fields consumed and proceeded by ECS Agent 2. fields with a global standard --- .../service_connect_validator.go | 103 ++++++++---------- .../service_connect_validator_test.go | 42 +------ .../api/task/task_attachment_handler_test.go | 2 +- 3 files changed, 48 insertions(+), 99 deletions(-) diff --git a/agent/api/serviceconnect/service_connect_validator.go b/agent/api/serviceconnect/service_connect_validator.go index af158476f47..c8e7425f8b0 100644 --- a/agent/api/serviceconnect/service_connect_validator.go +++ b/agent/api/serviceconnect/service_connect_validator.go @@ -24,40 +24,47 @@ import ( ) const ( - BridgeNetworkMode = "bridge" - AWSVPCNetworkMode = "awsvpc" - 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` - invalidDnsEntryFormat = `no %s in the DNS config hostname=%s, address=%s` + BridgeNetworkMode = "bridge" + AWSVPCNetworkMode = "awsvpc" + missingContainerInTaskFormat = `service connect container name=%s does not exist in the task` + duplicateContainerInTaskFormat = `found %d duplicate service connect container name=%s in the task` + invalidCidrFormat = `CIDR=%s is not a valid %s CIDR` + portCollisionFormat = `%s port collision detected in the ingress config with %s port=%d, and listener name=%s` + invalidDnsAddressFormat = `hostname=%s, address=%s in the DNS config is not valid: %w` + noScSupportNetworkModeFormat = `service connect does not support for %s newtork mode` + missingListenerInIngressFormat = `missing listener name in the ingress config with intercept port=%d` + invalidPortRangeFormat = `port=%d is not a valid port. A valid port ranges from 1 through 65535` + invalidIpAddressFormat = `address=%s is not a valid IP address` + invalidIngressPortFormat = `%s port=%d in the ingress config is not valid: %w` + warningIngressPortFormat = `Service connect config: %s port should not exist in the ingress config for %s network mode` + missingDnsEntryFormat = `missing %s in the DNS config with hostname=%s, and address=%s` ) -// validateContainerName validates the service connect container name. +// validateContainerName validates the service connect container name exists in the task and no duplication. func validateContainerName(scContainerName string, taskContainers []*ecsacs.Container) error { // service connect container name is required if scContainerName == "" { return fmt.Errorf("missing service connect container name") } - // validate the specified service connect container name exists in the task definition - numOfFoundSCContainer := 0 + // validate the specified service connect container name exists in the task + numOfFoundScContainer := 0 for _, container := range taskContainers { if aws.StringValue(container.Name) == scContainerName { - numOfFoundSCContainer += 1 + numOfFoundScContainer += 1 } } - if numOfFoundSCContainer == 0 { - return fmt.Errorf("service connect container name=%s does not exist in the task", scContainerName) - } else if numOfFoundSCContainer > 1 { - return fmt.Errorf("found %d duplicate service connect container name=%s exist in the task", numOfFoundSCContainer, scContainerName) + if numOfFoundScContainer == 0 { + return fmt.Errorf(missingContainerInTaskFormat, scContainerName) + } else if numOfFoundScContainer > 1 { + return fmt.Errorf(duplicateContainerInTaskFormat, numOfFoundScContainer, scContainerName) } return nil } -// validateEgressConfig validates the service connect egress config. +// validateEgressConfig validates the listener name, IPv4 CIDR format and IPv6 CIDR format in the service connect egress config. func validateEgressConfig(scEgressConfig *EgressConfig, ipv6Enabled bool) error { // egress config can be empty for the first service since there are no other tasks that it can talk to if scEgressConfig == nil { @@ -66,32 +73,21 @@ func validateEgressConfig(scEgressConfig *EgressConfig, ipv6Enabled bool) error // ListenerName is required if the egress config exists if scEgressConfig.ListenerName == "" { - return fmt.Errorf(invalidEgressConfigFormat, "listener name", "") - } - - // VIP is required if the egress config exists - // IPV4CIDR should be always required because an IPv6-only mode is not supoorted at this moment - if scEgressConfig.VIP.IPV4CIDR == "" { - return fmt.Errorf(invalidEgressConfigFormat, "VIP IPv4CIDR", "") - } - - // IPV6CIDR is required when IPv6 is enabled - if ipv6Enabled && scEgressConfig.VIP.IPV6CIDR == "" { - return fmt.Errorf(invalidEgressConfigFormat, "VIP IPv6CIDR", "It must not be empty when the task is IPv6 enabled") + return fmt.Errorf("missing listener name in the egress config") } // validate IPV4CIDR if it exists if scEgressConfig.VIP.IPV4CIDR != "" { - trimmedIpv4cidr := strings.TrimSpace(scEgressConfig.VIP.IPV4CIDR) - if err := validateCIDR(trimmedIpv4cidr, "IPv4"); err != nil { + trimmedIpv4Cidr := strings.TrimSpace(scEgressConfig.VIP.IPV4CIDR) + if err := validateCIDR(trimmedIpv4Cidr, "IPv4"); err != nil { return err } } // validate IPV6CIDR if it exists if scEgressConfig.VIP.IPV6CIDR != "" { - trimmedIpv6cidr := strings.TrimSpace(scEgressConfig.VIP.IPV6CIDR) - if err := validateCIDR(trimmedIpv6cidr, "IPv6"); err != nil { + trimmedIpv6Cidr := strings.TrimSpace(scEgressConfig.VIP.IPV6CIDR) + if err := validateCIDR(trimmedIpv6Cidr, "IPv6"); err != nil { return err } } @@ -108,7 +104,7 @@ func validateCIDR(cidr, protocol string) error { } } - return fmt.Errorf("cidr=%s is not a valid %s CIDR", cidr, protocol) + return fmt.Errorf(invalidCidrFormat, cidr, protocol) } // getProtocol returns validity of the given IP based on the target protocol. @@ -128,27 +124,22 @@ func getProtocol(ip net.IP, protocol string) bool { return false } -// validateDnsConfig validates the service connnect DNS config. -func validateDnsConfig(scDnsConfligList []DNSConfigEntry, scEgressConfig *EgressConfig, ipv6Enabled bool) error { - // DNS config associates to egress config - if len(scDnsConfligList) == 0 && scEgressConfig != nil { - return fmt.Errorf("no service connect DNS config. The DNS config is required when the egress config exists") - } - +// validateDnsConfig validates hostnames and addresses in the service connnect DNS config. +func validateDnsConfig(scDnsConfligList []DNSConfigEntry) error { for _, dnsEntry := range scDnsConfligList { // HostName is required if dnsEntry.HostName == "" { - return fmt.Errorf(invalidDnsEntryFormat, "hostname", dnsEntry.HostName, dnsEntry.Address) + return fmt.Errorf(missingDnsEntryFormat, "hostname", dnsEntry.HostName, dnsEntry.Address) } // Address is required if dnsEntry.Address == "" { - return fmt.Errorf(invalidDnsEntryFormat, "address", dnsEntry.HostName, dnsEntry.Address) + return fmt.Errorf(missingDnsEntryFormat, "address", dnsEntry.HostName, dnsEntry.Address) } // validate the address is a valid IPv4/IPv6 address if err := validateAddress(dnsEntry.Address); err != nil { - return fmt.Errorf("invalid address in the DNS config hostname=%s, address=%s: %w", dnsEntry.HostName, dnsEntry.Address, err) + return fmt.Errorf(invalidDnsAddressFormat, dnsEntry.HostName, dnsEntry.Address, err) } } @@ -158,7 +149,7 @@ func validateDnsConfig(scDnsConfligList []DNSConfigEntry, scEgressConfig *Egress // validateAddress validates the passed address is a valid IPv4/IPv6 address. func validateAddress(address string) error { if ip := net.ParseIP(address); ip == nil { - return fmt.Errorf("address=%s is not a valid IP address", address) + return fmt.Errorf(invalidIpAddressFormat, address) } return nil } @@ -176,7 +167,7 @@ func validateIngressConfig(scIngressConfigList []IngressConfigEntry, taskNetwork return err } default: - return fmt.Errorf("service connect does not support for %s newtork mode", taskNetworkMode) + return fmt.Errorf(noScSupportNetworkModeFormat, taskNetworkMode) } return nil @@ -215,7 +206,7 @@ func validateIngressConfigEntry(scIngressConfigList []IngressConfigEntry, networ if err := validateInterceptPort(interceptPortValue, entry.ListenerName, interceptAndListenerPortsMap); err != nil { return err } - // Save the listener port value + // save the listener port value interceptAndListenerPortsMap[interceptPortValue] = true } @@ -225,7 +216,7 @@ func validateIngressConfigEntry(scIngressConfigList []IngressConfigEntry, networ if err := validateListenerPort(listenerPortValue, entry.ListenerName, interceptAndListenerPortsMap); err != nil { return err } - // Save the listener port value + // save the listener port value interceptAndListenerPortsMap[listenerPortValue] = true } @@ -235,7 +226,7 @@ func validateIngressConfigEntry(scIngressConfigList []IngressConfigEntry, networ if err := validateHostPort(hostPortValue, entry.ListenerName, hostPortsMap); err != nil { return err } - // Save the host port value + // save the host port value hostPortsMap[hostPortValue] = true } } @@ -250,7 +241,7 @@ func validateInterceptPort(interceptPortValue uint16, listenerName string, inter } if listenerName == "" { - return fmt.Errorf("no listener name in the ingress config with the intercept port=%d", interceptPortValue) + return fmt.Errorf(missingListenerInIngressFormat, interceptPortValue) } if present := interceptAndListenerPortsMap[interceptPortValue]; present { @@ -293,10 +284,13 @@ func validatePort(port uint16) error { return nil } - return fmt.Errorf("the port=%d is an invalid port. A valid port ranges from 1 through 65535", port) + return fmt.Errorf(invalidPortRangeFormat, port) } -// ValidateSCConfig validates service connect container name, config, egress config, and ingress config. +// ValidateServiceConnectConfig validates service connect container name, +// fields in egress config, dns config and ingress config when +// 1) fields consumed and proceeded by ECS Agent +// 2) fields with a global standard, e.g. CIDR format func ValidateServiceConnectConfig(scConfig *Config, taskContainers []*ecsacs.Container, taskNetworkMode string, @@ -305,16 +299,11 @@ func ValidateServiceConnectConfig(scConfig *Config, return err } - // 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") - } - if err := validateEgressConfig(scConfig.EgressConfig, ipv6Enabled); err != nil { return err } - if err := validateDnsConfig(scConfig.DNSConfig, scConfig.EgressConfig, ipv6Enabled); err != nil { + if err := validateDnsConfig(scConfig.DNSConfig); err != nil { return err } diff --git a/agent/api/serviceconnect/service_connect_validator_test.go b/agent/api/serviceconnect/service_connect_validator_test.go index 821bb8588be..ba4e2b7fc82 100644 --- a/agent/api/serviceconnect/service_connect_validator_test.go +++ b/agent/api/serviceconnect/service_connect_validator_test.go @@ -253,25 +253,16 @@ 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 +289,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) }) } } @@ -344,24 +331,6 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv4Address), testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, testInboundListenerName, false, uint16(0), testAwsVpcDefaultInterceptPort, aws.Uint16(0)), }, - { - testName: "AWSVPC default case with no IPv4 CIDR in the egress config when Ipv6 is not enabled", - testNetworkMode: AWSVPCNetworkMode, - testIsIPv6Enabled: false, - testContainerName: testServiceConnectContainerName, - testEgressConfig: getTestEgressConfig(testOutboundListenerName, "", ""), - testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv4Address), - testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, testInboundListenerName, false, uint16(0), testAwsVpcDefaultInterceptPort, aws.Uint16(0)), - }, - { - testName: "AWSVPC default case with no IPv6 CIDR in the egress config when IPv6 is enabled", - testNetworkMode: AWSVPCNetworkMode, - testIsIPv6Enabled: true, - testContainerName: testServiceConnectContainerName, - testEgressConfig: getTestEgressConfig(testOutboundListenerName, testIPv4Cidr, ""), - testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv6Address), - testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, testInboundListenerName, false, uint16(0), testAwsVpcDefaultInterceptPort, aws.Uint16(0)), - }, { testName: "AWSVPC override case with the invalid IPv4 CIDR in the egress config", testNetworkMode: AWSVPCNetworkMode, @@ -380,15 +349,6 @@ func TestValidateServiceConnectConfigWithError(t *testing.T) { testDnsConfigEntry: getTestDnsConfigEntry(testHostName, testIPv6Address), testIngressConfigEntry: getTestIngressConfigEntry(AWSVPCNetworkMode, "", true, testListenerPort, aws.Uint16(0), aws.Uint16(0)), }, - { - testName: "Bridge default case with the egress config but no dns config", - testNetworkMode: BridgeNetworkMode, - testIsIPv6Enabled: false, - testContainerName: testServiceConnectContainerName, - testEgressConfig: getTestEgressConfig(testOutboundListenerName, "", testIPv6Cidr), - testDnsConfigEntry: DNSConfigEntry{}, - testIngressConfigEntry: getTestIngressConfigEntry(BridgeNetworkMode, "", false, testBridgeDefaultListenerPort, aws.Uint16(0), aws.Uint16(0)), - }, { testName: "Bridge override case with no the invalid address in dns config when IPv6 is enabled", testNetworkMode: BridgeNetworkMode, diff --git a/agent/api/task/task_attachment_handler_test.go b/agent/api/task/task_attachment_handler_test.go index fccc2358159..00cbf1d6dfd 100644 --- a/agent/api/task/task_attachment_handler_test.go +++ b/agent/api/task/task_attachment_handler_test.go @@ -115,7 +115,7 @@ func TestHandleTaskAttachmentsWithServiceConnectAttachment(t *testing.T) { testName: "AWSVPC IPv6 enabled with error", testServiceConnectConfig: constructTestServiceConnectConfig( testIngressPort, - testInboundListener, + "", testOutboundListener, "", testIPv6CIDR,