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

[FEATURE BRANCH] Force allow create on patch if content type is apply #65723

Merged
merged 1 commit into from Jul 9, 2018

Conversation

@jennybuckley
Copy link
Contributor

jennybuckley commented Jul 2, 2018

What this PR does / why we need it:
Force allow create on patch if content type is apply and change the apply test to use a resource which doesn't have create-on-update enabled.

Special notes for your reviewer:
Based on top of #65720 because it relies on #65154 from master.
Only 083a596 is part of this PR.

Release note:

NONE

/sig api-machinery
/kind feature
/cc @seans3 @liggitt @apelisse

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 2, 2018

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jul 2, 2018

is this a merge of master or is there other feature-branch-specific content mixed in here? can we keep the merge-master PRs separate for ease of reviewing?

@apelisse

This comment has been minimized.

Copy link
Member

apelisse commented Jul 2, 2018

I'm assuming this goes on top of #65720. but it hard to tell. I would wait for that PR to get merged before looking at this.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jul 2, 2018

I'm assuming this goes on top of #65720. but it hard to tell. I would wait for that PR to get merged before looking at this.

ah, ok, thanks. that's what I get for not reading the whole description :)

@apelisse

This comment has been minimized.

Copy link
Member

apelisse commented Jul 2, 2018

that's what I get for not reading the whole description :)

Ah yes, I see, that's confirmed by the description 🤣

@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

jennybuckley commented Jul 2, 2018

Looks good except for the issue in the PR this was based on. This one will be fixed once #65720 merges so I can rebase it.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 3, 2018

@jennybuckley: Those labels are not set on the issue: sig/cluster-lifecycle, sig/scheduling, area/kubeadm

In response to this:

/remove-sig cluster-lifecycle
/remove-sig scheduling
/remove-area kubeadm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Jul 3, 2018

@apelisse

This comment has been minimized.

Copy link
Member

apelisse commented Jul 4, 2018

/retest

// TODO: Check with the authorizer if the user has permission to create
objToUpdate, patchErr = p.mechanism.createNewObject()
} else {
objToUpdate, patchErr = p.mechanism.applyPatchToCurrentObject(currentObject)
if objToUpdateHasUID, err := hasUID(objToUpdate); err != nil {

This comment has been minimized.

@apelisse

apelisse Jul 4, 2018

Member

Why is it OK to apply/update with UID, but not to apply/create?

This comment has been minimized.

@liggitt

liggitt Jul 4, 2018

Member

uid is a precondition that requires we match the existing object... it's the only way you can guarantee your applied patch is merged into the same object. if you don't want that precondition, don't include uid

This comment has been minimized.

@apelisse

apelisse Jul 4, 2018

Member

OK, So it's safe to assume that the code will check that. Great, thanks :-)

if objToUpdateHasUID, err := hasUID(objToUpdate); err != nil {
return nil, err
} else if objToUpdateHasUID {
return nil, errors.NewConflict(p.resource.GroupResource(), p.name, fmt.Errorf("object to create cannot specify UID"))

This comment has been minimized.

@liggitt

liggitt Jul 4, 2018

Member

calling out the conflict is due to a uid mismatch might be clearer:

fmt.Errorf("uid mismatch: the provided object specified uid %s, and no existing object was found", objToUpdateUID)

This comment has been minimized.

@liggitt

liggitt Jul 4, 2018

Member

for reference, the uid precondition failure on delete prints this:

"the UID in the precondition (%s) does not match the UID in record (%s). The object might have been deleted and then recreated"

if objToUpdateHasUID, err := hasUID(objToUpdate); err != nil {
return nil, err
} else if objToUpdateHasUID {
return nil, errors.NewConflict(p.resource.GroupResource(), p.name, fmt.Errorf("object to create cannot specify UID"))

This comment has been minimized.

@liggitt

liggitt Jul 4, 2018

Member

for reference, the uid precondition failure on delete prints this:

"the UID in the precondition (%s) does not match the UID in record (%s). The object might have been deleted and then recreated"

// TestCreateOnApplyFailsWithUID makes sure that PATCH requests with the apply content type
// will not create the object if it doesn't already exist and it specifies a UID
func TestCreateOnApplyFailsWithUID(t *testing.T) {
// TODO: make setup do this optionally, and make it temporary rather than affecting global state.

This comment has been minimized.

@liggitt

liggitt Jul 4, 2018

Member

SetFeatureGateDuringTest will set a gate and give you a function you can call deferred to reset the gate to its original value

@@ -401,7 +408,7 @@ func (p *patcher) patchResource(ctx context.Context, scope RequestScope) (runtim
p.updatedObjectInfo = rest.DefaultUpdatedObjectInfo(nil, p.applyPatch, p.applyAdmission)
result, err := finishRequest(p.timeout, func() (runtime.Object, error) {
// TODO: Pass in UpdateOptions to override UpdateStrategy.AllowUpdateOnCreate
updateObject, created, updateErr := p.restPatcher.Update(ctx, p.name, p.updatedObjectInfo, p.createValidation, p.updateValidation, false)
updateObject, created, updateErr := p.restPatcher.Update(ctx, p.name, p.updatedObjectInfo, p.createValidation, p.updateValidation, p.forceAllowCreate)

This comment has been minimized.

@yue9944882

yue9944882 Jul 5, 2018

Member

@jennybuckley Could you sync with #65572? Checking CREATE operation in p.createValidation?😃

This comment has been minimized.

@jennybuckley

jennybuckley Jul 6, 2018

Author Contributor

Thanks for the review!

I think #65572 is to fix the admission called in create on update, but this PR is only concerned with create on patch. The server side apply feature branch, which this PR is being made to, already has a PR merged to it (#64892) which gets the right validating and mutating admission called. The staticAdmissionAttributes for create are set here, and then the p.createValidation is built using those attributes here

@jennybuckley jennybuckley force-pushed the jennybuckley:apply-can-post-6 branch from a2a02af to 1bb7c8a Jul 6, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 6, 2018

@jennybuckley jennybuckley force-pushed the jennybuckley:apply-can-post-6 branch 5 times, most recently from ec4f501 to 131b980 Jul 6, 2018

@jennybuckley jennybuckley force-pushed the jennybuckley:apply-can-post-6 branch from 131b980 to 0e37153 Jul 6, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jul 6, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 6, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, jennybuckley, liggitt

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jul 7, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

jennybuckley commented Jul 8, 2018

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jul 8, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

4 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jul 8, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jul 8, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jul 8, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jul 8, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@apelisse

This comment has been minimized.

Copy link
Member

apelisse commented Jul 9, 2018

/retest

It's been 16 times that this PR has been tested ... is it introducing flakes or is everything super flaky these days (I don't see how this could flake, but ...)?

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 9, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 9, 2018

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit 2ddbc51 into kubernetes:feature-serverside-apply Jul 9, 2018

16 of 17 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation jennybuckley authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Skipped
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.