Add support for bearer token auth in Prow Jenkins controller #4210

stevekuznetsov · 2017-08-28T14:39:56Z

Not all Jenkins masters allow for basic auth. This patch adds support
for bearer token auth in the Prow Jenkins client.

Signed-off-by: Steve Kuznetsov [email protected]

/area prow
/cc @spxtr @Kargakis @csrwng

stevekuznetsov · 2017-08-28T14:41:16Z

(1) [k8sCI] Add bearer token auth to the Prow Jenkins controller

stevekuznetsov · 2017-08-28T16:57:47Z

/assign @Kargakis

spxtr

Overall idea SGTM, as long as it doesn't break our current setup :)

I'll let @Kargakis review in depth.

0xmichalis · 2017-08-29T09:15:50Z

prow/cmd/deck/main.go

+				}
+			}
+		} else {
+			logrus.Fatal("An auth token for basic or bearer token auth must be supplied.")


deck is used only for logs and on public jenkins instances we don't even need basic auth so we shouldn't break this.

The code prior to this change is

if *jenkinsURL != "" { jenkinsSecretRaw, err := ioutil.ReadFile(*jenkinsTokenFile) if err != nil { logrus.WithError(err).Fatalf("Could not read token file.") } jenkinsToken := string(bytes.TrimSpace(jenkinsSecretRaw)) jc = jenkins.NewClient(*jenkinsURL, *jenkinsUserName, jenkinsToken) }

How does that allow for no token?

Oh well.. I think I had a branch where I was making optional the parsing of the file because it's not really needed (tested). Nevermind, it's ok for now.

It allows for no token if *jenkinsURL is empty.

It won't work without a url.

It won't be able to look up any agent: jenkins jobs, but it will be able to do everything else.

Yeah, I was referring to getting the logs from Jenkins.

0xmichalis · 2017-08-29T09:30:45Z

Jenkins controller changes lgtm

Thoughts on deck: As we move forward, adding agent-specific flags in deck will become untenable. For this PR this is fine, but is it reasonable in the future to feed deck with a secret that contains all the HTTP headers required for auth or is this a crazy idea? Related issue: #3407

stevekuznetsov · 2017-08-29T13:33:16Z

@Kargakis do we need to dry out the flag parsing --> jenkins client creation code? Where would we put it?

0xmichalis · 2017-08-29T14:12:47Z

@Kargakis do we need to dry out the flag parsing --> jenkins client creation code? Where would we put it?

No, this is ok.

/lgtm

0xmichalis · 2017-08-29T14:37:52Z

prow/cmd/deck/main.go

-		jenkinsSecretRaw, err := ioutil.ReadFile(*jenkinsTokenFile)
-		if err != nil {
-			logrus.WithError(err).Fatalf("Could not read token file.")
+		if jenkinsTokenFile != nil {


Will this ever be non-nil? I think we should keep the default values empty and populate the options inside the deployments.

0xmichalis · 2017-08-29T14:38:17Z

/lgtm cancel

stevekuznetsov · 2017-08-29T14:57:12Z

prow/cluster/jenkins_deployment.yaml

@@ -25,9 +25,10 @@ spec:
    spec:
      containers:
      - name: jenkins-operator
-        image: gcr.io/k8s-prow/jenkins-operator:0.39
+        image: gcr.io/k8s-prow/jenkins-operator:.1


@Kargakis right now ...

$ prow/bump.sh jenkins-operator program: jenkins-operator old version: new version: .1 $ prow/bump.sh jenkins program: jenkins old version: 0.39 new version: 0.40

One of these updates the deployment, one gets the Makefile?

0xmichalis · 2017-08-30T08:56:08Z

Add an announcement similar to what you did in #4213

stevekuznetsov · 2017-08-30T21:13:45Z

I'll wait for it to merge so we can do that cleanly

stevekuznetsov · 2017-09-01T14:04:11Z

@Kargakis rebased. added README

0xmichalis · 2017-09-01T14:10:05Z

/lgtm

0xmichalis · 2017-09-01T14:10:39Z

I will let @spxtr merge.

stevekuznetsov · 2017-09-01T21:20:32Z

@spxtr @Kargakis rebased

spxtr · 2017-09-01T21:22:00Z

#4213 is in.

stevekuznetsov · 2017-09-01T21:25:40Z

Yeah, I've got the README bits in here now as well

spxtr · 2017-09-01T22:43:56Z

prow/README.md

@@ -31,6 +31,12 @@ state and no claims of backwards compatibility are made for any external API.
   Cluster administrators upgrading to the newest version of Prow should move
   plugin configuration from the main `ConfigMap`. For more context, please see
   [this pull request.](https://github.com/kubernetes/test-infra/pull/4213)
+ - *September 1, 2017* Deck and Jenkins-Operator controllers no longer provide


These should go the other way, with newer entries at the top.

Can we also include the versions or commits that break?

stevekuznetsov · 2017-09-05T14:30:07Z

@spxtr @Kargakis updated the README -- NACK in general to adding versions to the comments as that requires a bump in the PR adding it ... and we were just converging in #4292 on not asking contributors to bump versions in their PRs.

stevekuznetsov · 2017-09-05T14:30:38Z

Adding a SHA in there might be risky as it has a high chance of drifting.

0xmichalis · 2017-09-05T14:32:14Z

and we were just converging in #4292 on not asking contributors to bump versions in their PRs.

Hopefully not every PR breaks something. For breakages, bumping makes sense to me.

0xmichalis · 2017-09-05T14:34:15Z

Btw, you need to rebase on top of HEAD

stevekuznetsov · 2017-09-05T14:43:38Z

Btw, you need to rebase on top of HEAD

I rebased before the last push

stevekuznetsov · 2017-09-05T14:44:11Z

Hopefully not every PR breaks something. For breakages, bumping makes sense to me.

Fair point -- that makes good sense to me too.

0xmichalis · 2017-09-05T15:32:03Z

prow/cluster/jenkins_deployment.yaml

        args:
        - --dry-run=false
+        - --jenkins-token=/etc/jenkins/jenkins


--jenkins-token-file

0xmichalis · 2017-09-05T15:32:09Z

prow/cluster/deck_deployment.yaml

        ports:
          - name: http
            containerPort: 8080
        args:
        - --jenkins-url=$(JENKINS_URL)
+        - --jenkins-token=/etc/jenkins/jenkins


--jenkins-token-file

Not all Jenkins masters allow for basic auth. This patch adds support for bearer token auth in the Prow Jenkins client. Signed-off-by: Steve Kuznetsov <[email protected]>

0xmichalis · 2017-09-05T15:40:09Z

/lgtm

stevekuznetsov requested a review from spxtr as a code owner August 28, 2017 14:39

k8s-ci-robot requested review from csrwng and 0xmichalis August 28, 2017 14:39

k8s-ci-robot added area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 28, 2017

k8s-ci-robot assigned 0xmichalis Aug 28, 2017

spxtr approved these changes Aug 28, 2017

View reviewed changes

0xmichalis reviewed Aug 29, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2017

0xmichalis reviewed Aug 29, 2017

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2017

stevekuznetsov force-pushed the skuznets/bearer-token branch 3 times, most recently from d9860bb to 1d2c796 Compare August 29, 2017 14:56

stevekuznetsov commented Aug 29, 2017

View reviewed changes

stevekuznetsov force-pushed the skuznets/bearer-token branch 2 times, most recently from d1e6fe2 to 1cd9fca Compare August 29, 2017 15:01

stevekuznetsov force-pushed the skuznets/bearer-token branch 3 times, most recently from 9ffdbcb to f36ef2a Compare September 1, 2017 14:03

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017

0xmichalis mentioned this pull request Sep 1, 2017

Make sinker delete only pods created by prow #4314

Merged

stevekuznetsov force-pushed the skuznets/bearer-token branch from f36ef2a to 8dd6824 Compare September 1, 2017 21:19

spxtr reviewed Sep 1, 2017

View reviewed changes

stevekuznetsov force-pushed the skuznets/bearer-token branch 3 times, most recently from 50312a1 to 9d6b45a Compare September 5, 2017 14:28

0xmichalis reviewed Sep 5, 2017

View reviewed changes

Add support for bearer token auth in Prow Jenkins controller

206a5e9

Not all Jenkins masters allow for basic auth. This patch adds support for bearer token auth in the Prow Jenkins client. Signed-off-by: Steve Kuznetsov <[email protected]>

stevekuznetsov force-pushed the skuznets/bearer-token branch from 9d6b45a to 206a5e9 Compare September 5, 2017 15:36

0xmichalis merged commit 67103c0 into kubernetes:master Sep 5, 2017

0xmichalis mentioned this pull request Sep 5, 2017

Fix panic in jenkins operator #4385

Merged

Add support for bearer token auth in Prow Jenkins controller #4210

Add support for bearer token auth in Prow Jenkins controller #4210

Conversation

stevekuznetsov commented Aug 28, 2017

stevekuznetsov commented Aug 28, 2017

stevekuznetsov commented Aug 28, 2017

spxtr left a comment

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

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

0xmichalis commented Aug 29, 2017

stevekuznetsov commented Aug 29, 2017

0xmichalis commented Aug 29, 2017

Choose a reason for hiding this comment

0xmichalis commented Aug 29, 2017

Choose a reason for hiding this comment

0xmichalis commented Aug 30, 2017

stevekuznetsov commented Aug 30, 2017

stevekuznetsov commented Sep 1, 2017

0xmichalis commented Sep 1, 2017

0xmichalis commented Sep 1, 2017

stevekuznetsov commented Sep 1, 2017 • edited Loading

spxtr commented Sep 1, 2017

stevekuznetsov commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov commented Sep 5, 2017

stevekuznetsov commented Sep 5, 2017

0xmichalis commented Sep 5, 2017

0xmichalis commented Sep 5, 2017

stevekuznetsov commented Sep 5, 2017

stevekuznetsov commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Sep 5, 2017

stevekuznetsov commented Sep 1, 2017 •

edited

Loading