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

Refactor ACS attach task ENI message handling #3744

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

danehlim
Copy link
Contributor

@danehlim danehlim commented Jun 9, 2023

Summary

Refactor ACS attach task ENI message handling, update other associated code, and clean up tests.

Implementation details

Please note that, in the context of this PR, "ACS message responder"/"responder" is more or less synonymous with "ACS message handler"/"handler".

  1. Refactor ACS attach task ENI message handling and move to ecs-agent/
    • agent/acs/handler/attach_task_eni_handler.go -> ecs-agent/acs/session/attach_task_eni_responder.go
      • Refactor attachTaskENIHandler struct and existing code into attachTaskENIResponder struct, such that attachTaskENIResponder struct implements the wsclient.RequestResponder interface
      • Handle ENI attachment in async (no reason for this to be a blocking operation)
      • Consider an attach task ENI message with multiple network interface devices to be valid, in the event that ACS does send one
        • A similar change has already been previously done for payload messages (see here), this simply just a followup to that
      • Perform more validation of the ENI(s) in the attach task ENI message than just validating MAC address is present
      • Log with level ERROR instead of WARN when attach task ENI message is invalid
      • Use github.com/aws/amazon-ecs-agent/ecs-agent/logger instead of github.com/cihub/seelog for logging
    • Update tests accordingly based off of above changes
      • agent/acs/handler/attach_task_eni_handler_test.go (partial) -> agent/acs/handler/attach_task_eni_responder_test.go
        • Tests that need to interact with Agent state belong in this file
      • agent/acs/handler/attach_task_eni_handler_test.go (partial) -> ecs-agent/acs/session/attach_task_eni_responder_test.go
        • Tests that need to interact only with stateless components of the responder (e.g., message validation) belong in this file
  2. Update other associated files
    • agent/acs/handler/acs_handler.go
      • Initialize attach task ENI responder and add its HandlerFunc function as a request handler to the ACS client, such that attach task ENI responder dictates what to do in response to receiving attach task ENI messages from ACS
        • Eventually, each ACS message type will have its own responder (which implements the wsclient.RequestResponder interface) that dictates what to do in response to ACS messages of that specific type
  3. General cleanup of tests, most notably:
    • Increase test coverage
    • Eliminate redundant initialization of test attachTaskENIMessage variables for each attach task ENI responder test that requires one
    • Extract some testing constants commonly used across multiple ACS responders into centralized file
      • Only constants that were used in both tests for attach task/instance ENI responders as well as tests for other responder(s) were extracted into ecs-agent/acs/session/testconst/test_const.go
      • Not in the scope of this pull request: moving all ACS responders' common testing constants to the centralized file and refactoring all ACS responders testing code to use testing constants instead of hard-coded values (when applicable). More testing constants will be moved to the centralized file as we continue to refactor other ACS responders in future pull requests

Testing

Unit, integration, and functional tests.

New tests cover the changes: yes

Description for the changelog

Refactor ACS attach task ENI message handling

Licensing

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

@danehlim danehlim marked this pull request as ready for review June 9, 2023 23:03
@danehlim danehlim requested a review from a team as a code owner June 9, 2023 23:03
@danehlim danehlim merged commit 0de7229 into aws:dev Jun 19, 2023
6 checks passed
@prateekchaudhry prateekchaudhry mentioned this pull request Jun 22, 2023
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.

4 participants