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

Allow users to specify a proxy for git repositories #19132

Merged
merged 6 commits into from
Aug 14, 2019

Conversation

carbonin
Copy link
Member

@carbonin carbonin commented Aug 9, 2019

This PR uses my changes to rugged (libgit2/rugged#808) to allow users to configure a git repository proxy url.

This also builds on ManageIQ/manageiq-schema#406 which moved the embedded ansible proxy options to a top level setting so it could apply to all git repositories. We did this because it made it much easier to get the proxy options to the worktree instance from the GitRepository model. Without the settings change we would have to make the proxy settings conditional on whether or not the git repository instance existed because it was part of an embedded ansible configuration script source.

Plus automate domains get proxy support for free 😄

https://bugzilla.redhat.com/show_bug.cgi?id=1735712

TODO:

  • Add more worktree specs
  • Add git repository specs
  • Test this for real

lib/git_worktree.rb Outdated Show resolved Hide resolved
@carbonin carbonin force-pushed the add_git_repo_proxy_url branch from 59f9cd2 to 465053a Compare August 12, 2019 21:01
Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

Minor change requested (just for the PR description):

Possibly include a brief mention as to why we are using our own fork of rugged instead of waiting for upstream to merge, just so this is searchable in the future and there is place documented as to the origination of this decision being made. I understand why, but other possibly doing some "git spelunking" in the future might not, and not everyone will be searching commit messages for this info either.

@NickLaMuro
Copy link
Member

This pull request is not mergeable. Please rebase and repush.

Woops... my guess is this was my PR... 😄

Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

Since I think I know the answer to the two questions I asked, I am going to just approve this PR.

Looks good to me. Nice work!

lib/git_worktree.rb Show resolved Hide resolved
lib/git_worktree.rb Show resolved Hide resolved
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM...just need to clean up the rubocops.

This gives us the proxy_url option for clone_at and other
remote operations

https://bugzilla.redhat.com/show_bug.cgi?id=1735712
@carbonin carbonin force-pushed the add_git_repo_proxy_url branch from 465053a to ad9b2b2 Compare August 13, 2019 19:33
@Fryguy
Copy link
Member

Fryguy commented Aug 13, 2019

Just waiting on rubocops and the "Test this for real"

@carbonin
Copy link
Member Author

Status here is I'm trying to test this with something similar to the ssh tunnel proxies described in the original proxy BZ https://bugzilla.redhat.com/show_bug.cgi?id=1475954

I was able to get the proxy info saved, but didn't provide a scheme which caused a segfault. Probably need a check for a valid url in the rugged code.

After getting past that, the request out to the proxy failed and hung on the first call. Still trying to work out what's happening there.

@carbonin
Copy link
Member Author

I was able to get this to work by setting up a simple squid proxy server. The ssh (socks5) version isn't going to work for a variety of reasons.

But while testing that I noticed that the ssh-style repo urls raise an interesting error when we provide a proxy:

irb(main):006:0> Rugged::Repository.clone_at("[email protected]:ManageIQ/container-ruby.git", "./things", :proxy_url => "http://192.168.122.203:3128/")
Traceback (most recent call last):
       13: from bin/rails:4:in `<main>'
       12: from bin/rails:4:in `require'
       11: from railties (5.1.7) lib/rails/commands.rb:16:in `<top (required)>'
       10: from railties (5.1.7) lib/rails/command.rb:44:in `invoke'
        9: from railties (5.1.7) lib/rails/command/base.rb:63:in `perform'
        8: from thor (0.19.4) lib/thor.rb:369:in `dispatch'
        7: from thor (0.19.4) lib/thor/invocation.rb:126:in `invoke_command'
        6: from thor (0.19.4) lib/thor/command.rb:27:in `run'
        5: from railties (5.1.7) lib/rails/commands/console/console_command.rb:97:in `perform'
        4: from railties (5.1.7) lib/rails/commands/console/console_command.rb:17:in `start'
        3: from railties (5.1.7) lib/rails/commands/console/console_command.rb:62:in `start'
        2: from (irb):6
        1: from (irb):6:in `clone_at'
Rugged::SshError (authentication required but no callback set)

Assuming we're not going to hold this for authentication support, I figure we should only use the proxy for http(s) based repos, right?

@carbonin carbonin force-pushed the add_git_repo_proxy_url branch from ad9b2b2 to 12fd936 Compare August 14, 2019 21:03
This commit also adds a path setting to the proxy url because
without it the url validation was failing in libgit2. It wants
at least a trailing slash.

https://bugzilla.redhat.com/show_bug.cgi?id=1735712
This causes an authentication error. Specifically:

Rugged::SshError (authentication required but no callback set)

https://bugzilla.redhat.com/show_bug.cgi?id=1735712
@carbonin carbonin force-pushed the add_git_repo_proxy_url branch from 6330383 to b1a6d95 Compare August 14, 2019 21:18
@carbonin carbonin changed the title [WIP] Allow users to specify a proxy for git repositories Allow users to specify a proxy for git repositories Aug 14, 2019
@carbonin carbonin removed the wip label Aug 14, 2019
@miq-bot
Copy link
Member

miq-bot commented Aug 14, 2019

Checked commits carbonin/manageiq@c6bce7a~...b1a6d95 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. ⭐

@carbonin
Copy link
Member Author

Removed WIP. This does what I feel like is the MVP to satisfy the regression in the timeframe we have.

Follow-up work to here and rugged itself would be needed to support credentials and ssh-style repo urls.

@Fryguy Fryguy merged commit cdbb3e7 into ManageIQ:master Aug 14, 2019
@Fryguy Fryguy added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 14, 2019
@Fryguy
Copy link
Member

Fryguy commented Aug 14, 2019

Yeah, we can support proxy + https right now...I'm not sure what callback they are looking for that we haven't already provided.

@carbonin carbonin deleted the add_git_repo_proxy_url branch August 15, 2019 13:05
simaishi pushed a commit that referenced this pull request Aug 16, 2019
Allow users to specify a proxy for git repositories

(cherry picked from commit cdbb3e7)

https://bugzilla.redhat.com/show_bug.cgi?id=1735712
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 7ae56390e57907c5b250d8ec369981c026f26bba
Author: Jason Frey <[email protected]>
Date:   Wed Aug 14 17:57:39 2019 -0400

    Merge pull request #19132 from carbonin/add_git_repo_proxy_url
    
    Allow users to specify a proxy for git repositories
    
    (cherry picked from commit cdbb3e781350718a6dfddc40bf1884bcca1ad924)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1735712

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.

5 participants