Add SSH support for Embedded Ansible repositories #19108

Fryguy · 2019-08-06T21:16:28Z

The code here involved a bit of an overhaul of the credentials handling inside GitWorktree, but I think it's a lot cleaner this way. Basically, we wrap credential usage in a with block that creates the key as a Tempfile, does the operation and then deletes the key. For user/pass, we still use the block format, but nothing actually happens.

Note that the only mechanism provided by rugged seems to be SSH keys as files, hence the use of Tempfile. libgit2 seems to have support for SSH key in memory, but it's not exposed via rugged from what I can tell.

I think as a follow up we may want to either a) expose that ssh key in memory cred in rugged, or b) allow for IO objects in addition to filename Strings for the privatekey and publickey options.

@carbonin @NickLaMuro Please review.

Fryguy · 2019-08-06T21:18:22Z

Also note that this does not yet support the ssh user@host:path format. Right now it's just ssh://[user@]host/path. Adding the former I think will require more complicated handling of the url validation, and I'd rather do that iteratively on a follow up PR.

app/models/git_repository.rb

Fryguy · 2019-08-06T21:21:57Z

lib/git_worktree.rb

+    @remote_name = 'origin'
+    @base_name   = File.basename(@path)
+
+    if @ssh_private_key && !Rugged.features.include?(:ssh)


In our appliance builds I've already talked to @simaishi about getting SSH support for rugged and it's relatively simple. If a local developer doesn't compile rugged with ssh support this gives them a nice message.

FWIW, the command I used to build it locally on a Mac is:

brew install libssh2 PKG_CONFIG_PATH="$PKG_CONFIG_PATH:/usr/local/opt/openssl/lib/pkgconfig" gem install rugged -v 0.27.7

On Fedora/CentOS, the presence of libssh2 seems to be enough to get it to build properly.

We will have to document this somewhere in the dev setup. Even so, a "normal" installation of rugged will still work for user/pass over http(s)...just ssh won't work.

Reference to that change in the appliance:

ManageIQ/manageiq-appliance-build#336

Fryguy · 2019-08-06T21:29:44Z

Also, also, known issue...if you change the scm_url (particularly from user/pass to ssh) we need to somehow push that value down to the on-disk-repo itself...another follow-up

Fryguy · 2019-08-06T22:07:54Z

Leaving the rubocops as they were there before.

NickLaMuro

Overall, don't have a big problem with the changes and direction of them. Just have a few minor things that should be addressed first.

app/models/git_repository.rb

NickLaMuro · 2019-08-06T22:57:35Z

lib/git_worktree.rb

+    @remote_name = 'origin'
+    @base_name   = File.basename(@path)
+
+    if @ssh_private_key && !Rugged.features.include?(:ssh)


Reference to that change in the appliance:

ManageIQ/manageiq-appliance-build#336

NickLaMuro · 2019-08-06T22:59:20Z

lib/git_worktree.rb

    @certificate_check_cb = options[:certificate_check]
+
+    @remote_name = 'origin'
+    @base_name   = File.basename(@path)


BUT WHY DIDN'T YOU ALIGN THESE WITH THE REST!!!1!

That's why I put the blank line :) IMO, there first few are "extracting options" and the rest are "other" so they are logically separate groups.

lib/git_worktree.rb

NickLaMuro · 2019-08-06T23:13:17Z

lib/git_worktree.rb

+    yield options
+  ensure
+    if @ssh_private_key_file
+      @ssh_private_key_file.unlink


I don't think I have a better way of doing this, but effectively for every action that requires this method, we are going to be creating and deleting the @ssh_private_key_file, correct? Might be a bit of extra trashing of the file system as a result.

Yeah, but only for remote interactions, so it only happens once per invocation. I thought the same as you that it might thrash but I think we can deal with that later if it's really an issue.

Yeah, kinda assumed. I am not going to "resolve this" as I do want it more visible for others in the future, but I think it can stay as is until it becomes a problem.

Fryguy · 2019-08-08T19:00:16Z

@NickLaMuro Updated.

pull is a safer method as it does proper locking when multiple processes are involved.

Fryguy · 2019-08-08T20:33:17Z

Updated specs and added some more

miq-bot · 2019-08-08T20:35:46Z

Checked commits Fryguy/manageiq@51faaf4~...f1c707d with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 2 offenses detected

lib/git_worktree.rb

❗ - Line 395, Col 77 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.xmlschema, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.httpdate, Time.now.to_i, Time.now.to_f instead.

spec/lib/git_worktree_spec.rb

❗ - Line 334, Col 14 - Style/BlockDelimiters - Avoid using {...} for multi-line blocks.

carbonin

👍 looks good to me. Kicked the specs for brakeman. Looks like they're still having some network issues.

Add SSH support for Embedded Ansible repositories (cherry picked from commit 727acba)

simaishi · 2019-08-08T21:55:47Z

Ivanchuk backport details:

$ git log -1
commit 1ee8dee69fef8aa977352cd055bfbf7b8fc9e1d9
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Thu Aug 8 17:17:49 2019 -0400

    Merge pull request #19108 from Fryguy/embedded_ansible_ssh_support
    
    Add SSH support for Embedded Ansible repositories
    
    (cherry picked from commit 727acba010a39c9b4467afce69614f6a90fe7ec8)

Fryguy added core/embedded ansible enhancement labels Aug 6, 2019

Fryguy commented Aug 6, 2019

View reviewed changes

app/models/git_repository.rb Show resolved Hide resolved

Fryguy commented Aug 6, 2019

View reviewed changes

Fryguy force-pushed the embedded_ansible_ssh_support branch from 567bb72 to 0f7e1d7 Compare August 6, 2019 22:07

NickLaMuro suggested changes Aug 6, 2019

View reviewed changes

Fryguy force-pushed the embedded_ansible_ssh_support branch from 0f7e1d7 to ab10b9b Compare August 8, 2019 18:58

NickLaMuro approved these changes Aug 8, 2019

View reviewed changes

Fryguy force-pushed the embedded_ansible_ssh_support branch from ab10b9b to 414d473 Compare August 8, 2019 20:17

Fryguy added 2 commits August 8, 2019 16:33

Add SSH support for Embedded Ansible repositories

51faaf4

Use pull instead of fetch_and_merge

f1c707d

pull is a safer method as it does proper locking when multiple processes are involved.

Fryguy force-pushed the embedded_ansible_ssh_support branch from 414d473 to f1c707d Compare August 8, 2019 20:33

carbonin approved these changes Aug 8, 2019

View reviewed changes

carbonin self-assigned this Aug 8, 2019

carbonin added changelog/yes core ivanchuk/yes labels Aug 8, 2019

carbonin merged commit 727acba into ManageIQ:master Aug 8, 2019

carbonin added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 8, 2019

simaishi pushed a commit that referenced this pull request Aug 8, 2019

Merge pull request #19108 from Fryguy/embedded_ansible_ssh_support

1ee8dee

Add SSH support for Embedded Ansible repositories (cherry picked from commit 727acba)

simaishi added ivanchuk/backported and removed ivanchuk/yes labels Aug 8, 2019

Fryguy deleted the embedded_ansible_ssh_support branch August 9, 2019 15:26

NickLaMuro mentioned this pull request Feb 28, 2020

[GitWorktree] Add instructions for ssh support #19889

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSH support for Embedded Ansible repositories #19108

Add SSH support for Embedded Ansible repositories #19108

Fryguy commented Aug 6, 2019

Fryguy commented Aug 6, 2019

Fryguy Aug 6, 2019

NickLaMuro Aug 6, 2019

Fryguy commented Aug 6, 2019

Fryguy commented Aug 6, 2019

NickLaMuro left a comment •

edited

Loading

NickLaMuro Aug 6, 2019

NickLaMuro Aug 6, 2019

Fryguy Aug 8, 2019

NickLaMuro Aug 6, 2019

Fryguy Aug 8, 2019

NickLaMuro Aug 8, 2019

Fryguy commented Aug 8, 2019

Fryguy commented Aug 8, 2019

miq-bot commented Aug 8, 2019

carbonin left a comment

simaishi commented Aug 8, 2019

Add SSH support for Embedded Ansible repositories #19108

Add SSH support for Embedded Ansible repositories #19108

Conversation

Fryguy commented Aug 6, 2019

Fryguy commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Aug 6, 2019

Fryguy commented Aug 6, 2019

NickLaMuro left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Aug 8, 2019

Fryguy commented Aug 8, 2019

miq-bot commented Aug 8, 2019

carbonin left a comment

Choose a reason for hiding this comment

simaishi commented Aug 8, 2019

NickLaMuro left a comment •

edited

Loading