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

Fix e2e instructions and add a Docker runner for them #248

Merged
merged 1 commit into from
Jul 29, 2019
Merged

Conversation

snormore
Copy link

@snormore snormore commented Jul 18, 2019

Noticed a couple things that were off in the e2e instructions, and also wanted an isolated way to run this in a Docker image.

@snormore snormore requested a review from timoreimann July 18, 2019 17:24
@snormore snormore force-pushed the e2e-fixes branch 2 times, most recently from 908619d to 180e45f Compare July 18, 2019 17:35
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few gotchas I spotted during testing. I like the idea to containerize the end-to-end tests a lot, great idea. 👏

e2e/Dockerfile Outdated Show resolved Hide resolved
scripts/e2e.sh Outdated Show resolved Hide resolved
scripts/e2e.sh Outdated Show resolved Hide resolved
scripts/e2e.sh Outdated Show resolved Hide resolved
e2e/.gitignore Outdated Show resolved Hide resolved
e2e/e2e.sh Outdated Show resolved Hide resolved
e2e/scripts/setup_cluster.sh Outdated Show resolved Hide resolved
scripts/e2e.sh Outdated Show resolved Hide resolved
scripts/e2e.sh Outdated Show resolved Hide resolved
e2e/Dockerfile Outdated Show resolved Hide resolved
@snormore
Copy link
Author

snormore commented Jul 21, 2019

Comments addressed and should be 👌 for another round of 👀 whenever you get a chance. 🙏

e2e/Dockerfile Outdated Show resolved Hide resolved
e2e/Dockerfile Outdated Show resolved Hide resolved
e2e/docker-run.sh Outdated Show resolved Hide resolved
e2e/docker-run.sh Outdated Show resolved Hide resolved
e2e/e2e.sh Outdated Show resolved Hide resolved
e2e/e2e.sh Outdated Show resolved Hide resolved
e2e/scripts/setup_cluster.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one more comment about the cleanup script and a few more about public/private key handling. But please read on before addressing my comments.

I've been thinking more about how--or rather if--we should handle keys, especially creating them. My latest and greatest thoughts are that we should leave this entirely to the user. I came to this conclusion tracking backwards, here's my train of thought:

  1. Expecting keys to be located under e2e/ (or any version controlled-directory, for that matter) seems worrisome since it's very easy to accidentally commit private keys. While we git-ignore e2e/id_rsa*, we cannot assume that everybody uses that. Even if we were to git-ignore a specific directory, there's still some risk that users put their keys elsewhere and commit them unintentionally.
  2. The directory where keys should be located in is $HOME/.ssh. That's the safest place to, and it's it's also what SSH tooling is tailored for (e.g., to ensure proper file permissions are set). Some users may even want their CCM end-to-end key to be located there.
  3. We definitely should not create keys in $HOME/.ssh from our scripts.

Consequently, I think we should leave it to the user to set up their keys accordingly.

What we could theoretically do is check if the fingerprint is found in the account (via doctl) and bail out if it isn't. That requires being able to read the key from inside the container, which probably means mounting in $HOME/.ssh again as read-only.

Thoughts on that?

e2e/scripts/cleanup-resources.sh Outdated Show resolved Hide resolved
e2e/docker.sh Show resolved Hide resolved
e2e/docker.sh Outdated Show resolved Hide resolved
e2e/docker.sh Outdated Show resolved Hide resolved
e2e/scripts/setup_cluster.sh Outdated Show resolved Hide resolved
e2e/scripts/setup_cluster.sh Outdated Show resolved Hide resolved
e2e/scripts/setup_cluster.sh Outdated Show resolved Hide resolved
e2e/scripts/setup_cluster.sh Outdated Show resolved Hide resolved
e2e/scripts/setup_cluster.sh Outdated Show resolved Hide resolved
@snormore snormore force-pushed the e2e-fixes branch 4 times, most recently from b7a07c9 to ab63f9a Compare July 29, 2019 10:52
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor things we need to revert to account for the change of course in regards to SSH key usage.

Approving already to save one roundtrip.

Thanks a lot @snormore 👏

.gitignore Outdated Show resolved Hide resolved
e2e/README.md Outdated Show resolved Hide resolved
@snormore snormore merged commit e8cf1cc into master Jul 29, 2019
@snormore snormore deleted the e2e-fixes branch July 29, 2019 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants