Support e2e test EFS create on EKS clusters by finding EKS node subnets #707

wongma7 · 2022-06-08T20:38:23Z

Is this a bug fix or adding new feature? /bug

What is this PR about? / Why do we need it? The e2e test can run against an EKS cluster if an EFS file system is provided (i.e., --create-file-system=false). However, if an EFS file system is not provided then the e2e test EFS create fails because it fails to find EKS node subnets which are needed for creating EFS mount targets.

I don't know of a tag that is guaranteed to exist on ALL EKS clusters so I am trying


				Name: aws.String("tag:Name"),
				Values: []*string{
					aws.String(fmt.Sprintf("*%s*", clusterName)),
				},

eksctl does tag clusters with alpha.eksctl.io/cluster-name but not all EKS clusters are created by eksctl

What testing is done?

local test TEST_ID=1 CLEAN=false make test-e2e CLUSTER_TYPE=eksctl \ K8S_VERSION="1.20" \ DRIVER_NAME=aws-efs-csi-driver \ HELM_VALUES_FILE="./hack/values_eksctl.yaml" \ CONTAINER_NAME=efs-plugin \ TEST_EXTRA_FLAGS='--cluster-name=$CLUSTER_NAME' \ AWS_REGION=us-west-2 \ AWS_AVAILABILITY_ZONES=us-west-2a,us-west-2b,us-west-2c \ TEST_PATH=./test/e2e/... \ GINKGO_FOCUS="\[efs-csi\]" \ ./hack/e2e/run.sh
CI test EKS (trick CI into executing EKS by modifying Make rule)
CI test Kops (make sure my refactoring didn't break anything)

wongma7 · 2022-06-09T00:23:31Z

EKS test passed

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-efs-csi-driver/707/pull-aws-efs-csi-driver-e2e/1534677627753730048

Ashley-wenyizha · 2022-06-09T18:15:21Z

Dockerfile

@@ -26,9 +26,9 @@ ADD . .
 ARG client_source=k8s
 ENV EFS_CLIENT_SOURCE=$client_source

-RUN GOOS=${TARGETOS} GOARCH=${TARGETARCH} make aws-efs-csi-driver
+RUN OS=${TARGETOS} ARCH=${TARGETARCH} make $TARGETOS/$TARGETARCH


curious the purpose of the change on make here?

i wanted to make it more consistent with fsx and ebs Makefiles so I copied this pattern from there. Specifically I need the make image rule for the e2e test to work beacuse the e2e test needs to build/push an image to a per-test tag and the current rule was pushing it to hardcoded master tag which is not usable since multijple tests could be running in parallel.

As for why ebs and fsx are this in the first place, it's because ebs must build for windows so I made a bunch of changes to its Makefile to use these generic OS/ARCH env variables. That comes at cost of readability/simplicity since for efs we only care about arm/x64... so I am open to simplifying the Makefile but probably not in the near future 😅

Ashley-wenyizha · 2022-06-09T18:17:34Z

Makefile

+.PHONY: all-push
+all-push:
+	docker buildx build \
+		--no-cache-filter=linux-amazon \


nice to build the no cache! would this be helpful on avoiding unnecessary image cache triggered CVEs?

yes, this is to avoid case where somehow you have cache with older yum packages

nice, I will give it a build and push to my dev account to see this can help sort out most of our CVE mitigation

hack/e2e/eksctl.sh

Ashley-wenyizha · 2022-06-09T18:22:51Z

hack/e2e/eksctl.sh

@@ -68,6 +71,7 @@ function eksctl_create_cluster() {

  if [[ "$WINDOWS" == true ]]; then
    ${BIN} create nodegroup \
+      --managed=false \


reading this flag description, do we change it to false because this is test nodegroups and we do not want to make it show up from the console?

setting --managed=false or using the nodeGroups field creates an unmanaged nodegroup. Bear in mind that unmanaged nodegroups do not appear in the EKS console, which as a general rule only knows about EKS-managed nodegroups.

it's because "Windows is only supported for self-managed (--managed=false flag) nodegroups." (note this is in the WINDOWS clause) . https://eksctl.io/usage/windows-worker-nodes/ . I will add a comment in a follow-up PR for hack/e2e changes

hack/e2e/run.sh

wangnyue

LGTM

k8s-ci-robot · 2022-06-10T18:39:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wangnyue, wongma7

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [wongma7]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wangnyue · 2022-06-10T18:39:50Z

/lgtm

k8s-ci-robot · 2022-06-10T18:39:59Z

@wangnyue: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nckturner · 2022-06-13T18:47:01Z

test/e2e/cloud.go

+			{
+				Name: aws.String("tag:Name"),
+				Values: []*string{
+					aws.String(fmt.Sprintf("*%s*", clusterName)),


Are all the eks clusters created with eksctl?

Another problem is that you may get more subnets than expected. In the event that you have cluster1 and cluster10, you will get additional subnets when looking for those that belong to cluster1.

in GitHub CI, yes.

however I made this more generic because I also have clusters created with other tools/scripts

Updated:

first, call getEksctlSubnetIds. eksctl gives us reliable way to find the subnets "tag:alpha.eksctl.io/cluster-name"

second, call getEksCloudFormationSubnetIds. this is necessarily very generic since the name could be anything but for now i narrowed it to "%s-*", clusterName because that's how I name mine

Ashley-wenyizha · 2022-06-14T13:05:25Z

/lgtm

k8s-ci-robot · 2022-06-14T13:05:35Z

@Ashley-wenyizha: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nckturner · 2022-06-14T17:59:24Z

/lgtm

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 8, 2022

k8s-ci-robot requested review from jqmichael and nckturner June 8, 2022 20:38

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2022

wongma7 mentioned this pull request Jun 8, 2022

Removing Dependency on IMDS, allowing hostNetwork: true to be removed #681

Merged

wongma7 added 2 commits June 8, 2022 14:11

Update hack/e2e

7e62baa

Support e2e test EFS create on EKS clusters by finding EKS node subnets

9b8e67c

wongma7 force-pushed the ekse2e branch from 20ccd5f to 2323d73 Compare June 8, 2022 21:11

wongma7 added 2 commits June 8, 2022 17:29

Replace obsolete makefile rules with ones copied from fsx/ebs

c75f188

Grant EFS controller IAM policy

76fd6be

wongma7 force-pushed the ekse2e branch from 7cd8f6c to 76fd6be Compare June 9, 2022 00:29

Ashley-wenyizha reviewed Jun 9, 2022

View reviewed changes

hack/e2e/eksctl.sh Show resolved Hide resolved

Ashley-wenyizha reviewed Jun 9, 2022

View reviewed changes

hack/e2e/run.sh Show resolved Hide resolved

wangnyue approved these changes Jun 10, 2022

View reviewed changes

nckturner reviewed Jun 13, 2022

View reviewed changes

Add getEksctlSubnetIds and getEksCloudFormationSubnetIds

8693a3e

k8s-ci-robot assigned nckturner Jun 14, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2022

k8s-ci-robot merged commit f2597bb into kubernetes-sigs:master Jun 14, 2022

Ashley-wenyizha mentioned this pull request Jun 21, 2022

Merge PRs from master branch to branch v1.3 - MergePRstoBranchV1.3 #719

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support e2e test EFS create on EKS clusters by finding EKS node subnets #707

Support e2e test EFS create on EKS clusters by finding EKS node subnets #707

wongma7 commented Jun 8, 2022 •

edited

Loading

wongma7 commented Jun 9, 2022

Ashley-wenyizha Jun 9, 2022

wongma7 Jun 9, 2022

Ashley-wenyizha Jun 9, 2022

wongma7 Jun 9, 2022

Ashley-wenyizha Jun 9, 2022

Ashley-wenyizha Jun 9, 2022 •

edited

Loading

wongma7 Jun 9, 2022

wangnyue left a comment

k8s-ci-robot commented Jun 10, 2022

wangnyue commented Jun 10, 2022

k8s-ci-robot commented Jun 10, 2022

nckturner Jun 13, 2022

nckturner Jun 13, 2022

wongma7 Jun 13, 2022

wongma7 Jun 13, 2022

Ashley-wenyizha commented Jun 14, 2022

k8s-ci-robot commented Jun 14, 2022

nckturner commented Jun 14, 2022

Support e2e test EFS create on EKS clusters by finding EKS node subnets #707

Support e2e test EFS create on EKS clusters by finding EKS node subnets #707

Conversation

wongma7 commented Jun 8, 2022 • edited Loading

wongma7 commented Jun 9, 2022

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

Ashley-wenyizha Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangnyue left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 10, 2022

wangnyue commented Jun 10, 2022

k8s-ci-robot commented Jun 10, 2022

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

Ashley-wenyizha commented Jun 14, 2022

k8s-ci-robot commented Jun 14, 2022

nckturner commented Jun 14, 2022

wongma7 commented Jun 8, 2022 •

edited

Loading

Ashley-wenyizha Jun 9, 2022 •

edited

Loading