Modifies integration tests to be runnable outside of AWS #667

BennettJames · 2022-12-22T06:05:08Z

This patch reworks the test and CI system for integration tests to make the execution of the tests more platform agnostic. A system to manage and inject credentials is added that makes the tests runnable outside of an AWS context, and substantial adaptations are made to the test setup and runtime to make them run on a pure github action or linux environment.

Example integration test run: https://github.com/BennettJames/aws-app-mesh-controller-for-k8s/actions/runs/3756588966/jobs/6382826436
Example beta release run: https://github.com/BennettJames/aws-app-mesh-controller-for-k8s/actions/runs/3760250999/jobs/6391541156

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

BennettJames · 2022-12-22T06:06:34Z

.github/workflows/beta-release.yaml

+  IMAGE: "${{ secrets.CI_AWS_ACCOUNT }}.dkr.ecr.us-west-2.amazonaws.com/amazon/appmesh-controller"
+  IMAGE_TAG: "${{ github.event.inputs.tag }}"
+  IMAGE_TAG_AMD: "${{ github.event.inputs.tag }}-linux_amd64"
+  IMAGE_TAG_ARM: "${{ github.event.inputs.tag }}-linux_arm64"


Quick note on the variables here: before merging, I'd want to standardize on using just the beta account here. I'd like to get the patch mostly approved / reviewed, then create the appropriate roles in the beta account, then move over.

BennettJames · 2022-12-22T06:08:56Z

.github/actions/integration-test/action.yaml

@@ -0,0 +1,78 @@
+name: "Run Integration Test"


This is a more elaborate integration test setup that runs on plain ubuntu github actions. The setup behavior is much more complex than before (or rather - the complexity is explicit, rather than having been done out of sight on ec2 runners), and has been moved to a separate action so as to be callable from multiple locations.

BennettJames · 2022-12-22T06:10:01Z

.github/workflows/beta-release.yaml

+            --platform linux/arm64 \
+            --build-arg GOPROXY="$GOPROXY" \
+            -t "${IMAGE}:${IMAGE_TAG_ARM}" \
+            . --push


I had some earlier logic that did a proper multiarch deploy, but took it out to avoid the already rather broad scope. I can dial back the logic even further if desired to make this look more like the original action.

BennettJames · 2022-12-22T06:11:39Z

Dockerfile

+COPY apis/ ./apis/
+COPY controllers/ ./controllers/
+COPY mocks/ ./mocks/
+COPY webhooks/ ./webhooks/


This wasn't strictly related, but I tweaked this dockerfile to be mildly more performant under some circumstances. Particularly: it uses the host build architecture for the building (which leads to fewer total actions in cross-building scenarios), and narrowed the scope of copied files to avoid spurious rebuilds.

BennettJames · 2022-12-22T06:14:35Z

test/framework/manifest/core_resources.go

+	// If set, will not enable ipv6 on any of the pods. This is needed to handle
+	// testing on github actions, where ipv6 is not supported -
+	// https://github.com/actions/runner-images/issues/668.
+	DisableIPv6 bool


This was a pretty annoying last minute surprise. I'm not happy with how that turned out - there's a lot of ugly logic in here to force ipv6 off in the integration tests to make it run on github actions. Otherwise, any test involving an actual envoy will hang during the proxyinit step displaying any error.

I'm tempted to add a separate flag to the controller itself to disable ipv6 on envoys. I'm worried that any additional work on the tests will result in future developers hitting the same problem, whereas a hard off switch for ipv6 in the controller itself would be more robust.

I think both flag or hard off switch are acceptable. Since (I imagine) other customers have hit this ipv6 problem, what do you think about documenting this issue in a readme soemwhere?

I think I may introduce a single flag w/ docs in a follow-up patch.

BennettJames · 2022-12-22T06:17:16Z

scripts/test-with-kind.sh

+      --set "env.AWS_SESSION_TOKEN='$AWS_SESSION_TOKEN'" \
+      --set sidecar.envoyAwsAccessKeyId="$AWS_ACCESS_KEY_ID" \
+      --set sidecar.envoyAwsSecretAccessKey="$AWS_SECRET_ACCESS_KEY" \
+      --set sidecar.envoyAwsSessionToken="$AWS_SESSION_TOKEN"


Quick note on the permissions here:

This forwards the same set of credentials to two different locations: one, to the environment on the controller itself, and two, to the environment on the envoy containers. This could be finer grained - the envoy's need narrower permissions than the controller. This is needed as the runners lack the same AWS setup that let credentials be bootstrapped off the environment / metadata service.

I'm not thrilled by the system I added, and am very open to criticisms / suggestions here.

I think this looks reasonable to me for now. Since you have the credentials as separate here, we can always introduce another role later.

BennettJames · 2022-12-22T06:18:10Z

scripts/test-with-kind.sh

+    kubectl get pod -n appmesh-system
+
+    echo -n "running integration test type $__test ... "
+    ginkgo --flakeAttempts=2 -v -r $__test_dir -- \


I never like doing this, but I added an auto-retry to tests here w/ the flakeAttempts flag. At present, the test suite is just not super robust, and this increases the success rate considerably.

bendu

I'm really happy to see this PR. Nice work!

bendu · 2022-12-22T21:14:30Z

scripts/test-with-kind.sh

+      --set "env.AWS_SESSION_TOKEN='$AWS_SESSION_TOKEN'" \
+      --set sidecar.envoyAwsAccessKeyId="$AWS_ACCESS_KEY_ID" \
+      --set sidecar.envoyAwsSecretAccessKey="$AWS_SECRET_ACCESS_KEY" \
+      --set sidecar.envoyAwsSessionToken="$AWS_SESSION_TOKEN"


I think this looks reasonable to me for now. Since you have the credentials as separate here, we can always introduce another role later.

bendu · 2022-12-22T21:15:22Z

test/Readme.md

@@ -60,5 +60,13 @@ ginkgo -v -r test/integration/virtualnode/ -- --cluster-kubeconfig=/Users/xxxx/.
 In case of failures, refer to [Troubleshooting](https://github.com/aws/aws-app-mesh-controller-for-k8s/blob/master/docs/guide/troubleshooting.md) guide.   


+### integration test suite with kind


Thanks for adding instructions here. This will really help out the next person

bendu · 2022-12-22T21:18:47Z

test/framework/manifest/core_resources.go

+	// If set, will not enable ipv6 on any of the pods. This is needed to handle
+	// testing on github actions, where ipv6 is not supported -
+	// https://github.com/actions/runner-images/issues/668.
+	DisableIPv6 bool


I think both flag or hard off switch are acceptable. Since (I imagine) other customers have hit this ipv6 problem, what do you think about documenting this issue in a readme soemwhere?

bendu · 2022-12-22T21:20:50Z

.github/actions/integration-test/action.yaml

+  vpc_id:
+    description: "aws vpc id to use for the test"
+    required: true
+  account_id:


How are the inputs passed in? Will the account_id and role still be marked as secret?

Yes - all inputs are elided in the same way (I think secrets are hidden based off of just pattern matching, so the origin doesn't really matter). You can see this in the raw output.

This patch reworks the test and CI system for integration tests to make the execution of the tests more platform agnostic. A system to manage and inject credentials is added that makes the tests runnable outside of an AWS context, and substantial adaptations are made to the test setup and runtime to make them run on a pure github action or linux environment.

BennettJames commented Dec 22, 2022

View reviewed changes

BennettJames force-pushed the master branch 2 times, most recently from 3db0a61 to e24509b Compare December 22, 2022 19:38

BennettJames requested review from bendu, joesbigidea and rakeb December 22, 2022 19:45

bendu approved these changes Dec 22, 2022

View reviewed changes

bendu requested a review from sshver December 22, 2022 21:26

BennettJames force-pushed the master branch from e24509b to 51f6b1a Compare December 23, 2022 00:38

BennettJames merged commit a1322ac into aws:master Dec 23, 2022

This was referenced Dec 23, 2022

[DRAFT] New integration test workflow #636

Closed

[DRAFT] Move integration tests to EKS #640

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifies integration tests to be runnable outside of AWS #667

Modifies integration tests to be runnable outside of AWS #667

BennettJames commented Dec 22, 2022 •

edited

Loading

BennettJames Dec 22, 2022

BennettJames Dec 22, 2022

BennettJames Dec 22, 2022

BennettJames Dec 22, 2022

BennettJames Dec 22, 2022

bendu Dec 22, 2022

BennettJames Dec 23, 2022

BennettJames Dec 22, 2022

bendu Dec 22, 2022

BennettJames Dec 22, 2022

bendu left a comment

bendu Dec 22, 2022

bendu Dec 22, 2022

bendu Dec 22, 2022

bendu Dec 22, 2022

BennettJames Dec 22, 2022

		@@ -60,5 +60,13 @@ ginkgo -v -r test/integration/virtualnode/ -- --cluster-kubeconfig=/Users/xxxx/.
		In case of failures, refer to [Troubleshooting](https://github.com/aws/aws-app-mesh-controller-for-k8s/blob/master/docs/guide/troubleshooting.md) guide.


		### integration test suite with kind

Modifies integration tests to be runnable outside of AWS #667

Modifies integration tests to be runnable outside of AWS #667

Conversation

BennettJames commented Dec 22, 2022 • 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

bendu left a 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

BennettJames commented Dec 22, 2022 •

edited

Loading