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

Jump to bottom

allow http proxy env variables to be set in privileged sti container #3401

Merged
merged 1 commit into from Jun 26, 2015
Merged

allow http proxy env variables to be set in privileged sti container #3401

merged 1 commit into from Jun 26, 2015

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Jun 23, 2015

fixes #3395

lots of changes here now:

  1. updated the sample template to v1 api
  2. added Env field to Docker builds
  3. whitelist remains what it was (build_loglevel is the only allowed env variable for now in the privileged containers)
  4. added httpProxy and httpsProxy fields to GitBuildSource. If present, those values are set as env variables just before we invoke git clone and unset immediately after, so they only impact the clone step - applicable to S2I and Docker type builds

@bparees
Copy link
Contributor Author

bparees commented Jun 23, 2015

@mfojtik ptal
@smarterclayton do you foresee a problem with allowing users to inject HTTP_PROXY env variables into our privileged STI and Docker builder containers? they need to do this to control the proxy used by our git clone operation.

@bparees
Copy link
Contributor Author

bparees commented Jun 23, 2015

(Note that i also added an env stanza to docker type builds..since it's additive i believe it's ok to just add it to the v1 api)

@mfojtik
Copy link
Member

mfojtik commented Jun 23, 2015

LGTM (the HTTP_PROXY var will be supported also by docker build soon, so if you set it, docker build can pick it up and the build might use it which is not what you always want...)

@nak3
Copy link
Member

nak3 commented Jun 23, 2015

I'm sorry if I'm wrong, but just in case.
http proxy for git clone needs to set with git config --global http.proxy $http_proxy in the container. It doesn’t work with only env value.

@bparees
Copy link
Contributor Author

bparees commented Jun 23, 2015

@nak3 at least one reference i've found implies it does support just setting the env variables, in lowercase form:
http://nknu.net/git-through-proxy/

I don't have an environment setup to easily validate that though, if you do, i'd appreciate knowing!

@nak3
Copy link
Member

nak3 commented Jun 24, 2015

@bparees After I tested an environment setup again, it worked correctly. Sorry! Please merge this PR.

@smarterclayton
Copy link
Contributor

Do we need to subdivide the proxy for different parts of a build? Docker build vs git clone vs hooks called by the s2i scripts?

On Jun 23, 2015, at 1:46 PM, Ben Parees notifications@github.com wrote:

@mfojtik ptal
@smarterclayton do you foresee a problem with allowing users to inject HTTP_PROXY env variables into our privileged STI and Docker builder containers? they need to do this to control the proxy used by our git clone operation.

—
Reply to this email directly or view it on GitHub.

@mfojtik
Copy link
Member

mfojtik commented Jun 24, 2015

@smarterclayton when the docker PR to add support for HTTP_PROXY gets merged, then we will have to do it... I'm not sure what would be the best way to do this however...

soltysh
soltysh reviewed Jun 24, 2015
View reviewed changes
pkg/build/controller/strategy/util.go
@@ -17,7 +17,7 @@ const (
sourceSecretMountPath = "/var/run/secrets/openshift.io/source"
)

var whitelistEnvVarNames = []string{"BUILD_LOGLEVEL"}
var whitelistEnvVarNames = []string{"BUILD_LOGLEVEL", "HTTP_PROXY", "HTTPS_PROXY", "http_proxy", "https_proxy"}
Copy link
Member

Choose a reason for hiding this comment

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

Can we type this once and assume we'll always allow both upper case and lower case vars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds insecure to me :)

Copy link
Member

Choose a reason for hiding this comment

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

i like to make this explicit :-)

Copy link
Member

Choose a reason for hiding this comment

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

Fine by me, was just curious.

@bparees
Copy link
Contributor Author

bparees commented Jun 24, 2015

@smarterclayton
So if we want to separate this, we need to add a GIT_PROXY field to the strategies and then update our git clone logic to utilize that proxy. I can do that, but it's a more invasive change.

the question would be, do we still want to leave the _PROXY env vars on the whitelist?

let me take a crack at it.

@bparees bparees force-pushed the whitelist branch 3 times, most recently from 49d25a1 to 9074d7f Compare June 25, 2015 01:12
@bparees
Copy link
Contributor Author

bparees commented Jun 25, 2015

@smarterclayton @mfojtik massive rework, ptal again.
[test]

soltysh
soltysh reviewed Jun 25, 2015
View reviewed changes
pkg/build/builder/sti.go
os.Setenv("https_proxy", s.build.Parameters.Source.Git.HTTPSProxy)
setHttps = true
}
if len(s.build.Parameters.Source.Git.HTTPSProxy) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

You're checking HTTPS but setting HTTP proxy here/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doh. thanks.

@bparees
Copy link
Contributor Author

bparees commented Jun 25, 2015

@soltysh cut+paste error corrected in sti and docker. thanks for the catch.

@bparees
Copy link
Contributor Author

bparees commented Jun 25, 2015

@soltysh any other comments?
[test]

mfojtik
mfojtik reviewed Jun 26, 2015
View reviewed changes
pkg/api/deep_copy_generated.go
@@ -1004,12 +1004,26 @@ func deepCopy_api_DockerBuildStrategy(in buildapi.DockerBuildStrategy, out *buil
} else {
out.PullSecret = nil
}
if in.Env != nil {
Copy link
Member

Choose a reason for hiding this comment

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

can you reverse this if? So you just set Env to nil and return and nuke the else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's generated code, so no.

Copy link
Member

Choose a reason for hiding this comment

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

oh i should guess it from the name :-)

@mfojtik
Copy link
Member

mfojtik commented Jun 26, 2015

@bparees couple coding nits, otherwise LGTM

soltysh
soltysh reviewed Jun 26, 2015
View reviewed changes
pkg/build/api/v1/types.go
@@ -166,6 +166,12 @@ type GitBuildSource struct {

// Ref is the branch/tag/ref to build.
Ref string `json:"ref,omitempty" description:"identifies the branch/tag/ref to build"`

// HTTPProxy is a proxy used to reach the git repository over http
HTTPProxy string `json:"httpProxy,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Description please, all the new vars should have one.

@bparees bparees force-pushed the whitelist branch 2 times, most recently from f21260c to 37e82d6 Compare June 26, 2015 13:48
@mfojtik
Copy link
Member

mfojtik commented Jun 26, 2015

LGTM [merge]

@soltysh
Copy link
Member

soltysh commented Jun 26, 2015

Damn it @mfojtik you were faster with your LGTM 😜

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/3424/) (Image: devenv-fedora_1880)

@openshift-bot
Copy link
Contributor

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

@bparees
Copy link
Contributor Author

bparees commented Jun 26, 2015

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 64d3312

openshift-bot pushed a commit that referenced this pull request Jun 26, 2015
Merge pull request #3401 from bparees/whitelist
65a1921 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 65a1921 into openshift:master Jun 26, 2015
@bparees bparees deleted the whitelist branch June 30, 2015 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

osc start-build doesn't work under http(s) proxy
7 participants
@bparees @mfojtik @nak3 @smarterclayton @soltysh @openshift-bot @liggitt