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 test-e2e-external-eks make rule that tests EKS with pod instance metadata disabled. Remove hostNetwork from DaemonSet #907

Merged
merged 6 commits into from
Jun 10, 2021

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented May 27, 2021

Is this a bug fix or adding new feature? test

What is this PR about? / Why do we need it? test for the case where instance metadata is disabled for pods. EKS specifically recommends this. In this case, only pods with hostNetwork true will have access to instance metadata.

To create the environment, use eksctl --disable-pod-imds.

The expected behavior is that in lieu of instance metadata

  • the controller: should get credentials from IAM for SA. This gets configured via eksctl.
  • the node: should get instance info from k8s API. This gets configured by our helm chart. The node should NOT need hostNetwork true anymore because the sole reason that was there was so it could bypass disable-pod-imds to touch instance metadata.

TODO for future:
a test case where instance metadata is totally disabled on instances, not just for pods. hostNetwork is now false for both controller and node, so whether instance metadata is available on host or not should make no difference , but an extra test case won't hurt.

What testing is done?

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 27, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 27, 2021
@coveralls
Copy link

coveralls commented May 27, 2021

Coverage Status

Coverage increased (+0.002%) to 79.578% when pulling 38b0165 on wongma7:metadatatest into ccc060a on kubernetes-sigs:master.

@wongma7
Copy link
Contributor Author

wongma7 commented May 27, 2021

/test pull-aws-ebs-csi-driver-external-test-eks

@wongma7
Copy link
Contributor Author

wongma7 commented Jun 1, 2021

/test pull-aws-ebs-csi-driver-external-test-eks

1 similar comment
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 1, 2021

/test pull-aws-ebs-csi-driver-external-test-eks

@wongma7
Copy link
Contributor Author

wongma7 commented Jun 2, 2021

I0601 23:59:18.405899 1 metadata.go:101] retrieving instance data from ec2 metadata
W0601 23:59:21.746294 1 metadata.go:104] ec2 metadata is not available
I0601 23:59:21.746314 1 metadata.go:112] retrieving instance data from kubernetes api
I0601 23:59:21.747904 1 metadata.go:117] kubernetes api is available

@wongma7 wongma7 changed the title WIP: Add test-e2e-external-eks make rule that tests EKS with pod instance metadata disabled. Remove hostNetwork from DaemonSet Add test-e2e-external-eks make rule that tests EKS with pod instance metadata disabled. Remove hostNetwork from DaemonSet Jun 2, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 2, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 2, 2021

/test pull-aws-ebs-csi-driver-e2e-single-az

@wongma7
Copy link
Contributor Author

wongma7 commented Jun 2, 2021

/test pull-aws-ebs-csi-driver-external-test-eks

@vdhanan
Copy link
Contributor

vdhanan commented Jun 2, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2021
@vdhanan
Copy link
Contributor

vdhanan commented Jun 2, 2021

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2021
@vdhanan
Copy link
Contributor

vdhanan commented Jun 2, 2021

do we need to bump the helm chart version?

@wongma7
Copy link
Contributor Author

wongma7 commented Jun 2, 2021

@vdhanan I think these changes should go in the next new driver + helm chart release, I don't want to release a helm chart yet.

My reasoning: if we release a new helm chart now, then the driver should work even with hostNetwork off and instance metadata off, but it hasn't been tested, the only thing that has been tested is the particular combination of driver + helm chart built in this PR.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Jun 9, 2021

rebased, conflict was hack/values.yaml got logLevel: 5 added to it (to easily debug tests)

@nckturner
Copy link

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2021
@vdhanan
Copy link
Contributor

vdhanan commented Jun 10, 2021

/unhold

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants