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

Check for required name parameter in dynamic client #75072

Merged

Conversation

@lblackstone
Copy link
Contributor

commented Mar 6, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
The dynamic client in the client-go library was not validating that the required name parameter was non-empty for the Delete, Get, and Patch requests. If an empty string was provided, these methods would panic.

This PR adds a check for empty string and returns an error rather than panicking.

Which issue(s) this PR fixes:

Fixes #75069

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add name validation for dynamic client methods in client-go
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Hi @lblackstone. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@yue9944882

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

/ok-to-test

@@ -216,6 +217,9 @@ func (c *dynamicResourceClient) DeleteCollection(opts *metav1.DeleteOptions, lis
}

func (c *dynamicResourceClient) Get(name string, opts metav1.GetOptions, subresources ...string) (*unstructured.Unstructured, error) {
if name == "" {
return nil, fmt.Errorf("name is required")

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Mar 7, 2019

Member

this validation should also be applied to Patch, Delete, Update, UpdateStatus.

This comment has been minimized.

Copy link
@lblackstone

lblackstone Mar 7, 2019

Author Contributor

I updated Patch and Delete. The other ones didn't have a name parameter to validate.

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 21, 2019

Member

update/updatestatus can use accessor.GetName()

create should also validate name if len(subresources) > 0

@lblackstone lblackstone force-pushed the lblackstone:dynamic-get-name-validation branch from 6cfac3f to ec958c4 Mar 7, 2019

@k8s-ci-robot k8s-ci-robot added size/S and removed size/XS labels Mar 7, 2019

@lblackstone lblackstone changed the title Check for required name parameter on dynamic Get Check for required name parameter in dynamic client Mar 7, 2019

@fedebongio

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

/assign @caesarxuchao

@yue9944882

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

/lgtm
/cc @liggitt
for approval

Check for required name parameter in dynamic client
The Create, Delete, Get, Patch, Update and UpdateStatus
methods in the dynamic client all expect the name
parameter to be non-empty, but did not validate this
requirement, which could lead to a panic. Add explicit
checks to these methods.

@lblackstone lblackstone force-pushed the lblackstone:dynamic-get-name-validation branch from ec958c4 to a9cba03 Mar 21, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 21, 2019

@lblackstone

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

I rebased on master and added the name check to Create, Update and UpdateStatus

@@ -102,6 +103,9 @@ func (c *dynamicResourceClient) Create(obj *unstructured.Unstructured, opts meta
return nil, err
}
name = accessor.GetName()
if len(name) == 0 {
return nil, fmt.Errorf("name is required")

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Mar 21, 2019

Member

name can be empty, if we set the generateName

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 21, 2019

Member

not when dealing with a subresource

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Mar 21, 2019

Member

oh tricked. it's subresource.

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lblackstone, 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

@lblackstone

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

The pull-kubernetes-e2e-gce-100-performance test has failed several times today, but I don't think the failure would be related to this change. I'll retry again since it passed before.

@lblackstone

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

/retest

@fejta-bot

This comment has been minimized.

Copy link

commented Mar 22, 2019

/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 or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 0118630 into kubernetes:master Mar 22, 2019

17 checks passed

cla/linuxfoundation lblackstone authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps 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-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@lblackstone lblackstone deleted the lblackstone:dynamic-get-name-validation branch Apr 2, 2019

k8s-ci-robot added a commit that referenced this pull request May 7, 2019

Merge pull request #76200 from mvladev/automated-cherry-pick-of-#7507…
…2-upstream-release-1.13

Automated cherry pick of #75072: Check for required name parameter in dynamic client

k8s-ci-robot added a commit that referenced this pull request May 10, 2019

Merge pull request #76199 from mvladev/automated-cherry-pick-of-#7507…
…2-upstream-release-1.14

Automated cherry pick of #75072: Check for required name parameter in dynamic client
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.