Set router skip clean to false #1844

suneyz · 2019-02-13T02:10:20Z

Summary

This enables agent to handle request like "//v2/credential" correctly. This brings back the support for //path that agent version 1.20.3 or before
#1839

Implementation details

set router skip clean to false
doc for skip clean https://godoc.org/github.com/gorilla/mux#Router.SkipClean

Testing

New tests cover the changes:

yes,

unit test, functional test

Description for the changelog

Licensing

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

samuelkarp · 2019-02-13T02:25:28Z

New tests cover the changes:

I don't see any tests in the pull request. Can you add a functional test covering this change?

yhlee-aws · 2019-02-13T17:02:05Z

v3 tests are failing, is this causing different regression?
TestTaskHTTPEndpointErrorCode404 (0.02s)
TestTaskHTTPEndpointErrorCode404/Test_path:/v3///task/
TestTaskHTTPEndpointErrorCode400
TestTaskHTTPEndpointErrorCode400/Test_path:/v3//stats
TestTaskHTTPEndpointErrorCode400/Test_path:/v3//task
TestTaskHTTPEndpointErrorCode400/Test_path:/v3//task/stats

suneyz · 2019-02-13T20:03:43Z

New tests cover the changes:

I don't see any tests in the pull request. Can you add a functional test covering this change?

Great call out! Added tests, there is actually unit tests that covers router

samuelkarp · 2019-02-13T20:18:06Z

Can you add a functional test as well?

suneyz · 2019-02-14T18:10:05Z

Can you add a functional test as well?

updated with functional tests

shubham2892 · 2019-02-14T18:27:07Z

misc/task-server-endpoint-validator/Dockerfile

@@ -0,0 +1,7 @@
+FROM ubuntu:latest


Locking this to a version would be a good idea. We are using 16.04 at other places.

make sense :)

sharanyad · 2019-02-14T18:30:29Z

agent/handlers/task_server_setup_test.go

@@ -1101,11 +1128,11 @@ func TestTaskHTTPEndpointErrorCode400(t *testing.T) {
 		"/v3/wrong-v3-endpoint-id",
 		"/v3/",
 		"/v3/wrong-v3-endpoint-id/stats",
-		"/v3//stats",
+		"/v3/stats",


Why was this changed? Shouldn't "/v3//stats" work as well?

we handle double/muti slash with a redirect prior to 1.20.3 release. at 1.21.0 release we changed the router and added the behavior to not clean double/multi slash. This is the root cause of this issue #1839.

shubham2892 · 2019-02-14T18:31:31Z

...unctional_tests/testdata/taskdefinitions/task-server-endpoint-validator/task-definition.json

+    "networkMode": "host",
+    "containerDefinitions": [{
+        "image": "127.0.0.1:51670/amazon/task-server-endpoint-validator:latest",
+        "name": "container_1",


nit: rename this to a more descripritive one.

do you have a suggestion? i thought the task definition and image name is descriptive enough and i didn't want to introduce similar/same name to cause confusion as it is the only container in the test.

Ah what about something like task_server_endpoint_validator_container? 🤔

fierlion · 2019-02-14T18:38:32Z

agent/handlers/task_server_setup.go

-	muxRouter.SkipClean(true)
+	// Set this to false so that for request like "//v3//metadata/task"
+	// to permanently redirect(301) to "/v3/metadata/task" handler
+	muxRouter.SkipClean(false)


Since this defaults to false (from the docs: SkipClean defines the path cleaning behaviour for new routes. The initial value is false.) I initially wondered if we even need the explicit setting here.
But I like it here. It's deliberate.
I would make the comment clearer (like the one in the docs: When false, the path will be cleaned, so //v3//metadata/task will become /v3/metadata/task)

I think a 301 redirect is more clear as it is not "becoming" the new url. it is a specific 301 redirect to the new url. let me know how you think

I see w.WriteHeader(http.StatusMovedPermanently) aka 301. Cool.

samuelkarp · 2019-02-14T18:44:43Z

agent/functional_tests/tests/functionaltests_unix_test.go

@@ -1427,3 +1427,45 @@ func TestElasticInferenceValidator(t *testing.T) {
 	exitCode, _ := task.ContainerExitcode("container_1")
 	assert.Equal(t, 42, exitCode, fmt.Sprintf("Expected exit code of 42 for container; got %d", exitCode))
 }
+
+// TestServerEndpointValidator tests the workflow of task server endpoint
+func TestServerEndpointValidator(t *testing.T) {


Is there a particular reason you hand-wrote this code instead of using the simpletests package? I would recommend using that instead.

I believe it is for setting the task iam roles configuration.

Discussed with Sam offline, we might not need network mode setup for it to work.

But I'm thinking for now, let's keep test file as hand-written to release it today to solve our customers' issue. I will keep troubleshooting to see if I can make functional test work with simpletests file

Task IAM roles are supported with "host", "bridge", and "awsvpc" network modes, and we have existing functional tests that validate this. There should not be a dependency on using the "host" network mode.

Read some document, agree on this should not depend on host mode. I will dive deep to see if my host needs some config to make it functional test on other modes, and I will follow up with another pr to address it once i get it figured out.

samuelkarp · 2019-02-14T18:47:07Z

misc/task-server-endpoint-validator/Dockerfile

@@ -0,0 +1,7 @@
+FROM ubuntu:latest


If you use an image like busybox which already includes wget or an image like amazonlinux:2 which already includes curl, you can avoid having to construct a new image and instead can just override the command.

awesome! will change it to al2 image

tommyhahn · 2019-02-14T19:11:59Z

scripts/setup-test-registry

@@ -45,7 +45,7 @@ mirror_local_image() {
  docker rmi $2
 }

-for image in "amazon/amazon-ecs-netkitten" "amazon/amazon-ecs-volumes-test" "amazon/amazon-ecs-pid-namespace-test" "amazon/amazon-ecs-ipc-namespace-test" "amazon/squid" "amazon/awscli" "amazon/image-cleanup-test-image1" "amazon/image-cleanup-test-image2" "amazon/image-cleanup-test-image3" "amazon/fluentd" "amazon/amazon-ecs-agent-introspection-validator" "amazon/amazon-ecs-taskmetadata-validator" "amazon/amazon-ecs-v3-task-endpoint-validator" "amazon/amazon-ecs-container-metadata-file-validator" "amazon/amazon-ecs-elastic-inference-validator"; do
+for image in "amazon/amazon-ecs-netkitten" "amazon/amazon-ecs-volumes-test" "amazon/amazon-ecs-pid-namespace-test" "amazon/amazon-ecs-ipc-namespace-test" "amazon/squid" "amazon/awscli" "amazon/image-cleanup-test-image1" "amazon/image-cleanup-test-image2" "amazon/image-cleanup-test-image3" "amazon/fluentd" "amazon/amazon-ecs-agent-introspection-validator" "amazon/amazon-ecs-taskmetadata-validator" "amazon/amazon-ecs-v3-task-endpoint-validator" "amazon/amazon-ecs-container-metadata-file-validator" "amazon/amazon-ecs-elastic-inference-validator" "amazon/task-server-endpoint-validator"; do


Maybe it is trivial but I am wondering why this diff is the only line that specific changes are not highlighted in the deeper green for "amazon/task-server-endpoint-validator"? Or just it happens once in a while...

not sure, maybe it is a github bug lol

sharanyad · 2019-02-14T19:43:59Z

could you please shorten the commit message to 75 characters or less?
You could a description in a different line below if needed.
Example commit message: c140bd3

This will redirect request like //v2/credential to /v2/credential for the correct handler to handle it

suneyz · 2019-02-14T19:53:09Z

could you please shorten the commit message to 75 characters or less?
You could a description in a different line below if needed.
Example commit message: c140bd3

Will do

sharanyad

once tests pass

suneyz · 2019-02-14T22:17:58Z

failed on flaky test TestDockerStopTimeout for both linux and arm. merge the change

aaithal · 2019-02-14T22:51:29Z

failed on flaky test TestDockerStopTimeout for both linux and arm. merge the change

Is there an issue tracking that? How long can we expect these tests to be flaky?

suneyz · 2019-02-14T23:08:03Z

failed on flaky test TestDockerStopTimeout for both linux and arm. merge the change

Is there an issue tracking that? How long can we expect these tests to be flaky?

It is tracked in our sprint board.

aaithal · 2019-02-14T23:10:43Z

It is tracked in our sprint board.

Can you please create an issue in this repo because that's where we track these things? That way, you can tie that into releases, track when it surfaced, when it was fixed etc. Also, as a customer, or as someone who doesn't have visibility into your sprint board, it's hard to track if there's nothing in the github repo related to that.

suneyz · 2019-02-14T23:24:54Z

It is tracked in our sprint board.

Can you please create an issue in this repo because that's where we track these things? That way, you can tie that into releases, track when it surfaced, when it was fixed etc. Also, as a customer, or as someone who doesn't have visibility into your sprint board, it's hard to track if there's nothing in the github repo related to that.

Github issue created. #1855

suneyz requested a review from a team February 13, 2019 02:10

suneyz force-pushed the 1839 branch from f3110aa to 9d13256 Compare February 13, 2019 19:50

suneyz added the bot/test label Feb 13, 2019

suneyz force-pushed the 1839 branch 3 times, most recently from a0d61db to c997144 Compare February 14, 2019 07:54

suneyz added bot/test and removed bot/test labels Feb 14, 2019

shubham2892 reviewed Feb 14, 2019

View reviewed changes

sharanyad reviewed Feb 14, 2019

View reviewed changes

shubham2892 reviewed Feb 14, 2019

View reviewed changes

fierlion reviewed Feb 14, 2019

View reviewed changes

samuelkarp reviewed Feb 14, 2019

View reviewed changes

tommyhahn reviewed Feb 14, 2019

View reviewed changes

suneyz force-pushed the 1839 branch 2 times, most recently from 1ea6e91 to 2751aee Compare February 14, 2019 19:47

Set router skip clean to false.

02846a0

This will redirect request like //v2/credential to /v2/credential for the correct handler to handle it

suneyz force-pushed the 1839 branch from 2751aee to 02846a0 Compare February 14, 2019 19:52

suneyz added bot/test and removed bot/test labels Feb 14, 2019

sharanyad approved these changes Feb 14, 2019

View reviewed changes

shubham2892 approved these changes Feb 14, 2019

View reviewed changes

yhlee-aws approved these changes Feb 14, 2019

View reviewed changes

suneyz merged commit fc92bf4 into aws:dev Feb 14, 2019

suneyz added this to the 1.25.3 milestone Feb 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set router skip clean to false #1844

Set router skip clean to false #1844

suneyz commented Feb 13, 2019 •

edited

Loading

samuelkarp commented Feb 13, 2019

yhlee-aws commented Feb 13, 2019

suneyz commented Feb 13, 2019

samuelkarp commented Feb 13, 2019

suneyz commented Feb 14, 2019

shubham2892 Feb 14, 2019

suneyz Feb 14, 2019

sharanyad Feb 14, 2019

suneyz Feb 14, 2019

shubham2892 Feb 14, 2019

suneyz Feb 14, 2019

shubham2892 Feb 14, 2019

fierlion Feb 14, 2019 •

edited

Loading

suneyz Feb 14, 2019

fierlion Feb 14, 2019

samuelkarp Feb 14, 2019

sharanyad Feb 14, 2019

suneyz Feb 14, 2019

samuelkarp Feb 14, 2019

suneyz Feb 14, 2019

samuelkarp Feb 14, 2019

suneyz Feb 14, 2019

tommyhahn Feb 14, 2019

suneyz Feb 14, 2019

sharanyad commented Feb 14, 2019

suneyz commented Feb 14, 2019

sharanyad left a comment

suneyz commented Feb 14, 2019

aaithal commented Feb 14, 2019 •

edited

Loading

suneyz commented Feb 14, 2019

aaithal commented Feb 14, 2019

suneyz commented Feb 14, 2019

Set router skip clean to false #1844

Set router skip clean to false #1844

Conversation

suneyz commented Feb 13, 2019 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

samuelkarp commented Feb 13, 2019

yhlee-aws commented Feb 13, 2019

suneyz commented Feb 13, 2019

samuelkarp commented Feb 13, 2019

suneyz commented Feb 14, 2019

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

fierlion Feb 14, 2019 • 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

sharanyad commented Feb 14, 2019

suneyz commented Feb 14, 2019

sharanyad left a comment

Choose a reason for hiding this comment

suneyz commented Feb 14, 2019

aaithal commented Feb 14, 2019 • edited Loading

suneyz commented Feb 14, 2019

aaithal commented Feb 14, 2019

suneyz commented Feb 14, 2019

suneyz commented Feb 13, 2019 •

edited

Loading

fierlion Feb 14, 2019 •

edited

Loading

aaithal commented Feb 14, 2019 •

edited

Loading