-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
apiserver: Fail if dry-run query param is specified #63557
Conversation
@@ -318,3 +319,7 @@ func parseTimeout(str string) time.Duration { | |||
} | |||
return 30 * time.Second | |||
} | |||
|
|||
func isDryRun(url *url.URL) bool { | |||
return len(url.Query()["dry-run"]) != 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing to debate is if this is the way we want to represent dry run. I'd open an issue & send an email to the api machinery list, since this is a bit late in the cycle. It's a very simple change so there shouldn't be a problem but let's be noisy about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably OK for this sort of PR (maybe @deads2k won't think so), but you should be aware that this skips all of our ordinary URL parsing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably OK for this sort of PR (maybe @deads2k won't think so), but you should be aware that this skips all of our ordinary URL parsing code.
I do think it's an example of the feature branch allowing flexibility we wouldn't allow into master. I think that flexibility is very useful when working something like this, but it does undermine the idea that the feature branch is as rigorous as master.
On a related note. If dry-run works well in this branch and the semantics are well described, it seems like a good target to bring into master separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is targeting master :)
The goal is to get dry run as a concept established as early as possible so that it can be relied on to fail safe, since the consequences of submitting a dry run request the server doesn’t know is a dry run is really bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the parameter is actually used for anything other than a top-level short-circuiting fail like this, I’d expect the create/update/patch handlers to be updated to use the *Options approach and decode parameters just like the other handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this change is for master and wouldn't make sense for the feature branch.
I agree w/ @liggitt that it needs to go through the right stack before we do anything productive with it. I think this is fine as an easy & safe way to reserve it, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at all the other *Options
structs, we'd add this as dryRun
, not dry-run
/retest |
David & I talked earlier-- this is good to go if you can just add a test ;) |
Added tests, PTAL |
} | ||
} | ||
|
||
func TestDryRunPatch(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also test update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how I missed it :-) Thanks
I've added the missing test, and |
} | ||
} | ||
|
||
func TestDryRunDelete(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deletecollection as well? (with test)
this covers subresources with standard rest crud handlers. what do we plan to do with subresources that implement "connect" (things like pods/exec, pods/attach, */proxy, pods/portforward, etc)? |
That's a great point, I think those should *always* reject dryRun
requests--there's no way to pass along the dryRun invariant.
…On Thu, May 10, 2018 at 10:21 AM Jordan Liggitt ***@***.***> wrote:
this covers subresources with standard rest crud handlers. what do we plan
to do with subresources that implement "connect" (things like pods/exec,
pods/attach, */proxy, pods/portforward, etc)?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#63557 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAnglp6NNrai-OzYV13QkOT9CEbIOLFsks5txHcfgaJpZM4T3a6Q>
.
|
wouldn't you expect them to exercise admission/validation the same way a normal request would, and stop short of execution? especially for exec/attach, I could see that being really handy. |
That does sound super handy, actually. @apelisse is going to write up a
design doc and this is a good thing to include.
…On Thu, May 10, 2018 at 11:07 AM Jordan Liggitt ***@***.***> wrote:
I think those should *always* reject dryRun requests--there's no way to
pass along the dryRun invariant.
wouldn't you expect them to exercise admission/validation the same way a
normal request would, and stop short of execution? especially for
exec/attach, I could see that being really handy.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#63557 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAnglvZN7OSmsHfptCBzwHs_Tyrzc4vpks5txIHJgaJpZM4T3a6Q>
.
|
d52d7b5
to
143a068
Compare
I've had to change more test code than I wish I had to, but I think this is correct. |
return nil, nil | ||
} | ||
|
||
// TestUnimplementedRESTStorage ensures that if a rest.Storage does not implement a given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should update that comment...
@@ -318,3 +319,7 @@ func parseTimeout(str string) time.Duration { | |||
} | |||
return 30 * time.Second | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be a good idea to add a TODO here explaining that this will be moved to our versioned structs like other parameters before being implemented for real.
Thanks for putting the test like we discussed IRL :) I have one suggestion but I think it's optional. I think, given where you put the check, it's not possible to screw it up by doing start-up plumbing wrong, so there's no need to do an integration check. I'm not sure how to make this work for aggregated apiservers--we can't force people to upgrade. I will discuss in the issue. /lgtm |
/retest |
/release-note-none |
Are you sure this shouldn't say anything for a release note?
…On Mon, May 14, 2018 at 3:33 PM Antoine Pelisse ***@***.***> wrote:
/release-note-none
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#63557 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAnglpk6pRihZrTdGNC0I2U9KH9miTVhks5tygYhgaJpZM4T3a6Q>
.
|
Yeah, definitely needs a release note |
Tentatively added a release note. |
Edited the release note a bit :) /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, lavalamp 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:
Approvers can indicate their approval by writing |
Automatic merge from submit-queue (batch tested with PRs 63603, 63557, 62015). If you want to cherry-pick this change to another branch, please follow the instructions here. |
Adds a dry-run query parameter now that does nothing but reject the request. The sooner we have this check in master, the safer it will be for clients to send dry-run requests that are not going to be applied nonetheless.