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.

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

Conversation

chienhanlin
Copy link
Contributor

@chienhanlin chienhanlin commented Sep 30, 2022

Summary

As service connect config will be validated in ECS control plane before streaming down to Agent, Agent will only log validation warnings if there is any, and proceed the workflow. This PR makes changes on ValidateServiceConnectConfig() to log warning messages when egress config, dns config, and ingress config not passing validations at Agent side.

Implementation details

  1. service_connect_validator.go
  • Replace return error with logger.Warn() in ValidateServiceConnectConfig()
  1. service_connect_validator_test.go
  • Rename "TestValidateServiceConnectConfigWithWarning" to "TestValidateServiceConnectEgressIngressConfigWithWarning"
  • Update "TestValidateServiceConnectConfigWithEmptyConfig", "TestValidateServiceConnectConfigWithError" and "TestValidateServiceConnectConfigWithPortCollision" as we only log wanring messages when ingress, egress and dns configs validations failed at Agent side
  1. task_attachment_handler_test.go
  • Remove "AWSVPC IPv6 enabled with error" test case as only warning message will be logged after this PR

Testing

  1. New tests cover the changes: no
  2. E2E testing by modifying the return value from parseAttachment for following test cases:
  • Testing: Set IngressConfig to an empty list, and EgressConfig to nill for testing
level=warn time=xxx msg="Service connect egress config is nil and ingress config is empty. Will proceed anyway." container="ecs-service-connect-xxx"
level=info time=xxx msg="Received task payload from ACS" taskARN="xxx" version="1" desiredStatus=RUNNING
...
level=error time=xxx msg="Unable to configure pause container namespace" task="xxx" error="add network failed: either IngressConfig or EgressConfig must be present"
...
  • Testing: Set EgressConfig with an empty ListenerName and an empty DNSConfig for testing
level=warn time=xxx msg="Service connect egress config validation warnings. Will proceed anyway." error="no service connect listener name in the egress config. " container="ecs-service-connect-xxx" IPv6Enabled=false
level=warn time=xxx msg="Service connect DNS config validation warnings. Will proceed anyway." container="ecs-service-connect-d8547i" IPv6Enabled=false error="no service connect DNS config. The DNS config is required when the egress config exists"
level=info time=xxx msg="Received task payload from ACS" desiredStatus=RUNNING taskARN="xxx" version="1"
...
level=error time=2022-10-02T23:58:05Z msg="Unable to configure pause container namespace" task="xxx" error="add network failed: missing required parameter: EgressConfig VIP CIDR"
  • Testing: Set EgressConfig with invalid IPV4CIDR and IPV6CIDR, and DNSConfig with an invalid Address for testing
level=warn time=xxx msg="Service connect egress config validation warnings. Will proceed anyway." container="ecs-service-connect-xxx" IPv6Enabled=false error="cidr=testIPv4CIDR is not a valid IPv4 CIDR"
level=warn time=xxx msg="Service connect DNS config validation warnings. Will proceed anyway." IPv6Enabled=false error="invalid address in the DNS config hostname=testHost, address=testIPv6: address=testIPv6 is not a valid IP address" container="ecs-service-connect-xxx"
level=info time=xxx msg="Received task payload from ACS" taskARN="xxx" version="1" desiredStatus=RUNNING
  • Testing: Set IngressConfig with an invalid ListenerPort for testing
level=warn time=xxx msg="a host port should not exist for awsvpc mode in the ingress config" listenerName="testListener" listenerPort=8080 hostPort=8080 interceptPort=8080
level=warn time=xxx msg="Service connect ingress config validation warnings. Will proceed anyway." container="ecs-service-connect-Za7i0j" networkMode="awsvpc" error="listener port collision detected in the ingress config with the listener port=8080, and listener name=testListener"
level=info time=xxx msg="Received task payload from ACS" taskARN="xxx" version="1" desiredStatus=RUNNING
  1. E2E testing by running a client-only service in a namespace
  • Before this PR, task will stopped with reason "UnrecogniedTaskError: Error loading task - service connect config validation failed: no service connect DNS config. The DNS config is required when the egress config exists"
  • After replacing ECS Agent on the container instance with the artifact built from this PR, task will continue reaching to RUNNING state, and the warning message will be logged in ecs-agent.log
level=info time=xxx msg="Task network mode initialized" task="xxx" networkMode="awsvpc"
level=warn time=xxx msg="Received an unrecognized attachment property" attachmentProperty="{\n  Name: \"EcsTaskSetArn\",\n  Value: \"axxx\"\n}"
level=warn time=xxx msg="Service connect DNS config validation warnings. Will proceed anyway." error="no service connect DNS config. The DNS config is required when the egress config exists" container="ecs-service-connect-mPXLo7S" IPv6Enabled=false
level=info time=xxx msg="Received task payload from ACS" taskARN="axxx" version="1" desiredStatus=RUNNING

Description for the changelog

Log warning messages for invalid service connect configs

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@chienhanlin chienhanlin changed the title Log warning messages for an invalid service connect config [WIP] Log warning messages for an invalid service connect config Sep 30, 2022
@chienhanlin chienhanlin force-pushed the sc_config_warning_msg branch 2 times, most recently from b3675d9 to 3e56ab6 Compare September 30, 2022 20:39
@yinyic yinyic force-pushed the feature/service-connect branch from 99eda24 to 76d6899 Compare October 4, 2022 00:01
@chienhanlin chienhanlin force-pushed the sc_config_warning_msg branch from 3e56ab6 to 79bb726 Compare October 4, 2022 00:30
@chienhanlin chienhanlin changed the title [WIP] Log warning messages for an invalid service connect config Log warning messages for an invalid service connect config Oct 4, 2022
}

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{

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.

@chienhanlin
Copy link
Contributor Author

Closing this PR and opening a new PR #3424 to

  1. Update service connect config validator to validate fields with a global standard, or consumed and proceeded by ECS Agent
  2. Change the destination branch to dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants