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

Add TMDS initialization functionality to ecs-agent module #3660

Merged
merged 9 commits into from
Apr 27, 2023

Conversation

amogh09
Copy link
Contributor

@amogh09 amogh09 commented Apr 26, 2023

Summary

Add TMDS initialization functionality to ecs-agent module. The server read/write timeout, request rate limits, listen address, and router need to be provided by the consumer. The server is configured with a request rate limiter and logging middleware just like the TMDS in agent module. agent module will be updated in a separate PR to consume this new functionality for initializing TMDS.

Also making integration test make targets run integration tests of ecs-agent module.

Implementation details

  • Add a new tmds package to ecs-agent module and add NewServer() function to it. The function takes an instance of AuditLogger and an arbitrary number of configuration options and initializes a net/http server for TMDS. Configuration options must include a listen address and a router for the server.
  • The NewServer() function creates request rate limiter and logger middlewares and adds them to the server it initializes and returns the server.
  • Update make run-integ-tests target to run integration tests for ecs-agent module too.

Testing

Not a part of this PR but I replaced the current TMDS initialization in agent module with the new TMDS initialization from ecs-agent module and built Agent from source. I then sent some requests to TMDS which were successful. More comprehensive tests will be performed for the next CR that will include changes I used for the testing.

Added integration tests for server write timeout and rate limiter.

New tests cover the changes: yes

Description for the changelog

Add TMDS initialization functionality 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.

}

// Define a reqeuest rate limiter
limiter := tollbooth.
Copy link
Contributor Author

@amogh09 amogh09 Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server setup here is taken from agent module

limiter := tollbooth.NewLimiter(float64(steadyStateRate), nil)
limiter.SetOnLimitReached(handlersutils.LimitReachedHandler(auditLogger))
limiter.SetBurst(burstRate)
// Log all requests and then pass through to muxRouter.
loggingMuxRouter := mux.NewRouter()
// rootPath is a path for any traffic to this endpoint, "root" mux name will not be used.
rootPath := "/" + handlersutils.ConstructMuxVar("root", handlersutils.AnythingRegEx)
loggingMuxRouter.Handle(rootPath, tollbooth.LimitHandler(
limiter, NewLoggingHandler(muxRouter)))
loggingMuxRouter.SkipClean(false)
server := http.Server{
Addr: "127.0.0.1:" + strconv.Itoa(config.AgentCredentialsPort),
Handler: loggingMuxRouter,
ReadTimeout: readTimeout,
WriteTimeout: writeTimeout,
}

}

// LimitReachedHandler logs the throttled request in the credentials audit log
func limitReachedHandler(auditLogger audit.AuditLogger) func(http.ResponseWriter, *http.Request) {
Copy link
Contributor Author

@amogh09 amogh09 Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This handler is taken from agent module

func LimitReachedHandler(auditLogger audit.AuditLogger) func(http.ResponseWriter, *http.Request) {
return func(w http.ResponseWriter, r *http.Request) {
logRequest := request.LogRequest{
Request: r,
}
auditLogger.Log(logRequest, http.StatusTooManyRequests, "")
}
}

@amogh09 amogh09 changed the title Unified tmds init Add TMDS initialization function to ecs-agent module Apr 26, 2023
@amogh09 amogh09 changed the title Add TMDS initialization function to ecs-agent module Add TMDS initialization functionality to ecs-agent module Apr 26, 2023
@amogh09 amogh09 marked this pull request as ready for review April 26, 2023 21:38
@amogh09 amogh09 requested a review from a team as a code owner April 26, 2023 21:38
"github.com/cihub/seelog"
)

// LoggingHandler is used to log all requests for an endpoint.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is taken from agent module.

// LoggingHandler is used to log all requests for an endpoint.
type LoggingHandler struct{ h http.Handler }
// NewLoggingHandler creates a new LoggingHandler object.
func NewLoggingHandler(handler http.Handler) LoggingHandler {
return LoggingHandler{h: handler}
}
// ServeHTTP logs the method and remote address of the request.
func (lh LoggingHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
seelog.Debug("Handling http request", "method", r.Method, "from", r.RemoteAddr)
lh.h.ServeHTTP(w, r)
}

}

// Create a new HTTP Task Metadata Server (TMDS)
func NewServer(auditLogger audit.AuditLogger, options ...ConfigOpt) (*http.Server, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ecs-agent/tmds/server.go Outdated Show resolved Hide resolved
ecs-agent/tmds/server.go Outdated Show resolved Hide resolved
ecs-agent/tmds/server.go Outdated Show resolved Hide resolved
ecs-agent/tmds/server.go Show resolved Hide resolved
singholt
singholt previously approved these changes Apr 27, 2023
if err == nil {
return nil // server is up now
}
time.Sleep(100 * time.Millisecond)
Copy link
Contributor

@prateekchaudhry prateekchaudhry Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit/q: probably not very significant here.. should this sleep be before Get call? I think we may always want to sleep at least once, and the last sleep (i==9) is redundant if we put the sleep here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I will move the sleep to before the HTTP call!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in d7da34f

@amogh09 amogh09 dismissed stale reviews from prateekchaudhry and singholt via d7da34f April 27, 2023 21:12
@amogh09 amogh09 merged commit c83389c into aws:dev Apr 27, 2023
@amogh09 amogh09 deleted the unified-tmds-init branch April 27, 2023 23:49
@Realmonia Realmonia mentioned this pull request May 9, 2023
Realmonia pushed a commit to Realmonia/amazon-ecs-agent that referenced this pull request May 16, 2023
Realmonia pushed a commit to Realmonia/amazon-ecs-agent that referenced this pull request May 21, 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