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

Fix error for strategic merge patch of custom resources #53558

Merged

Conversation

nikhita
Copy link
Member

@nikhita nikhita commented Oct 7, 2017

Fixes #50037.

We need the go struct tags patchMergeKey and patchStrategy for fields that support a strategic merge patch. For native resources, we can easily figure out these tags since we know the fields.

Because custom resources are decoded as Unstructured and because we're missing the metadata about how to handle each field in a strategic merge patch, we can't find the go struct tags. Hence, we can't easily do a strategic merge for custom resources.

So we should fail fast and return an error.

Release note:

NONE

/cc @sttts @deads2k @ncdc

@k8s-ci-robot k8s-ci-robot requested review from deads2k and ncdc Oct 7, 2017
@k8s-ci-robot k8s-ci-robot added the release-note-none label Oct 7, 2017
@k8s-ci-robot k8s-ci-robot requested a review from sttts Oct 7, 2017
@k8s-ci-robot k8s-ci-robot added size/S cncf-cla: yes labels Oct 7, 2017
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Oct 7, 2017

Hi @nikhita. 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.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test label Oct 7, 2017
@nikhita
Copy link
Member Author

@nikhita nikhita commented Oct 7, 2017

Now, the error message is shown as:

$ cat crd.yaml
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  name: crontabs.stable.example.com
spec:
  group: stable.example.com
  version: v1
  scope: Namespaced
  names:
    plural: crontabs
    singular: crontab
    kind: CronTab
    shortNames:
    - ct

$ kubectl create -f crd.yaml
customresourcedefinition "crontabs.stable.example.com" created

$ cat crontab-object.yaml
apiVersion: "stable.example.com/v1"
kind: CronTab
metadata:
  name: my-new-cron-object
spec:
  cronSpec: "* * * * */5"
  image: my-awesome-cron-image

$ kubectl create -f crontab-object.yaml
crontab "my-new-cron-object" created

$ kubectl patch crontab my-new-cron-object -p '{"spec":{"image":"new-image"}}'
Error from server: cannot apply strategic merge patch for custom resources, try --type merge

@dims
Copy link
Member

@dims dims commented Oct 9, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test label Oct 9, 2017
@nikhita
Copy link
Member Author

@nikhita nikhita commented Oct 9, 2017

/retest

// each field in a strategic merge patch, we can't easily do a strategic merge for custom resources.
// So we should fail fast and return an error.
if t.Name() == "Unstructured" {
return nil, fmt.Errorf("cannot apply strategic merge patch for custom resources, try --type merge")
Copy link
Contributor

@sttts sttts Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the strategy patch logic without any metadata just the classical non-strategic patch? What does the user see now?

In general, I would prefer not to mention custom resources here in apimachinery. An error constant in vendor/k8s.io/apimachinery/pkg/util/mergepatch/errors.go with "ErrUnsupportedPatchFormat" would make more sense.

Copy link
Member Author

@nikhita nikhita Oct 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@nikhita nikhita force-pushed the cr-strategic-merge-patch branch from 7507ac4 to e93baef Compare Oct 10, 2017
@nikhita
Copy link
Member Author

@nikhita nikhita commented Oct 10, 2017

What does the user see now?

The user sees: Error from server: unable to find api field in struct Unstructured for the json field "spec".

After this update, it is Error from server: strategic merge patch format is not supported.

@ncdc
Copy link
Member

@ncdc ncdc commented Oct 10, 2017

@nikhita feel like adding a unit test?

@nikhita
Copy link
Member Author

@nikhita nikhita commented Oct 10, 2017

@ncdc Will do!

Also, maybe we should hold this off until #53379 (comment) is resolved (working on it).

@enisoc enisoc added this to Assigned in CustomResourceDefinition Oct 11, 2017
@apelisse
Copy link
Member

@apelisse apelisse commented Oct 26, 2017

Looks good to me.

@nikhita nikhita force-pushed the cr-strategic-merge-patch branch from e93baef to 2ae617e Compare Oct 31, 2017
@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Oct 31, 2017
@nikhita
Copy link
Member Author

@nikhita nikhita commented Oct 31, 2017

Added a test and updated the comment to add more detail into why we can't support strategic merge patch for custom resources.

// Because custom resources are decoded as Unstructured and because we're missing the metadata about how to handle
// each field in a strategic merge patch, we can't find the go struct tags. Hence, we can't easily do a strategic merge
// for custom resources. So we should fail fast and return an error.
if t.Name() == "Unstructured" {
Copy link
Contributor

@sttts sttts Nov 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would _, ok := dataStruct.(Unstructured); ok be the same?

Copy link
Member Author

@nikhita nikhita Nov 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_, ok := dataStruct.(*unstructured.Unstructured); ok would be the same (with the pointer). 👍

updated.

We need the go struct tags `patchMergeKey` and `patchStrategy`
for fields that support a strategic merge patch. For native
resources, we can easily figure out these tags since we know
the fields.

Because custom resources are decoded as Unstructured and
because we're missing the metadata about how to handle
each field in a strategic merge patch, we can't find the
go struct tags. Hence, we can't easily  do a strategic merge
for custom resources.

So we should fail fast and return an error.
@nikhita nikhita force-pushed the cr-strategic-merge-patch branch from 2ae617e to 79349c9 Compare Nov 1, 2017
@nikhita
Copy link
Member Author

@nikhita nikhita commented Nov 1, 2017

/retest

@sttts
Copy link
Contributor

@sttts sttts commented Nov 2, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 2, 2017
@k8s-github-robot
Copy link
Contributor

@k8s-github-robot k8s-github-robot commented Nov 2, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikhita, sttts

Associated issue: 50037

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved label Nov 2, 2017
@k8s-github-robot
Copy link
Contributor

@k8s-github-robot k8s-github-robot commented Nov 2, 2017

Automatic merge from submit-queue (batch tested with PRs 54800, 53898, 54812, 54921, 53558). If you want to cherry-pick this change to another branch, please follow the instructions here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved cncf-cla: yes lgtm release-note-none size/M
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants