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

Make SSH path handling consistent across providers during e2e #68753

Merged

Conversation

@johnSchnake
Copy link
Contributor

commented Sep 17, 2018

There were providers which would:

  • allow overrides of the file base name but not the path
  • allow oeverrides of the file path
  • not allow any overrides at all

This change makes it such that each of the supported providers
can override the SSH key location using an env var. The env
var itself may vary based on the provider though.

Fixes: #68747

What this PR does / why we need it:
Makes working with SSH keys on different providers more consistent which makes running tests easier. Allows overriding the path on GCE and also allows overriding the entire path (not just the base name) on local/vsphere/skeleton providers.

Special notes for your reviewer:
This is a slightly breaking change since it does change the interpretation of an env var or two. The problem was that the env vars here were interpreted two different ways already (AWS allowed a path override, local was a base name override but the directory was hardcoded). Rather than create new env vars which would have different meanings on each provider to fill in the gaps, I thought this would be better.

Release note:

action required

If you are running E2E tests which require SSH keys and you utilize environment variables to override their location, you may need to modify the environment variable set. On all providers the environment variable override can now be either an absolute path to the key or a relative path (relative to ~/.ssh). Specifically the changes are:
 - Created new GCE_SSH_KEY allowing specification of SSH keys for gce, gke, and kubemark.
 - AWS_SSH_KEY, previously assumed to be an absolute path can now be either relative or absolute
 - LOCAL_SSH_KEY (for local and vsphere providers) was previously assumed to be a filename relative to ~/.ssh but can now also be an absolute path
 - KUBE_SSH_KEY (for skeleton provider) was previously assumed to be a filename relative to ~/.ssh but can now also be an absolute path
@johnSchnake

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2018

@sig-testing-pr-reviews

@neolit123

This comment has been minimized.

Copy link
Member

commented Sep 17, 2018

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug and removed needs-kind labels Sep 17, 2018
@johnSchnake

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

/assign @rramkumar1

@neolit123

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

/ok-to-test

@johnSchnake

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2018

/test pull-kubernetes-integration

@johnSchnake

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

@neolit123 @rramkumar1 Tests are green; is there anything else needed to achieve lgtm/merge?

@neolit123

This comment has been minimized.

Copy link
Member

commented Sep 30, 2018

the maintainers need to find time to look at this.

@johnSchnake

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

@kubernetes/sig-testing-pr-reviews Has anyone had a chance to look at this?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

@johnSchnake: Reiterating the mentions to trigger a notification:
@kubernetes/sig-testing-pr-reviews

In response to this:

@kubernetes/sig-testing-pr-reviews Has anyone had a chance to look at this?

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.

@ixdy

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

It looks like the notable breaking changes are that LOCAL_SSH_KEY and KUBE_SSH_KEY are now expected to be absolute paths rather than relative paths.

I'm not sure who sets these variables, but that might be problematic. Maybe use filepath.IsAbs to maintain the existing functionality?

@ixdy

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

at the very least, maybe update the release note to point out which providers have changing behavior?

There were providers which would:
 - allow overrides of the file base name but not the path
 - allow oeverrides of the file path
 - not allow any overrides at all

This change makes it such that each of the supported providers
can override the SSH key location using an env var. The env
var itself may vary based on the provider though.

If given an absolute path to the key, it is used. If given
a relative path it will be made relative to ~/.ssh

Fixes: #68747
@johnSchnake johnSchnake force-pushed the johnSchnake:68687-consistentSSHKeyHandling branch from 7450e31 to 266080e Nov 14, 2018
@johnSchnake

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

@ixdy You're right, that logic wasn't awkward to accommodate and actually made it more versatile. Now each provider can support relative or absolute paths and, assuming the users env vars were properly set in the past, then this should still work.

Release notes were updated to specify the exact changes for reach of the providers affected. In every case it was extending existing functionality and now makes them all consistent.

@johnSchnake

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

Do I need to just wait until the v1.13 gate is lifted or does that need to be added by someone?

@ixdy

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

/lgtm
/approve

One of the sig-testing chairs can decide whether this would be appropriate for v1.13. It seems fairly safe, since it maintains backwards compatibility.

/assign @spiffxp @fejta @stevekuznetsov @timothysc

@johnSchnake

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2018

@ixdy Would you mind re-labeling with lgtm/approved? The 1.13 gate has been removed but for some reason that seems to have also caused those labels to be removed too.

@ixdy

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

/lgtm
/approve

I think the webhook was dropped the first time around. :(

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ixdy, johnSchnake

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

@ixdy

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

/retest

2 similar comments
@johnSchnake

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

/retest

@ixdy

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

/retest

@spiffxp

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Dec 5, 2018
@fejta-bot

This comment has been minimized.

Copy link

commented Dec 5, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 82cefcd into kubernetes:master Dec 5, 2018
18 checks passed
18 checks passed
cla/linuxfoundation johnSchnake authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@johnSchnake johnSchnake deleted the johnSchnake:68687-consistentSSHKeyHandling branch Dec 5, 2018
@johnSchnake johnSchnake referenced this pull request Mar 4, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.