Add support for Docker Registry authentication to Build

[mfojtik]

(this is heavy WIP atm, any feedback appreciated)

[mfojtik]

@soltysh @bparees FYI, still need heavy testing, but I would appreciate early feedback about naming/etc ;-)

[mfojtik]

@bparees @soltysh I'm taking suggestion for this, I don't like this name

[soltysh]

I'd go with just SecretRef or RegistrySecretRef, I agree the one above i too long.

[bparees]

Even just Secret is fine, it doesn't have to contain "Ref" in the name, we don't do that in other places that we use ObjectReference.

I'd go w/ RegistrySecret

Is this field going to exist in all 3 strategies? if so i'm wondering if it should be factored out to a higher level.

[bparees]

I see it does, so that question stands. i kind of like keeping it at the same level as the image it's associated with, but i hate the fact that we have the same field 3 times.

[mfojtik]

@bparees @soltysh created issue for that, I want to do that as follow up with other properties.

and I like RegistrySecret

[mfojtik]

@smarterclayton PTAL

(this is pending tests which depend on having private registry with credentials support, which is currently WIP)

[ncdc]

update first word

[mfojtik]

my replace-fu failed :-( thanks

[mfojtik]

[test]

[openshift-bot]

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

[mfojtik]

@liggitt PTAL

quoting @smarterclayton : build configs should probably be referencing serivce accounts

[liggitt]

Make sure the kind is what you expect. Not sure what to check for the namespace

[mfojtik]

If I do, then users will have to add Kind to ObjectReference... Is that
what you mean?
On Mar 23, 2015 7:34 PM, "Jordan Liggitt" [email protected] wrote:

In pkg/build/api/validation/validation.go
#1411 (comment):

@@ -176,6 +180,17 @@ func validateStrategy(strategy *buildapi.BuildStrategy) errs.ValidationErrorList
return allErrs
}

+func validateDockerSecretRef(ref *kapi.ObjectReference) errs.ValidationErrorList {

• allErrs := errs.ValidationErrorList{}
• if ref == nil {

•   return allErrs
  

• }
• if len(ref.Name) == 0 {

•   allErrs = append(allErrs, errs.NewFieldInvalid("RegistrySecret", ref.Name, "the Secret name cannot be blank"))
  

• }
• return allErrs

Make sure the kind is what you expect. Not sure what to check for the
namespace

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/1411/files#r26965430.

[smarterclayton]

The build pods have to run as a service account (which will be set by some process, and potentially overriden by a user). The secret used to download a Git repo or url should be one of the ones named by the account. I think it may be ok to let users set a single named secret for the build and again one for the output.

On Mar 23, 2015, at 12:57 PM, Michal Fojtik [email protected] wrote:

@liggitt PTAL

quoting @smarterclayton : build configs should probably be referencing serivce accounts

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

[mfojtik]

@smarterclayton my last commit copies the namespace from the Build itself to Build pod. Does it cover the service accounts or can you or @liggitt elaborate on what does it mean 'service account' ? Is the current mechanism to inject the secrets to build wrong?

[mfojtik]

@smarterclayton @bparees I made some changes we were discussing yesterday, so I moved the RegistrySecret to Output and rename it to PushSecretName. This is no longer ObjectReference as the Secret can't be reference cross-namespace.

I updated all builders to recognize two env vars, PUSH_DOCKERCFG_PATH and PULL_DOCKERCFG_PATH (one for pushing and second for pulling). At this moment the one for pushing is also used for pulling images (with big TODO to change this once we get Service Accounts)

PTAL. This is now mergeable, I tested this using my Hub credentials and the output image is pushed to Hub properly.

One problem I had was with the Secret name itself. For now, Secret.Name and the Data[Name] = "base64(dockercfg)" 'Name' needs to match. I use 'Secret.Name' as the filename for the dockercfg.

[soltysh]

LGTM.

[bparees]

remove "Name"?

[mfojtik]

@bparees thought that it would be better to be specific on what exactly we do require here ('name' of the secret)

[mfojtik]

convinced to just PushSecret via IRC ;-)

[mfojtik]

@bparees comments addressed, squashed :-)

[smarterclayton]

Needs to be consistent with Paul's latest, which is "SecretName". You should not merge this until that discussion settles upstream.

[mfojtik]

@bparees convinced me that we don't need Name as we also not using Ref
suffix.
On Mar 24, 2015 6:09 PM, "Clayton Coleman" [email protected] wrote:

In pkg/build/api/types.go
#1411 (comment):

@@ -231,6 +231,11 @@ type BuildOutput struct {
// a Docker image repository to push to. Failure to find the To will result in a build error.
To *kapi.ObjectReference json:"to,omitempty"

• // pushSecret is the name of a Secret that would be used for setting
• // up the authentication for executing the Docker push to authentication
• // enabled Docker Registry (or Docker Hub).
• PushSecret string json:"pushSecret,omitempty"

Needs to be consistent with Paul's latest, which is "SecretName". You
should not merge this until that discussion settles upstream.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/1411/files#r27050027.

[bparees]

lgtm for handling push.

[mfojtik]

@bparees @smarterclayton comments addressed, do you want me still wait for @pmorie name vs. not name ?

[bparees]

@mfojtik yeah might as well have the 5 minute discussion rather than having to do another PR.

[mfojtik]

@bparees @pmorie @smarterclayton updated, all comments addressed, so I think this is ready for push and follow up work (documentation && testing)

[bparees]

lgtm, +1 on needs doc + testing :)

[mfojtik]

[merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1289/) (Image: devenv-fedora_1128)

[openshift-bot]

Evaluated for origin up to 13acec5