introduce no proxy setting for git cloning, with defaulter #10902

bparees · 2016-09-14T08:16:36Z

bug 1375902

Miciah · 2016-09-14T18:43:16Z

pkg/build/api/v1/types.go

@@ -350,6 +350,9 @@ type GitBuildSource struct {

 	// httpsProxy is a proxy used to reach the git repository over https
 	HTTPSProxy *string `json:"httpsProxy,omitempty" protobuf:"bytes,4,opt,name=httpsProxy"`
+
+	// NoProxy is the list of domains for which the proxy should not be used
+	NoProxy *string `json:"noProxy,omitempty" protobuf:"bytes,5,opt,name=httpsProxy"`


Should be name=noProxy, right?

bparees · 2016-09-14T19:27:01Z

yes, fixed, thanks.

On Wed, Sep 14, 2016 at 8:43 PM, Miciah Dashiel Butler Masters <
notifications@github.com> wrote:

@Miciah commented on this pull request.

In pkg/build/api/v1/types.go
#10902 (review):

@@ -350,6 +350,9 @@ type GitBuildSource struct {

// httpsProxy is a proxy used to reach the git repository over https
HTTPSProxy *string json:"httpsProxy,omitempty" protobuf:"bytes,4,opt,name=httpsProxy"
+

// NoProxy is the list of domains for which the proxy should not be used

NoProxy *string json:"noProxy,omitempty" protobuf:"bytes,5,opt,name=httpsProxy"

Should be name=noProxy, right?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#10902 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEvl3kPNRpP6pq_jkb93jKT9gE6TEUo5ks5qqEBNgaJpZM4J8gpZ
.

Ben Parees | OpenShift

bparees · 2016-09-20T07:32:28Z

@openshift/api-review ptal.
@csrwng bump
[test]

csrwng

Just one minor change, and extra credit if you add an extended test here: https://github.com/openshift/origin/blob/master/test/extended/builds/proxy.go

csrwng · 2016-09-20T11:01:49Z

pkg/build/api/v1/types.go

@@ -350,6 +350,9 @@ type GitBuildSource struct {

 	// httpsProxy is a proxy used to reach the git repository over https
 	HTTPSProxy *string `json:"httpsProxy,omitempty" protobuf:"bytes,4,opt,name=httpsProxy"`
+
+	// NoProxy is the list of domains for which the proxy should not be used


s/NoProxy/noProxy/

bparees · 2016-09-20T21:00:46Z

@csrwng TC added and a few other changes including a semi-unrelated change to glog to ensure we get proper line number references printed from the glog wrapper instead of "glog.50:blah blah"

bparees · 2016-09-20T21:00:51Z

[testextended][extended:core(support proxies)]

csrwng · 2016-09-20T21:09:10Z

LGTM

smarterclayton · 2016-09-21T01:40:03Z

pkg/build/api/types.go

@@ -401,6 +401,9 @@ type GitBuildSource struct {

 	// HTTPSProxy is a proxy used to reach the git repository over https
 	HTTPSProxy *string
+
+	// NoProxy is the list of domains for which the proxy should not be used
+	NoProxy *string


Why isn't this just an env var to the build?

if it's a var to the build then:

we have to whitelist it so it can be set on the privileged container

all operations on the privileged container will use that value which makes it riskier(?) and also less granular control over proxy behavior

build env vars get used during assemble which might be undesirable

build env vars get baked into the final application image which might be undesirable

Also the ship has kinda already sailed w/ the httpproxy/httpsproxy fields so it makes sense to me to put the noproxy field in the same place.

bparees · 2016-09-21T07:06:17Z

yum flake [test]

smarterclayton · 2016-09-22T21:10:37Z

You cannot merge things that haven't gotten API approval.

bparees · 2016-09-23T07:02:05Z

You cannot merge things that haven't gotten API approval.

can i haz api approval? i responded to your questions above.

bparees · 2016-09-26T20:07:02Z

@smarterclayton internal api reworked, ptal.

smarterclayton · 2016-09-26T20:52:13Z

pkg/build/api/types.go

-	HTTPSProxy *string
+	// ProxyConfig defines the proxies to use for the git clone operation
+	// +k8s:conversion-gen=false
+	ProxyConfig ProxyConfig


Sorry, I should have been clearer. I expected this to look like CommonSpec and have the same config internal and external with the correct inlining statements. Is the only reason you have conversion-gen=false different is because of that? If so my intent was just to have:

type GitBuildSource struct { ... GitProxyConfig `json:",inline"` } type GitProxyConfig struct { HTTPProxy *string ... }

sigh. and yes that's why i have conversion-gen=false.

bparees · 2016-09-26T21:49:24Z

pkg/build/admission/defaults/api/v1/types.go

@@ -15,6 +15,9 @@ type BuildDefaultsConfig struct {
 	// GitHTTPSProxy is the location of the HTTPSProxy for Git source
 	GitHTTPSProxy string `json:"gitHTTPSProxy,omitempty"`

+	// GitNoProxy is the list of domains for which the proxy should not be used
+	GitNoProxy string `json:"gitNoProxy,omitempty"`
+


@smarterclayton did you want this struct reworked also?

No you don't have to

bparees · 2016-09-29T00:58:00Z

[test]

Ben Parees | OpenShift

On Sep 28, 2016 8:47 PM, "OpenShift Bot" notifications@github.com wrote:

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

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

openshift-bot · 2016-09-29T21:57:27Z

Evaluated for origin testextended up to 38bef0a

openshift-bot · 2016-09-29T21:57:34Z

Evaluated for origin test up to 38bef0a

openshift-bot · 2016-09-29T23:02:23Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/546/) (Extended Tests: core(support proxies))

bparees · 2016-09-29T23:25:22Z

[merge]

openshift-bot · 2016-09-30T19:34:28Z

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

openshift-bot · 2016-09-30T21:22:15Z

Evaluated for origin merge up to 38bef0a

openshift-bot · 2016-09-30T21:22:26Z

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

sdodson · 2016-10-07T13:31:50Z

Worth backporting to 1.3/3.3? Seems we have a few people running into this problem.

ikke-t · 2016-10-07T14:00:11Z

@sdodson can you please do backport asap? Our 3.3. project at customer is stuck due this. Just figured out an hour ago that it's about not getting no_proxy variable to git in builder image.

bparees · 2016-10-07T14:26:40Z

Not in my opinion. If you don't want a proxy used during cloning, don't set
a proxy in your Buildconfig git proxy section, and don't set one in the
build defaulter either.

This feature was specifically for the use case where someone set a global
git proxy defaulter value but wanted to ensure proxying wasn't done for a
certain domains, but was done for most domains.

If you don't have that scenario, you should just not configure a git proxy
default value in the build defaulter.

Ben Parees | OpenShift

On Oct 7, 2016 9:32 AM, "Scott Dodson" notifications@github.com wrote:

Worth backporting to 1.3/3.3? Seems we have a few people running into this
problem.

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

ikke-t · 2016-10-07T14:40:32Z

I recall proxy has been used for decades in a way that you have http(s)_proxy and no_proxy options, and pretty much all programs obey those. Now we are struggling in datacenter behind proxy, where builder images need to access internet via proxy to get all kinds of dependencies. Same with docker images etc. At the same time there is also git server (gogs) and other utility services running on top of OpenShift. So those need to be accessed via direct access, like normally instructed via no_proxy.

Pretty much everything in our setup finally works now via /etc/sysconfig/{docker,atomic-opensift*} and /etc/profile.d/proxy.sh files setting HTTP(s)_PROXY and NO_PROXY, but the builder images fail to git clone from NO_PROXY addresses. This is misfeature, and don't work as expected. All company internal code fails to build due cloning not working from inside the firewall or from OpenShift containers.

ikke

bparees · 2016-10-07T15:31:08Z

Please read my response again. The proxy settings used for git cloning are
specific to git cloning. They have nothing to do with proxy settings for
dependency retrieval or image pulling.

If you don't want git cloning to go through a proxy, don't set a git proxy
value. You don't need this feature backported to solve your problem.

Ben Parees | OpenShift

On Oct 7, 2016 10:40 AM, "Ilkka Tengvall" notifications@github.com wrote:

I recall proxy has been used for decades in a way that you have
http(s)_proxy and no_proxy options, and pretty much all programs obey
those. Now we are struggling in datacenter behind proxy, where builder
images need to access internet via proxy to get all kinds of dependencies.
Same with docker images etc. At the same time there is also git server
(g9gs) and other utility services running on top of OpenShift. So those
need to be accessed via direct access, like normally instructed via
no_proxy.

Pretty much everything in our setup finally works now via
/etc/sysconfig/{docker,atomic-opensift*} and /etc/profile.d/proxy.sh
files setting HTTP(s)_PROXY and NO_PROXY, but the builder images fail to
git clone from NO_PROXY addresses. This is misfeature, and don't work as
expected. All company internal code fails to build.

ikke

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

ikke-t · 2016-10-07T15:42:17Z

OK, thaks I'll try that first thing at monday.

ikke-t · 2016-10-10T07:02:11Z

So now it's monday morning here in UTC+3, and I got to test it. It works. Now internal git builds work, where as the externals fail unless the build config option is manually changed for each such build.

/etc/origin/master/master-config.yaml:

      gitHTTPProxy: ""
      gitHTTPSProxy: ""

Thanks. Next to hunt for similar setting for health checks, which fails next causing restart loops.

smarterclayton · 2017-03-10T19:59:48Z

pkg/build/api/v1/types.go

-	// httpsProxy is a proxy used to reach the git repository over https
-	HTTPSProxy *string `json:"httpsProxy,omitempty" protobuf:"bytes,4,opt,name=httpsProxy"`
+	// proxyConfig defines the proxies to use for the git clone operation
+	ProxyConfig `json:",inline" protobuf:"bytes,3,opt,name=proxyConfig"`


Did we ever ship HTTPProxy and HTTPSProxy in the previous release? I.e. did we break the api between 1.3 and 1.4 here?

We did ship HTTPProxy and HTTPSProxy in the previous release, and they're still there in 1.4 ... the ProxyConfig struct is inline.

smarterclayton · 2017-03-10T20:19:18Z

The protobuf structure changed. Fortunately we weren't storing protobuf. We can't change external structs without ensuring the protobuf structure is the same On Mar 10, 2017, at 3:08 PM, Cesar Wong <notifications@github.com> wrote: *@csrwng* commented on this pull request. ------------------------------ In pkg/build/api/v1/types.go <#10902 (comment)>:

@@ -349,11 +361,8 @@ type GitBuildSource struct {

// ref is the branch/tag/ref to build. Ref string `json:"ref,omitempty" protobuf:"bytes,2,opt,name=ref"` - // httpProxy is a proxy used to reach the git repository over http - HTTPProxy *string `json:"httpProxy,omitempty" protobuf:"bytes,3,opt,name=httpProxy"` - - // httpsProxy is a proxy used to reach the git repository over https - HTTPSProxy *string `json:"httpsProxy,omitempty" protobuf:"bytes,4,opt,name=httpsProxy"` + // proxyConfig defines the proxies to use for the git clone operation + ProxyConfig `json:",inline" protobuf:"bytes,3,opt,name=proxyConfig"` We did ship HTTPProxy and HTTPSProxy in the previous release, and they're still there in 1.4 ... the ProxyConfig struct is inline. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10902 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p3a7UZho3wgmIZ2LgCmWog7x3GhBks5rka24gaJpZM4J8gpZ> .

bparees added the needs-api-review label Sep 14, 2016

Miciah reviewed Sep 14, 2016

View reviewed changes

bparees force-pushed the git_noproxy branch from 03fa246 to 919e3f0 Compare September 14, 2016 19:26

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2016

bparees force-pushed the git_noproxy branch from 919e3f0 to c15848e Compare September 19, 2016 09:44

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2016

csrwng reviewed Sep 20, 2016

View reviewed changes

bparees force-pushed the git_noproxy branch 6 times, most recently from 58ed89c to 5cc9fa6 Compare September 20, 2016 20:58

smarterclayton reviewed Sep 21, 2016

View reviewed changes

bparees force-pushed the git_noproxy branch 2 times, most recently from c4efad1 to a90cde5 Compare September 26, 2016 19:59

bparees force-pushed the git_noproxy branch from a90cde5 to 9e9e94b Compare September 26, 2016 20:08

smarterclayton reviewed Sep 26, 2016

View reviewed changes

bparees force-pushed the git_noproxy branch from 9e9e94b to 1035f14 Compare September 26, 2016 21:36

bparees commented Sep 26, 2016

View reviewed changes

bparees force-pushed the git_noproxy branch from 1035f14 to a9a11fe Compare September 26, 2016 22:08

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2016

bparees force-pushed the git_noproxy branch from 66b5fdd to cbdd84b Compare September 28, 2016 23:25

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2016

bparees force-pushed the git_noproxy branch from cbdd84b to 9493cee Compare September 29, 2016 15:17

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2016

bparees force-pushed the git_noproxy branch 4 times, most recently from b2cf645 to 6636959 Compare September 29, 2016 19:24

introduce no proxy setting for git cloning, with defaulter

38bef0a

bparees force-pushed the git_noproxy branch from 6636959 to 38bef0a Compare September 29, 2016 21:54

openshift-bot merged commit 82f1067 into openshift:master Sep 30, 2016

bparees deleted the git_noproxy branch October 13, 2016 19:33

smarterclayton reviewed Mar 10, 2017

View reviewed changes

introduce no proxy setting for git cloning, with defaulter #10902

introduce no proxy setting for git cloning, with defaulter #10902

Conversation

bparees commented Sep 14, 2016 • edited

Choose a reason for hiding this comment

bparees commented Sep 14, 2016

@Miciah commented on this pull request.

bparees commented Sep 20, 2016

csrwng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Sep 20, 2016

bparees commented Sep 20, 2016

csrwng commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Sep 21, 2016

smarterclayton commented Sep 22, 2016

bparees commented Sep 23, 2016

bparees commented Sep 26, 2016

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

bparees commented Sep 29, 2016

openshift-bot commented Sep 29, 2016

openshift-bot commented Sep 29, 2016

openshift-bot commented Sep 29, 2016

bparees commented Sep 29, 2016

openshift-bot commented Sep 30, 2016

openshift-bot commented Sep 30, 2016

openshift-bot commented Sep 30, 2016 • edited

sdodson commented Oct 7, 2016

ikke-t commented Oct 7, 2016

bparees commented Oct 7, 2016

ikke-t commented Oct 7, 2016 • edited

bparees commented Oct 7, 2016

ikke-t commented Oct 7, 2016

ikke-t commented Oct 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Mar 10, 2017 via email

bparees commented Sep 14, 2016 •

edited

openshift-bot commented Sep 30, 2016 •

edited

ikke-t commented Oct 7, 2016 •

edited