Add wsclient library to ecs-agent module #3690

RichaGangwar · 2023-05-11T16:02:06Z

Summary

Add wsclient library to ecs-agent

Implementation details

Added a new directory wsclient to ecs-agent.
Added the new files in wsclient related to wsclient functionality
Added utils directory to move the needed dependencies from agent to ecs-agent.
Edited the needed imports in agent related to moved utils.
Moved Proxy( *http.Request) function to proxy.go from utils.go to avoid moving the whole utils.go in ecs-agent.

TODOs to be handled in future PRs:

Periodic timeout/disconnection of WebSocket connections in wsclient instead of tcs/acs clients
Move exitcodes package to /ecs-agent
Use context.Canceled.Error() instead of hardcoding "context canceled"
Replace GetMockServer() used in unit tests with gomock generated mock interfaces.
Remove docker specific checks in wsclient library.
Remove existing agent/wsclient after plugging in the new libraries in ecs-agent, i.e. tcs and acs.

Testing

Unit tested with existing tests in wsclient
Unit tested agent with make test

New tests cover the changes: no

Description for the changelog

Add wsclient library to ecs-agent module.

Licensing

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

danehlim · 2023-05-11T21:20:35Z

Linux / Linux unit tests check is failing at the moment, just want to confirm is this PR ready for review or currently still WIP?

RichaGangwar · 2023-05-11T22:19:23Z

Linux / Linux unit tests check is failing at the moment, just want to confirm is this PR ready for review or currently still WIP?
@danehlim Its ready for review. The unit tests passed in my local setup. The Linux unit tests is failing here due to less coverage than expected. Ideally, we should not see the coverage being lowered as we are moving the existing files. I am checking on it.

danehlim · 2023-05-12T19:19:02Z

agent/wsclient/client.go

@@ -35,10 +35,11 @@ import (
 	"time"

 	"github.com/aws/amazon-ecs-agent/agent/config"
-	"github.com/aws/amazon-ecs-agent/agent/utils"
-	"github.com/aws/amazon-ecs-agent/agent/utils/cipher"
 	"github.com/aws/amazon-ecs-agent/agent/wsclient/wsconn"


Shouldn't imports of "github.com/aws/amazon-ecs-agent/agent/wsclient/wsconn" be updated to "github.com/aws/amazon-ecs-agent/ecs-agent/wsclient/wsconn" then we can remove the unused files in agent/wsclient/wsconn, right? (i.e., like what we've done with agent/utils/cipher and agent/utils/httpproxy)

I do not think it makes sense to switch just wsconn imports to ecs-agent/wsclient. The plan is to plug in the ecs-agent/wsclient with ecs-agent/tcs and ecs-agent/acs, and once we have the confidence in testing, we would completely remove the agent/wsclient package. It made sense to move the utils to avoid the cyclic dependency issue between agent and ecs-agent.

I was just thinking since the files in agent/wsclient/wsconn and ecs-agent/wsclient/wsconn are identical, we could just have a single one living in ecs-agent/to refer to as a source of truth since we will be removing agent/wsclient/wsconn in the future anyway.

once we have the confidence in testing, we would completely remove the agent/wsclient package

do we have a TODO for this anywhere (in code or in a tracking ticket)?

Reply from RichaGangwar:
Yes Ray, Thats the plan. As part of tcs and acs shared lib, we will integrate this new wsclient lib in the clients. And once well tested, we will remove the agent/wsclient. There isn't any TODO right now. Will add it in description to keep track of it. Thank you :)

danehlim · 2023-05-12T19:34:29Z

ecs-agent/wsclient/client.go

+// AgentConfig is a subset of agent's config. Since this requires agent specific information,
+// making this as a struct to be passed from the individual agents while using the wsclient shared lib.
+// The agent where the field is not applicable can be left empty
+type AgentConfig struct {


Can we rename this to something that makes it more distinguishable from the original agent config (maybe something along the lines ofMinimalAgentConfig, or WSClientAgentConfigs)?

Sure. I will update it.

danehlim · 2023-05-12T19:47:28Z

ecs-agent/wsclient/client.go

+	timeoutDialer := &net.Dialer{Timeout: wsConnectTimeout}
+	tlsConfig := &tls.Config{ServerName: parsedURL.Host, InsecureSkipVerify: cs.cfg.AcceptInsecureCert, MinVersion: tls.VersionTLS12}
+
+	//TODO: In order to get rid of the check -


Do we need to get rid of this check as part of this PR? It seems to be specific to Docker, and I don't think we want anything tied to or specific to Docker in the ecs-agent module.

Yes, ideally I want to get rid of the check. I had a discussion with multiple folks and able to resolve few other such checks. I am preparing to doc to deal with these two checks. Once everyone agrees on the strategy, we can remove these checks as well. I had raised the PR so that ACS is unblocked, as driving consensus is taking some time.

Sounds good, will look forward to this in a followup PR then. If not already, let's make sure we're tracking this internally.

danehlim · 2023-05-12T20:10:53Z

ecs-agent/wsclient/client.go

+// ClientServerImpl wraps commonly used methods defined in ClientServer interface.
+type ClientServerImpl struct {
+	// cfg is the subset of user-specified runtime configuration
+	cfg *AgentConfig


This needs to be capitalized to be exported.

Thank you for pointing out. Will fix this.

danehlim · 2023-05-12T20:14:33Z

ecs-agent/wsclient/client.go

+
+	"github.com/aws/aws-sdk-go/aws/credentials"
+	"github.com/aws/aws-sdk-go/private/protocol/json/jsonutil"
+	"github.com/cihub/seelog"


I think we want to be using github.com/aws/amazon-ecs-agent/ecs-agent/logger for any logging in ecs-agent/. I see we are already using it for some of the logging in this file, could we update to make sure we’re using that and not seelog directly in ecs-agent/ (whether that be in this PR or a future one)?

danehlim · 2023-05-12T20:20:11Z

ecs-agent/wsclient/client.go

+// by backend. It allows for bidirectional communication and acts as both a
+// client-and-server in terms of requests, but only as a client in terms of
+// connecting.
+package wsclient


Periodic timeout/disconnection of WebSocket connections seems to be missing from this PR, let’s make sure to add that in if we can.

Currently, the timeouts are initiated by individual components using the library, i.e. ACS and TACS have their own implementation of timeouts. I had not accounted for that as part of wsclient library, hence it's missing from this PR. Is that something we want to unify?

Yeah, I believe we decided that we want to have the same logic for refreshing our websocket connection across all our websocket connections (i.e., ACS and TACS websocket connections). I'll reach out to you offline and point you in the direction of these previous discussions.

danehlim · 2023-05-15T18:11:21Z

ecs-agent/utils/sign_http_request.go

+	signer := v4.NewSigner(creds)
+	_, err := signer.Sign(req, body, service, region, time.Now())
+	if err != nil {
+		seelog.Warnf("Signing HTTP request failed: %v", err)


This should be updated to use logger instead of seelog as well (per this previous comment).

Fixed. Thank you for pointing out.

ohsoo · 2023-05-15T23:26:46Z

ecs-agent/wsclient/client.go

+
+	// ExitTerminal indicates the agent run into error that's not recoverable
+	// no need to restart
+	ExitTerminal = 5


Can we move the exitcodes package to /ecs-agent? Perhaps in a fast follow-up PR, if not this one.

Sure. Will move it as a follow-up PR. This is already quite bulky. But thank you for pointing out.

ohsoo · 2023-05-15T23:31:32Z

ecs-agent/wsclient/client.go

+}
+
+// RespondFunc specifies a function callback that can be used by the
+// RequestResonder to respond to requests.


nit: typo, RequestResonder -> RequestResponder

Fixed. Thanks.

ohsoo · 2023-05-15T23:39:40Z

ecs-agent/wsclient/client.go

+// ClientServerImpl wraps commonly used methods defined in ClientServer interface.
+type ClientServerImpl struct {
+	// Cfg is the subset of user-specified runtime configuration
+	Cfg *WSClientMinAgentConfig


Does Cfg need to be exported?

Yes, it would need to be populated by the ACS/TACS libraries. So it needs to be exported.

ohsoo · 2023-05-15T23:41:01Z

ecs-agent/wsclient/client.go

+	io.Closer
+}
+
+// AgentConfig is a subset of agent's config. Since this requires agent specific information,


nit: "WSClientMinAgentConfig is a subset[...]"

Fixed. Thanks.

ohsoo · 2023-05-15T23:44:36Z

ecs-agent/wsclient/client.go

+// messages from an active connection.
+func (cs *ClientServerImpl) ConsumeMessages(ctx context.Context) error {
+	// Since ReadMessage is blocking, we don't want to wait for timeout when context gets cancelled
+


nit: weird newline

danehlim · 2023-05-15T23:51:22Z

ecs-agent/wsclient/client_factory.go

+// ClientFactory interface abstracts the method that creates new ClientServer
+// objects. This is helpful when writing unit tests.
+type ClientFactory interface {
+	New(url string, credentialProvider *credentials.Credentials, rwTimeout time.Duration) ClientServer


This needs to be updated to include a *wsclient.WSClientMinAgentConfig parameter in the function signature.

Fixed. Thank you for pointing it out. :)

ohsoo · 2023-05-15T23:52:14Z

ecs-agent/wsclient/mock/utils/utils.go

+)
+
+// GetMockServer returns a mock websocket server that can be started up as TLS or not.
+// TODO replace with gomock


Is this TODO doable within this PR?

This mockServer is heavily used in all existing test cases. Maybe it can be picked up in a separate PR. But it would need good amount of rewrite.

I wonder if this isn't something we can pull in from our mockgen process in the agent. Ideally all of our mocks are generated. (We'll follow up offline).

danehlim · 2023-05-18T00:00:47Z

ecs-agent/wsclient/client_test.go

+//test for DecodeConnectionError
+//use WriteCloseMessage
+
+//send httpResponse in Connect()


Are there missing tests here on L349-L352, or what are these comments?

Missed removing the comments. I was using it while writing new tests to ensure the coverage requirements are met. Will remove them in latest commit.

ohsoo · 2023-05-18T16:04:53Z

ecs-agent/wsclient/client_test.go

+	waitForRequests.Wait()
+}
+
+func getClientServer(url string, msgType []interface{}) *ClientServerImpl {


Can we change this name to be dummyClientServer or testClientServer or similar? That seems a lot more clear to me.

Sure. Done.

ohsoo · 2023-05-18T16:04:56Z

ecs-agent/wsclient/client_test.go

+}
+
+func getClientServer(url string, msgType []interface{}) *ClientServerImpl {
+


nit: spacing

ohsoo · 2023-05-18T16:05:54Z

ecs-agent/wsclient/client_test.go

+
+// Close closes the underlying connection. Implement Close() in this file
+// as ClientServerImpl doesn't implement it. This is needed by the
+// TestSetReadDeadline* tests


Let's update comments to end with periods/have proper punctuation if they don't already

Sure. Fixed.

ohsoo

Let's do some commit squashing please. Overall, not seeing any major concerns!

RichaGangwar · 2023-05-18T18:09:30Z

Let's do some commit squashing please. Overall, not seeing any major concerns!

Sure Soo.

danehlim · 2023-05-18T18:16:34Z

ecs-agent/wsclient/client.go

+	for {
+		select {
+		case <-ctx.Done():
+			// Close connection and wait for Read goroutine to finish


Can we update our tests to cover this code path (i.e., L491-L495)? This is a major change from our current agent/wsclient/client.go, and we currently don't have any tests that validate this behavior.

Sure. Added.

danehlim · 2023-05-18T20:51:16Z

ecs-agent/wsclient/client_test.go

+	}()
+	// Cancel the context.
+	cancel()
+	assert.EqualError(t, <-messageError, "context canceled")


nit (not blocking, can be added in a followup PR so that this PR can get merged and unblock other work ASAP):

Can we use context.Canceled.Error() here instead of hardcoding "context canceled" (i.e., similar to what we do on L283)?

danehlim

Approved under the agreement that we will fast follow with a PR to include periodic timeout/disconnection of websocket connections logic as part of the wsclient itself (per this comment thread) and that we will squash commits before merging.

fierlion

Can you update the description to highlight TODOs from code and from comments so we can track these in our release process?

fierlion · 2023-05-22T17:06:57Z

agent/wsclient/client.go

@@ -35,10 +35,11 @@ import (
 	"time"

 	"github.com/aws/amazon-ecs-agent/agent/config"
-	"github.com/aws/amazon-ecs-agent/agent/utils"
-	"github.com/aws/amazon-ecs-agent/agent/utils/cipher"
 	"github.com/aws/amazon-ecs-agent/agent/wsclient/wsconn"


once we have the confidence in testing, we would completely remove the agent/wsclient package

do we have a TODO for this anywhere (in code or in a tracking ticket)?

Reply from RichaGangwar:
Yes Ray, Thats the plan. As part of tcs and acs shared lib, we will integrate this new wsclient lib in the clients. And once well tested, we will remove the agent/wsclient. There isn't any TODO right now. Will add it in description to keep track of it. Thank you :)

fierlion · 2023-05-22T22:24:20Z

ecs-agent/wsclient/mock/utils/utils.go

+)
+
+// GetMockServer returns a mock websocket server that can be started up as TLS or not.
+// TODO replace with gomock


I wonder if this isn't something we can pull in from our mockgen process in the agent. Ideally all of our mocks are generated. (We'll follow up offline).

fierlion · 2023-05-22T22:28:20Z

agent/httpclient/httpclient.go

 	"github.com/aws/amazon-ecs-agent/agent/version"
+	"github.com/aws/amazon-ecs-agent/ecs-agent/utils/cipher"
+	"github.com/aws/amazon-ecs-agent/ecs-agent/utils/httpproxy"


will we be able to remove https://github.com/aws/amazon-ecs-agent/blob/master/agent/utils/httpproxy in a future pr?

So, agent/utils/httpproxy is already moved to ecs-agent/utils/httpproxy as part of this PR. Is that what you are referring to Ray?

fierlion

Can you update the description to highlight TODOs from code and from comments so we can track these in our release process?

RichaGangwar · 2023-05-23T17:35:37Z

Can you update the description to highlight TODOs from code and from comments so we can track these in our release process?

Sure Ray.

RichaGangwar requested review from fierlion, Realmonia, ohsoo and danehlim May 11, 2023 16:02

RichaGangwar requested a review from a team as a code owner May 11, 2023 16:02

RichaGangwar added the bot/test label May 11, 2023

RichaGangwar changed the title ~~Ecs agent wsclient lib~~ Add wsclient library to ecs-agent module May 11, 2023

danehlim reviewed May 12, 2023

View reviewed changes

RichaGangwar force-pushed the ecs-agent-wsclient-lib branch from 21f2333 to f9488b3 Compare May 12, 2023 23:31

danehlim reviewed May 15, 2023

View reviewed changes

ohsoo reviewed May 15, 2023

View reviewed changes

danehlim reviewed May 15, 2023

View reviewed changes

ohsoo reviewed May 15, 2023

View reviewed changes

Realmonia added bot/test and removed bot/test labels May 17, 2023

amazon-ecs-bot removed the bot/test label May 17, 2023

danehlim reviewed May 18, 2023

View reviewed changes

ohsoo reviewed May 18, 2023

View reviewed changes

danehlim added the bot/test label May 18, 2023

amazon-ecs-bot removed the bot/test label May 18, 2023

danehlim added the bot/test label May 18, 2023

amazon-ecs-bot removed the bot/test label May 18, 2023

danehlim reviewed May 18, 2023

View reviewed changes

danehlim added the bot/test label May 18, 2023

amazon-ecs-bot removed the bot/test label May 18, 2023

danehlim reviewed May 18, 2023

View reviewed changes

danehlim previously approved these changes May 18, 2023

View reviewed changes

RichaGangwar force-pushed the ecs-agent-wsclient-lib branch from 8cfa5a0 to 6cc9959 Compare May 18, 2023 23:22

RichaGangwar dismissed danehlim’s stale review via f04f424 May 18, 2023 23:33

Add wsclient library to ecs-agent module

14407ee

RichaGangwar force-pushed the ecs-agent-wsclient-lib branch from f04f424 to 14407ee Compare May 19, 2023 20:57

Merge branch 'dev' into ecs-agent-wsclient-lib

26670d5

danehlim added the bot/test label May 19, 2023

amazon-ecs-bot removed the bot/test label May 19, 2023

danehlim approved these changes May 19, 2023

View reviewed changes

Realmonia approved these changes May 22, 2023

View reviewed changes

fierlion approved these changes May 22, 2023

View reviewed changes

Realmonia merged commit 6f994c6 into aws:dev May 23, 2023

ubhattacharjya mentioned this pull request May 24, 2023

Release 1.71.2 #3712

Merged

		}

		func getClientServer(url string, msgType []interface{}) *ClientServerImpl {

Add wsclient library to ecs-agent module #3690

Add wsclient library to ecs-agent module #3690

Conversation

RichaGangwar commented May 11, 2023 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

danehlim commented May 11, 2023

RichaGangwar commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehlim May 12, 2023 • edited Loading

Choose a reason for hiding this comment

fierlion May 22, 2023 • edited by RichaGangwar Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehlim May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohsoo May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohsoo left a comment

Choose a reason for hiding this comment

RichaGangwar commented May 18, 2023

danehlim May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehlim May 18, 2023 • edited Loading

Choose a reason for hiding this comment

danehlim left a comment • edited Loading

Choose a reason for hiding this comment

fierlion left a comment

Choose a reason for hiding this comment

fierlion May 22, 2023 • edited by RichaGangwar Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fierlion left a comment

Choose a reason for hiding this comment

RichaGangwar commented May 23, 2023

RichaGangwar commented May 11, 2023 •

edited

Loading

danehlim May 12, 2023 •

edited

Loading

fierlion May 22, 2023 •

edited by RichaGangwar

Loading

danehlim May 12, 2023 •

edited

Loading

ohsoo May 18, 2023 •

edited

Loading

danehlim May 18, 2023 •

edited

Loading

danehlim May 18, 2023 •

edited

Loading

danehlim left a comment •

edited

Loading

fierlion May 22, 2023 •

edited by RichaGangwar

Loading