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

ibmcloud: add support for SSH keys #475

Merged
merged 1 commit into from
Aug 21, 2020
Merged

ibmcloud: add support for SSH keys #475

merged 1 commit into from
Aug 21, 2020

Conversation

erlarese
Copy link
Contributor

@erlarese erlarese commented Aug 5, 2020

This PR adds support to the ibmcloud provider for parsing SSH keys. This applies to the IBM Cloud VPC Gen 2 environment. It does not apply to other aspects of IBM Cloud including the "classic" environment.

This code looks for the vendor-data file in the cidata drive and finds the cloud-config section within that file. A YAML parser is used to pull the SSH keys out from this section.

This is my first time writing any Rust code, so I apologize for anything that's obviously wrong. I have attempted to update the unit tests and verify they still pass. Please let me know if anything else is needed !

@lucab lucab changed the title fix: 472: add SSH key support to ibmcloud provider ibmcloud: add support for SSH keys Aug 5, 2020
@lucab
Copy link
Contributor

lucab commented Aug 5, 2020

@erlarese Thanks for the PR! I currently have a release in progress at #473, so I'll queue the review here after that. Also, the CI is on fire today, so don't worry about that right now.

Before I start with a first pass on this, can you please squash everything in a single commit?

@erlarese
Copy link
Contributor Author

erlarese commented Aug 5, 2020

I attempted to squash all 13 commits into one, but I appear to have done it poorly and somehow pulled in a bunch of other commits. I think I'm just going to create a new fork and start over with a new PR. Oops.

@lucab
Copy link
Contributor

lucab commented Aug 6, 2020

@erlarese as you prefer, but from the current branch status I think it is only missing a rebase on master (which should automatically detect and drop the other commits), see https://git-scm.com/book/en/v2/Git-Branching-Rebasing for guidance.

@erlarese erlarese force-pushed the master branch 2 times, most recently from e232494 to 4fbb6c1 Compare August 6, 2020 13:42
@erlarese
Copy link
Contributor Author

erlarese commented Aug 6, 2020

Thanks Luca for the pointer. I think I hopefully got everything squashed into a single commit and rebased now.

Cargo.toml Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member

This is my first time writing any Rust code, so I apologize for anything that's obviously wrong.

No worries, and thanks for submitting this PR! Personally it took me quite a while to learn Rust but it was also a rewarding process. Luca already went over some things, but in general don't hesitate to reach out with questions and the like on e.g. #forum-coreos on freenode.

Copy link
Contributor Author

@erlarese erlarese left a comment

Choose a reason for hiding this comment

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

Hi Luca, thanks for the feedback! I have attempted to implement everything. Please let me know what you think now. Thanks !

src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Show resolved Hide resolved
@lucab
Copy link
Contributor

lucab commented Aug 11, 2020

@erlarese thanks, great work! I've left a few minor nits and a bunch of suggestions in order to avoid a couple of unwrapping points.
In the failed CI jobs there are a few more suggestions from clippy and rustfmt (you can run those locally too, check Travis config to see how they run).

@erlarese
Copy link
Contributor Author

I'm not sure if this is candidate for merging yet, but please hold off on merging for now. I just built an FCOS image with the latest patches and afterburn is failing to run, I'm still debugging what exactly is wrong.

afterburn-sshkeys@core.service: Main process exited, code=exited, status=1/FAILURE
afterburn-sshkeys@core.service: Failed with result 'exit-code'.

@lucab
Copy link
Contributor

lucab commented Aug 12, 2020

@erlarese no hurry. A journalctl -u afterburn-sshkeys@core.service should you all the details.
However, when trying to investigate issues, I usually do a debug build (which produces a larger binary, with all symbols and full debugging on) and run it manually on the host.

@erlarese
Copy link
Contributor Author

Hi @lucab , thanks for the tip. The problem was that the IBM Cloud VPC control plane was generating a MIME vendor-data file that actually contained two sub sections with the cloud-config header. The switch to mailparse and serde caused it to match on the second cloud-config section which doesn't contain the SSH keys. My unit test passed anyway because I had used an abbreviated version of the vendor-data file to keep the UT smaller.

I fixed this by adding an additional check for a cloud-config section with ssh_authorized_keys inside, and I also expanded the UT to a full actual vendor-data file generated by the IBM Cloud (with SSH keys snipped).

With these updates, it appears the unit tests pass, the linters pass, and an FCOS image built with this boots successfully on the IBM Cloud and I can connect via SSH.

Please re-review and let me know if anything else is needed, thanks !

src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@erlarese erlarese left a comment

Choose a reason for hiding this comment

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

Hi @lucab, I believe I've made the requested changes.

One confusing thing is the linter is failing saying the import of use std::fs is unused, but it's pretty clearly used in that file to read the new fixture, and if I remove the import the code doesn't compile because fs can't be found. I tried a few variations of prefixing the read call with std:: and such, but I can't quite figure out why the linter thinks this import isn't used when the compiler seems to require it. Any idea on how to fix this?

src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
src/providers/ibmcloud/mod.rs Outdated Show resolved Hide resolved
@erlarese
Copy link
Contributor Author

Ah, ignore the earlier comment about the import error, I realized the unit tests actually have a separate dependency declaration and moving the import down there seems to have fixed the issue.

@lucab
Copy link
Contributor

lucab commented Aug 17, 2020

@erlarese thanks, code now looks overall fine to me. Can you please squash everything into a single commit, so that I'll do the final pass?

This allows IBM Cloud VPC Gen 2 SSH keys to be parsed
by Afterburn.

Fixes coreos#472
@erlarese
Copy link
Contributor Author

Hi @lucab -- the commits should be squashed back into one again. Please let me know if anything else needs to be changed!

@lucab
Copy link
Contributor

lucab commented Aug 21, 2020

LGTM, thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants