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.

Sign up for GitHub

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

Jump to bottom

start-build from-webhook: use canonical hostport to compare with config #10836

Merged
merged 1 commit into from
Sep 28, 2016
Merged

start-build from-webhook: use canonical hostport to compare with config #10836

merged 1 commit into from
Sep 28, 2016

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented Sep 7, 2016

When deciding whether to use the transport from the client config, use a canonical host/port string to compare the webhook URL with the config address.

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

@csrwng
Copy link
Contributor Author

csrwng commented Sep 7, 2016

@bparees ptal
@jwforres fyi

jwforres
jwforres reviewed Sep 7, 2016
View reviewed changes
pkg/cmd/cli/cmd/startbuild.go
@@ -598,7 +599,7 @@ func (o *StartBuildOptions) RunStartBuildWebHook() error {
config, err := o.ClientConfig.ClientConfig()
if err == nil {
if url, _, err := restclient.DefaultServerURL(config.Host, "", unversioned.GroupVersion{}, true); err == nil {
if url.Host == hook.Host && url.Scheme == hook.Scheme {
if netutil.CanonicalAddr(url) == netutil.CanonicalAddr(hook) && url.Scheme == hook.Scheme {
Copy link
Member

Choose a reason for hiding this comment

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

is it possible we were doing the direct Host comparison anywhere else in the cli?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was so proud of my simple fix, and you have to go and make it hard :) I'll take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the only other place I found was here:
https://github.com/openshift/origin/blob/master/pkg/cmd/cli/cmd/login/helpers.go#L28
https://github.com/openshift/origin/blob/master/pkg/cmd/cli/cmd/login/helpers.go#L40

However, I think the config is stored with the canonical address, so it may not be necessary to fix.
@fabianofranz do we have to worry about using the canonical address there?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's normalized: https://github.com/openshift/origin/blob/master/pkg/cmd/cli/cmd/login/loginoptions.go#L101

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx @liggitt

@bparees bparees added this to the 1.3.1 milestone Sep 7, 2016
@bparees
Copy link
Contributor

bparees commented Sep 7, 2016

lgtm, post 3.3.

@bparees bparees self-assigned this Sep 13, 2016
@smarterclayton smarterclayton modified the milestones: 1.3.1, 1.4.0 Sep 19, 2016
@bparees
Copy link
Contributor

bparees commented Sep 21, 2016

[merge]

@bparees
Copy link
Contributor

bparees commented Sep 21, 2016

@csrwng this appears to have bit-rotted:

++ Building go targets for linux/amd64: cmd/openshift cmd/oc
pkg/cmd/cli/cmd/startbuild.go:29:2: cannot find package "k8s.io/kubernetes/third_party/golang/netutil" in any of:
    /data/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/third_party/golang/netutil (vendor tree)
    /usr/lib/golang/src/k8s.io/kubernetes/third_party/golang/netutil (from $GOROOT)
    /data/src/github.com/openshift/origin/_output/local/go/src/k8s.io/kubernetes/third_party/golang/netutil (from $GOPATH)

@csrwng
Copy link
Contributor Author

csrwng commented Sep 21, 2016

Will update shortly

@csrwng csrwng force-pushed the start_build_fix_tls_transport branch from d1dfbdd to f03483a Compare September 21, 2016 15:17
@csrwng
Copy link
Contributor Author

csrwng commented Sep 21, 2016

updated package name

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@csrwng
Copy link
Contributor Author

csrwng commented Sep 21, 2016

Flake #9457
[test]

@bparees
Copy link
Contributor

bparees commented Sep 23, 2016

flake #11016
[merge]

@csrwng
Copy link
Contributor Author

csrwng commented Sep 23, 2016

#11016
[merge]

@bparees
Copy link
Contributor

bparees commented Sep 26, 2016

[merge]

@bparees
Copy link
Contributor

bparees commented Sep 26, 2016

flake #11058
[merge]

@bparees
Copy link
Contributor

bparees commented Sep 26, 2016

flake #8427
[merge]

@bparees
Copy link
Contributor

bparees commented Sep 27, 2016

[merge]

On Mon, Sep 26, 2016 at 8:27 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9317/)

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#10836 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEvl3ldJWN171DjQ8h4fWEQAh4gA05rdks5quGMBgaJpZM4J27Ct
.

Ben Parees | OpenShift

@csrwng
Copy link
Contributor Author

csrwng commented Sep 28, 2016

#10987
[merge]
[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to f03483a

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9403/)

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 28, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9403/) (Image: devenv-rhel7_5093)

@bparees
Copy link
Contributor

bparees commented Sep 28, 2016

[merge]

On Wed, Sep 28, 2016 at 1:22 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9409/)

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#10836 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEvl3sqNoDhECgBlBLnbtHETEr_7CKWuks5quqJogaJpZM4J27Ct
.

Ben Parees | OpenShift

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to f03483a

openshift-bot pushed a commit that referenced this pull request Sep 28, 2016
Merge pull request #10836 from csrwng/start_build_fix_tls_transport
dc416a8 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit dc416a8 into openshift:master Sep 28, 2016
@openshift-bot openshift-bot merged commit f03483a into openshift:master Sep 28, 2016
@csrwng csrwng deleted the start_build_fix_tls_transport branch December 21, 2016 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bparees bparees

Labels
None yet
Projects
None yet
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@csrwng @bparees @openshift-bot @liggitt @jwforres @smarterclayton