Deck: Optionally allow to make the rerun button create a new Job #12827

alvaroaleman · 2019-05-31T20:36:38Z

With this change it is possible to set a --rerun-creates-job flag in Deck, it defaults to false. If it is set to true and the Deck RBAC role is extended to allow CREATE for prowjobs, the rerun button in Deck will actually create a new ProwJob instead of showing instructions on how to create one (Which is only helpful to someone who has access to the Prow cluster).

I've tested manually that this works, but this is is the second time I've done something with TypeScript, so it is possible that that part looks weird to someone who knows how to do it properly 😬

/assign @Katharine

krzyzacy

This can be very useful for private prow instances!

Question I have in mind:

Do we want to add job-level blacklist/whitelist rerun?
I think we want to log who triggered this rerun, or else it's hard to audit, even for private instances
This can work with public Prow as well, if we want to introduce github auth :-)

krzyzacy · 2019-05-31T20:49:36Z

prow/cmd/deck/static/prow/prow.ts

+ var i;
+ if (rerunCreatesJob) {
+ i = icon.create("refresh", "Rerun this job");
+ i.href = `${url}`;


would like to see a pop up like created prowjob $prowjobID, or even a confirm page can help?

Yeah, had that thought as well. Will ask one of our frontend devs for help on Monday.

krzyzacy · 2019-05-31T20:58:25Z

cc @cjwagner @stevekuznetsov @fejta for thoughts ^^

alvaroaleman · 2019-05-31T21:05:26Z

Do we want to add job-level blacklist/whitelist rerun?

If we do that should be a generic thing and not specific to triggering them via Deck. /retest doesn't have something like that, so IMHO adding it just in Deck is confusing.

I think we want to log who triggered this rerun, or else it's hard to audit, even for private instances

Well, that is not possible because the auth for Deck is not integrated in Deck but instead done by some proxy that is put in front of it. If someone wants audit here, they should IMHO gather the relevant data from the proxy.

This can work with public Prow as well, if we want to introduce github auth :-)

Yes, but that is far beyond my frontend skills :P

Katharine · 2019-06-01T02:28:03Z

The ability to see the actual config used is pretty handy, as is the ability to see the prowjob ID (which is the UUID in that URL). I think that enabling this options completely removes the ability to do that. I am in favour of the functionality existing, but it would be nice to do something about the prior functionality, which went beyond a straight rerun (which is in practice almost none of my use of this button — I mostly just want a quick way to see what the job was).

It might also be worth somehow being clear about what "rerun" means — it uses the job configuration as of when that job was originally run (which may not be up to date), but re-resolves refs and so uses whatever the latest versions of those refs is. This may or may not be what people meant when they clicked it.

Katharine · 2019-06-01T02:29:52Z

prow/cmd/deck/static/prow/prow.ts

+ let i;
+ if (rerunCreatesJob) {
+ i = icon.create("refresh", "Rerun this job");
+ i.href = `${url}`;


I'd prefer to see this as an asynchronous call rather than a page reload.

But the page has to be reloaded anyways to see the new job or is there some other way? (Feel free to point my some doc, this is far from my area of expertise)

cjwagner · 2019-06-01T05:50:31Z

This can work with public Prow as well, if we want to introduce github auth :-)

This is something @mirandachrist will likely be working on very soon. Lets sync first thing next week and get a design doc for this so that we can implement this in an extensible way.

The ability to see the actual config used is pretty handy

+100 It would be great if we exposed the ProwJob yaml via a dedicated button instead of providing that info indirectly and non-obviously with the rerun button.

It might also be worth somehow being clear about what "rerun" means — it uses the job configuration as of when that job was originally run (which may not be up to date), but re-resolves refs and so uses whatever the latest versions of those refs is. This may or may not be what people meant when they clicked it.

Also a great point. It would be nice to provide both options with a UI switch. It would also be nice to have an option that runs the job newest configuration for the job instead of the spec that ran previously (we could add that in a follow up PR).

stevekuznetsov · 2019-06-03T15:39:04Z

I think a form fill guarded by GitHub Auth would be a much more forward-looking approach to this. +1 to Cole's comments

alvaroaleman · 2019-06-03T16:06:06Z

The ability to see the actual config used is pretty handy, as is the ability to see the prowjob ID (which is the UUID in that URL). I think that enabling this options completely removes the ability to do that. I am in favour of the functionality existing, but it would be nice to do something about the prior functionality, which went beyond a straight rerun (which is in practice almost none of my use of this button — I mostly just want a quick way to see what the job was).

Woho, I didn't realize. I was actually thinking about filing an issue so spyglass shows some infos like the job id and the name of the pod in its job view to easen debugging, but felt like I am complaining too much already 😬

It might also be worth somehow being clear about what "rerun" means — it uses the job configuration as of when that job was originally run (which may not be up to date), but re-resolves refs and so uses whatever the latest versions of those refs is. This may or may not be what people meant when they clicked it.

That does match with at least what I expected, although not sure if it also matches general expectations.

I think a form fill guarded by GitHub Auth would be a much more forward-looking approach to this. +1 to Cole's comments

Completely agree. But I would like to have this in the meantime as an intermediate solution. Currently the only way to restart periodics or postsubmits is by using what yaml from Deck and I would like to make this a bit better.

stevekuznetsov · 2019-06-03T17:04:49Z

That does match with at least what I expected, although not sure if it also matches general expectations.

I think we would want to make this an active choice from the user.

Completely agree. But I would like to have this in the meantime as an intermediate solution. Currently the only way to restart periodics or postsubmits is by using what yaml from Deck and I would like to make this a bit better.

While I appreciate that I do want to make sure we take the time to make this clean and not err too much on the side of quick, organic changes that complicate our landscape.

fejta · 2019-06-03T19:00:46Z

For presubmits we need to validate that the requesting user has the ability to call /test foo which might not always be the case: aka someone writes /test foo without leaving an /ok-to-test

stevekuznetsov · 2019-06-03T21:10:07Z

Yeah, and all of those questions are much more easily solved with a web page behind github auth. I think Alva means for this PR to be an admin-level flag on less locked-down Prow instances?

mirandachrist · 2019-06-04T18:57:00Z

I'm working on adding a button that links to the prowjob YAML, so the config will still be viewable after the rerun button is changed.

alvaroaleman · 2019-06-05T19:07:27Z

I think Alva means for this PR to be an admin-level flag on less locked-down Prow instances

Yes, exactly that, this should never get activated on a public Prow instance and that is also what the description of the flag says.

Discussed this with @cjwagner and @mirandachrist . Miranda will add another button for showing the yaml and improve what I added here by adding authorization and the possibility to change the refs.

alvaroaleman · 2019-06-14T17:55:28Z

So the "job config cant be displayed anymore" is not valid anymore, thanks to the dedicated button @mirandachrist added in 26eecf9.

@Katharine @cjwagner @stevekuznetsov Is there currently still something open you want to see as part of this PR? While the functionality is not really usable for public prow instances right now, its feature gated and shouldn't be too hard to extend with authn and/or authz.

Katharine · 2019-06-17T21:04:36Z

So I have one major concern about this - there is no CSRF (Cross-Site Request Forgery) protection present. The upshot is that a malicious user can get an authorised user to rerun a job by convincing them to follow a link. In theory, this is even a problem for an internal instance - in practice, given that you need a valid prowjob UUID to do anything, for a truly internal instance the risk is probably minimal. Externally, though, this would be a non-starter without additional CSRF protection, even once authorisation was added.

The general approach to CSRF protection is to issue users with a token on page load which is then validated when a protected action is taken. Usually this involves maintaining state, but if you can manage user/session identifiers somehow then you can pull it off using crypto instead.

Given the context of this PR, I'm not going to block on this - as long as only authorised users can discover valid prowjob UUIDs, I think the risk is pretty minimal. However, we must not blindly add authentication to this and open it up more broadly. I would like to see a comment somewhere pertinent in the code reminding people of this.

I also have a second concern: the request to /rerun should be required to be a POST request, for semantic reasons. This does not meaningfully improve security, but does avoid e.g. accidentally prefetching every /rerun on the page and thereby rerunning all the jobs. To do this from the page, you can either arrange for JavaScript to submit a form to this end, or make the rerun buttons be submit buttons for tiny forms with a hidden field each.

alvaroaleman · 2019-06-18T14:19:13Z

@Katharine This now does a POST request and has a warning regarding the CSRF

mirandachrist · 2019-06-18T21:30:21Z

prow/cmd/deck/static/prow/prow.ts

+ let i;
+ if (rerunCreatesJob) {
+ i = icon.create("refresh", "Rerun this job");
+ i.onclick = () => {


Just a heads up - some people have said they'd still like to see the "kubectl create ..." popup, even if directly triggering the rerun is enabled. In my PR, I'm adding a "run" button within that popup that will actually rerun the job, so it's still displayed:

@mirandachrist If you just hadn't explained that, I wouldn't have derived from the view on the screenshot that the run button runs the job tbh (But that discussion is probably better put on your PR)

Katharine · 2019-06-19T15:48:52Z

This looks almost good to me, but I would like to see handleRerun actually check the request method and only do the rerun if it is actually POST.

I'd ideally also like to see it take the prowjob name from a POSTed form rather than from the query string, but I'm not going to be picky about that one this time around.

cjwagner · 2019-06-19T17:56:57Z

Given the context of this PR, I'm not going to block on this - as long as only authorised users can discover valid prowjob UUIDs, I think the risk is pretty minimal.

Anyone can discover valid ProwJob UUIDs if they can access the front end. The /data.js, /prowjobs.js, and ProwJob YAML buttons all expose valid UUIDs.
IIUC this PR is only considered safe because the feature is meant for private Prow instances that have an oauth proxy in front of deck?

Katharine · 2019-06-19T18:05:29Z

IIUC this PR is only considered safe because the feature is meant for private Prow instances that have an oauth proxy in front of deck?

Correct. We cannot use this as part of the OAuth-ed rerun setup without further work (and there is now a comment in the relevant code reminding us of this).

alvaroaleman · 2019-06-19T18:22:02Z

This looks almost good to me, but I would like to see handleRerun actually check the request method and only do the rerun if it is actually POST.

@Katharine done

alvaroaleman · 2019-06-19T18:22:53Z

Or did you mean for it to always check for POST, not only in the case where the rerun is actually executed?

Katharine · 2019-06-19T20:04:06Z

Or did you mean for it to always check for POST, not only in the case where the rerun is actually executed?

Only in the case where the rerun is actually executed.

prow/cmd/deck/main.go

Co-Authored-By: Katharine Berry <ktbry@google.com>

k8s-ci-robot · 2019-06-19T20:31:30Z

LGTM label has been added.

Git tree hash: 93f1314bac35c12f14d1dfac772c58d91452b111

k8s-ci-robot · 2019-06-19T20:31:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, Katharine

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~prow/cluster/OWNERS~~ [Katharine]
~~prow/cmd/deck/OWNERS~~ [Katharine]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot assigned Katharine May 31, 2019

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 31, 2019

k8s-ci-robot requested review from amwat and Katharine May 31, 2019 20:36

alvaroaleman force-pushed the ui-restart branch 2 times, most recently from 002cb98 to 4ad9c05 Compare May 31, 2019 20:50

krzyzacy reviewed May 31, 2019

View reviewed changes

alvaroaleman force-pushed the ui-restart branch from 4ad9c05 to f323933 Compare May 31, 2019 21:06

Katharine reviewed Jun 1, 2019

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2019

alvaroaleman force-pushed the ui-restart branch from f323933 to 1693ae6 Compare June 14, 2019 17:50

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2019

alvaroaleman force-pushed the ui-restart branch 2 times, most recently from c21f094 to 4218197 Compare June 18, 2019 13:15

mirandachrist reviewed Jun 18, 2019

View reviewed changes

Katharine mentioned this pull request Jun 19, 2019

Prow Sub: Allow execute prowjob defined from payload #13076

Closed

Katharine mentioned this pull request Jun 19, 2019

Set up GitHub oauth for rerun button #13008

Merged

alvaroaleman force-pushed the ui-restart branch from 4218197 to 1b5b013 Compare June 19, 2019 18:19

Deck: Optionally allow to make the rerun button create a new Job

972b0fa

alvaroaleman force-pushed the ui-restart branch from 1b5b013 to 972b0fa Compare June 19, 2019 18:33

Katharine reviewed Jun 19, 2019

View reviewed changes

prow/cmd/deck/main.go Outdated Show resolved Hide resolved

Update prow/cmd/deck/main.go

9580d19

Co-Authored-By: Katharine Berry <ktbry@google.com>

Katharine approved these changes Jun 19, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2019

k8s-ci-robot merged commit 18c26b9 into kubernetes:master Jun 19, 2019

mirandachrist mentioned this pull request Jul 3, 2019

Allow direct triggering of reruns #13321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deck: Optionally allow to make the rerun button create a new Job #12827

Deck: Optionally allow to make the rerun button create a new Job #12827

alvaroaleman commented May 31, 2019

krzyzacy left a comment

krzyzacy May 31, 2019

alvaroaleman May 31, 2019

krzyzacy commented May 31, 2019

alvaroaleman commented May 31, 2019

Katharine commented Jun 1, 2019 •

edited

Loading

Katharine Jun 1, 2019

alvaroaleman Jun 3, 2019

cjwagner commented Jun 1, 2019

stevekuznetsov commented Jun 3, 2019

alvaroaleman commented Jun 3, 2019

stevekuznetsov commented Jun 3, 2019

fejta commented Jun 3, 2019

stevekuznetsov commented Jun 3, 2019

mirandachrist commented Jun 4, 2019

alvaroaleman commented Jun 5, 2019

alvaroaleman commented Jun 14, 2019

Katharine commented Jun 17, 2019 •

edited

Loading

alvaroaleman commented Jun 18, 2019

mirandachrist Jun 18, 2019

alvaroaleman Jun 19, 2019

Katharine commented Jun 19, 2019

cjwagner commented Jun 19, 2019

Katharine commented Jun 19, 2019

alvaroaleman commented Jun 19, 2019

alvaroaleman commented Jun 19, 2019

Katharine commented Jun 19, 2019

k8s-ci-robot commented Jun 19, 2019

k8s-ci-robot commented Jun 19, 2019

Deck: Optionally allow to make the rerun button create a new Job #12827

Deck: Optionally allow to make the rerun button create a new Job #12827

Conversation

alvaroaleman commented May 31, 2019

krzyzacy left a comment

Choose a reason for hiding this comment

krzyzacy May 31, 2019

Choose a reason for hiding this comment

alvaroaleman May 31, 2019

Choose a reason for hiding this comment

krzyzacy commented May 31, 2019

alvaroaleman commented May 31, 2019

Katharine commented Jun 1, 2019 • edited Loading

Katharine Jun 1, 2019

Choose a reason for hiding this comment

alvaroaleman Jun 3, 2019

Choose a reason for hiding this comment

cjwagner commented Jun 1, 2019

stevekuznetsov commented Jun 3, 2019

alvaroaleman commented Jun 3, 2019

stevekuznetsov commented Jun 3, 2019

fejta commented Jun 3, 2019

stevekuznetsov commented Jun 3, 2019

mirandachrist commented Jun 4, 2019

alvaroaleman commented Jun 5, 2019

alvaroaleman commented Jun 14, 2019

Katharine commented Jun 17, 2019 • edited Loading

alvaroaleman commented Jun 18, 2019

mirandachrist Jun 18, 2019

Choose a reason for hiding this comment

alvaroaleman Jun 19, 2019

Choose a reason for hiding this comment

Katharine commented Jun 19, 2019

cjwagner commented Jun 19, 2019

Katharine commented Jun 19, 2019

alvaroaleman commented Jun 19, 2019

alvaroaleman commented Jun 19, 2019

Katharine commented Jun 19, 2019

k8s-ci-robot commented Jun 19, 2019

k8s-ci-robot commented Jun 19, 2019

Katharine commented Jun 1, 2019 •

edited

Loading

Katharine commented Jun 17, 2019 •

edited

Loading