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 refresh credentials message handling #3830

Merged
merged 1 commit into from Jul 29, 2023

Conversation

danehlim
Copy link
Contributor

@danehlim danehlim commented Jul 26, 2023

Summary

Refactor ACS refresh credentials message handling, update other associated code, and improve/update 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 refresh credentials message handling
    • agent/acs/handler/refresh_credentials_handler.go -> ecs-agent/acs/session/refresh_credentials_responder.go (partial)
      • All components of the responder that do not directly interact with Agent state belong in this file
      • Refactor refreshCredentialsHandler struct and existing code into refreshCredentialsResponder struct, such that refreshCredentialsResponder struct implements the wsclient.RequestResponder interface
      • Move validation of the refresh credentials message's role type to the validateIAMRoleCredentialsMessage function
      • Use github.com/aws/amazon-ecs-agent/ecs-agent/logger instead of github.com/cihub/seelog for logging
      • Add support for publishing of metrics in various credentials refresh failure/success cases
        • NOTE: Our Agent defined in agent/ directory does not support metrics publishing, thus in agent/acs/handler/acs_handler.go our agent will supply the refresh credentials responder with a metrics.NewNopEntryFactory(), which is a no-op implementation of metrics publishing
    • agent/acs/handler/refresh_credentials_handler.go -> agent/acs/handler/refresh_credentials_responder.go (partial)
      • Components that interact with Agent state belong in this file
      • Refactor functionality from such components mentioned above into credentialsMetadataSetter struct such that credentialsMetadataSetter struct implements the CredentialsMetadataSetter interface defined in ecs-agent/acs/session/refresh_credentials_responder.go
    • agent/acs/handler/refresh_credentials_handler_linux.go -> agent/acs/handler/refresh_credentials_responder_linux.go (file renamed)
    • agent/acs/handler/refresh_credentials_handler_windows.go -> agent/acs/handler/refresh_credentials_responder_windows.go (file renamed)
  2. Update other associated files
    • agent/acs/handler/acs_handler.go
      • Initialize refresh credentials responder and add its HandlerFunc function as a request handler to the ACS client, such that refresh credentials responder dictates what to do in response to receiving refresh credentials messages from ACS
      • Remove refresh credentials messages from pending ACKs logic since it is no longer applicable after the refactor of ACS refresh credentials message handling
        • Such logic is no longer applicable since there is no need to do multi-thread synchronization with channels with this responder (like we currently do). As part of the refactor, we factor out this non-necessity and thus methods such as start(), stop(), clearAcks(), etc. can be removed. The resulting responder is a lot cleaner and simpler to follow.
          For reference see the following PR for a similar change previously made to the heartbeat responder for the same reason: Fix flakey leak test for ACS Handler #3232
    • agent/acs/handler/payload_handler.go
      • Don't pass in the entire refresh credentials responder to the payload responder as it is only being passed in to send ACKs of credentials associated with task(s) back to ACS. Instead, just write a private payload responder method makeCredentialsAckRequest and use it to accomplish the same while avoiding passing in a whole entire responder to another responder
        • For context, sending ACKs of credentials in the payload responder is necessary because payload responder handles credentials that are associated with task(s) contained in a payload message (type ecsacs.PayloadMessage). In contrast, refresh credentials responder handles credentials that are contained in a refresh credentials message (type ecsacs.IAMRoleCredentialsMessage)
    • ecs-agent/acs/session/generate_mocks.go and ecs-agent/acs/session/mocks/session_mock.go
      • Generate new mocks for the CredentialsMetadataSetter interface to be used in testing
    • ecs-agent/metrics/constants.go
      • Add constants specific to refresh credentials responder
  3. Improve/update tests, most notably:
    • Update existing tests accordingly based off of above changes to refactor ACS refresh credentials message handling in the following files:
      • agent/acs/handler/refresh_credentials_handler_test.go -> ecs-agent/acs/session/refresh_credentials_responder_test.go (partial)
        • Refresh credentials responder tests that only need to interact with stateless components (e.g., message validation) belong in this file
      • agent/acs/handler/refresh_credentials_handler_test.go -> agent/acs/handler/refresh_credentials_responder_test.go (partial)
        • Refresh credentials responder tests that need to directly interact with Agent state belong in this file
      • agent/acs/handler/acs_handler_test.go
    • Update existing tests in agent/acs/handler/payload_handler_test.go accordingly based off of above changes to agent/acs/handler/payload_handler.go
    • General cleanup/quality of life updates to tests (e.g., use assert package for test assertions instead of if statement blocks)
    • Replace usage of test constants currently defined in agent/acs/handler/refresh_credentials_handler_test.go with test constants defined in centralized file ecs-agent/acs/session/testconst/test_const.go instead.
      • This results in changes to the following non-refresh credentials responder test files:
        • agent/acs/handler/attach_eni_handler_common_test.go
        • agent/acs/handler/payload_handler_test.go
        • agent/acs/handler/task_manifest_handler_test.go

Testing

Unit, integration, and functional tests.

New tests cover the changes: yes

Description for the changelog

Refactor ACS refresh credentials 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 July 26, 2023 19:50
@danehlim danehlim requested a review from a team as a code owner July 26, 2023 19:50
Realmonia
Realmonia previously approved these changes Jul 27, 2023
@danehlim danehlim merged commit 0aa46c1 into aws:dev Jul 29, 2023
39 checks passed
@chienhanlin chienhanlin mentioned this pull request Aug 9, 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.

None yet

4 participants