Bug 1971332: revert incorrect allowance of ssh:// prefix with scp styled URLs #875

gabemontero · 2021-07-06T22:58:13Z

/hold

for now

want to discuss with @adambkaplan when he returns from PTO

this is a possible reconciliation with the concerns in this BZ with conclusions drawn from https://bugzilla.redhat.com/show_bug.cgi?id=1884270

but I'm not ready to make an executive decsion here

openshift-ci · 2021-07-06T22:58:19Z

@gabemontero: This pull request references Bugzilla bug 1971332, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @xiuwang

In response to this:

Bug 1971332: work around s2i parse url ssh:// prefix removal to allow use of ssh and private keys in git operations

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.

gabemontero · 2021-07-07T13:37:10Z

terraform flake on e2e-aws-builds

/test e2e-aws-builds

gabemontero · 2021-07-12T15:49:58Z

bugs 1973724 1975050 1972374 are all noted in sippy wrt metal ipi ... failures unrelated to this PR

I'll also mention here that adding a new option to oc new-build for end user control of how we manipulate the git url when ssh:// is involved is an option to discuss.

gabemontero · 2021-07-13T15:00:09Z

/retest

adambkaplan

/hold

adambkaplan · 2021-07-14T16:07:57Z

pkg/helpers/newapp/app/app.go

@@ -167,6 +168,9 @@ func (r *SourceRef) BuildSource() (*buildv1.BuildSource, []buildv1.BuildTriggerP
 			URI: r.URL.StringNoFragment(),
 			Ref: r.URL.URL.Fragment,
 		}
+		if r.SSHPrefixWorkaround {
+			source.Git.URI = "ssh://" + source.Git.URI


This feels like a band-aid on top of a thing we broke in s2i.

We should fix s2i so that handles regular SSH URIs (which are valid URIs) and SCP style URIs.

as I put a hold on in the description, and mentioned wanting to discuss this, I agree this is a band aid ... of course your additional hold does not hurt anything :-)

that said, the reason I want to discuss voice to voice in office hours is
a) fixing it this in s2i could prove trickier and more time consuming than we realize ... not 100% sure but I want to talk through it
b) if that pans out, is the band aid sufficient given what we have to do elsewhere

In particular, the wide use of the embedded golang URL inside the S2I URL throughout new-app / new-build code gave me some pause @adambkaplan

And that golang URL still can't deal with ssh://

My hope is bring up the code in an editor, walk through the references during office hours, and see what change makes sense where.

xiuwang · 2021-07-15T02:26:18Z

/bugzilla refresh

openshift-ci · 2021-07-15T02:26:22Z

@xiuwang: This pull request references Bugzilla bug 1971332, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jitendar-singh

In response to this:

/bugzilla refresh

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.

gabemontero · 2021-07-15T14:54:33Z

/hold cancel

OK @adambkaplan the revert of https://bugzilla.redhat.com/show_bug.cgi?id=1884270 plus additional test cases is up.

I'll wait until you approve the "copied s2i" piece here before I make the change in s2i, and once that merges, vendor that into opensift/builder and openshift/openshift-apiserver

gabemontero · 2021-07-15T17:31:49Z

only a sig-network flake on ipi-ovn-ipv6 ... all sig-builds passed @adambkaplan

adambkaplan

/approve

My only question is how we are going to support scp-style git locations in oc. We can do this in a follow-up PR (i.e. move the BZ back to ASSIGNED).

gabemontero · 2021-07-16T00:28:44Z

/retest

adambkaplan

/lgtm

gabemontero · 2021-07-23T14:18:34Z

/assign @soltysh

for an approve from someone in the OWNERS file

thanks

soltysh

/approve

openshift-ci · 2021-07-26T11:53:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, gabemontero, soltysh

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:

~~OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-07-26T13:49:24Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-07-26T16:32:27Z

@gabemontero: All pull requests linked via external trackers have merged:

Bugzilla bug 1971332 has been moved to the MODIFIED state.

In response to this:

Bug 1971332: revert incorrect allowance of ssh:// prefix with scp styled URLs

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.

gabemontero · 2021-08-11T13:10:54Z

/cherrypick release-4.8

openshift-cherrypick-robot · 2021-08-11T13:11:40Z

@gabemontero: new pull request created: #898

In response to this:

/cherrypick release-4.8

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.

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2021

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jul 6, 2021

openshift-ci bot requested review from xiuwang, adambkaplan and coreydaley July 6, 2021 22:58

gabemontero mentioned this pull request Jul 6, 2021

Bug 1971332: add clarifying comment on lack of ssh:// in url not affecting ssh private key setup openshift/builder#251

Closed

adambkaplan reviewed Jul 14, 2021

View reviewed changes

openshift-ci bot requested a review from jitendar-singh July 15, 2021 02:26

revert incorrect allowance of ssh:// prefix with scp styled URLs

9298da9

gabemontero force-pushed the bz1971332 branch from 9cb62e2 to 9298da9 Compare July 15, 2021 14:50

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2021

gabemontero changed the title ~~Bug 1971332: work around s2i parse url ssh:// prefix removal to allow use of ssh and private keys in git operations~~ Bug 1971332: revert incorrect allowance of ssh:// prefix with scp styled URLs Jul 15, 2021

adambkaplan approved these changes Jul 15, 2021

View reviewed changes

adambkaplan approved these changes Jul 23, 2021

View reviewed changes

openshift-ci bot assigned adambkaplan Jul 23, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2021

openshift-ci bot assigned soltysh Jul 23, 2021

soltysh approved these changes Jul 26, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2021

openshift-merge-robot merged commit 1557476 into openshift:master Jul 26, 2021

gabemontero deleted the bz1971332 branch July 26, 2021 21:37

openshift-cherrypick-robot mentioned this pull request Aug 11, 2021

[release-4.8] Bug 1992639: revert incorrect allowance of ssh:// prefix with scp styled URLs #898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1971332: revert incorrect allowance of ssh:// prefix with scp styled URLs #875

Bug 1971332: revert incorrect allowance of ssh:// prefix with scp styled URLs #875

gabemontero commented Jul 6, 2021

openshift-ci bot commented Jul 6, 2021

gabemontero commented Jul 7, 2021

gabemontero commented Jul 12, 2021

gabemontero commented Jul 13, 2021

adambkaplan left a comment

adambkaplan Jul 14, 2021

gabemontero Jul 14, 2021

gabemontero Jul 14, 2021

xiuwang commented Jul 15, 2021

openshift-ci bot commented Jul 15, 2021

gabemontero commented Jul 15, 2021

gabemontero commented Jul 15, 2021

adambkaplan left a comment

gabemontero commented Jul 16, 2021

adambkaplan left a comment

gabemontero commented Jul 23, 2021

soltysh left a comment

openshift-ci bot commented Jul 26, 2021

openshift-bot commented Jul 26, 2021

openshift-ci bot commented Jul 26, 2021

gabemontero commented Aug 11, 2021

openshift-cherrypick-robot commented Aug 11, 2021

Bug 1971332: revert incorrect allowance of ssh:// prefix with scp styled URLs #875

Bug 1971332: revert incorrect allowance of ssh:// prefix with scp styled URLs #875

Conversation

gabemontero commented Jul 6, 2021

openshift-ci bot commented Jul 6, 2021

gabemontero commented Jul 7, 2021

gabemontero commented Jul 12, 2021

gabemontero commented Jul 13, 2021

adambkaplan left a comment

Choose a reason for hiding this comment

adambkaplan Jul 14, 2021

Choose a reason for hiding this comment

gabemontero Jul 14, 2021

Choose a reason for hiding this comment

gabemontero Jul 14, 2021

Choose a reason for hiding this comment

xiuwang commented Jul 15, 2021

openshift-ci bot commented Jul 15, 2021

gabemontero commented Jul 15, 2021

gabemontero commented Jul 15, 2021

adambkaplan left a comment

Choose a reason for hiding this comment

gabemontero commented Jul 16, 2021

adambkaplan left a comment

Choose a reason for hiding this comment

gabemontero commented Jul 23, 2021

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 26, 2021

openshift-bot commented Jul 26, 2021

openshift-ci bot commented Jul 26, 2021

gabemontero commented Aug 11, 2021

openshift-cherrypick-robot commented Aug 11, 2021