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

Prevent resourceVersion updates for custom resources on no-op writes #67562

Merged

Conversation

nikhita
Copy link
Member

@nikhita nikhita commented Aug 18, 2018

Fixes partly #67541

For ObjectMeta pruning, we round trip by marshalling and unmarshalling. If the ObjectMeta contained any strings with "" (or other fields with empty values) and the respective fields are omitempty, those fields will be lost in the round trip process.

This makes ObjectMeta after the no-op write different from the one before the write.

Resource version is incremented every time data is written to etcd. Writes to etcd short-circuit if the bytes being written are identical to the bytes already present.

So this ends up incrementing the resourceVersion even on no-op writes. This PR updates the BeforeUpdate function such that omitempty fields have values set only if they are non-zero so that they produce an unstructured object that matches ObjectMeta omitempty semantics.

/sig api-machinery
/kind bug
/area custom-resources
/assign sttts liggitt

Release note:

Prevent `resourceVersion` updates for custom resources on no-op writes.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/bug Categorizes issue or PR as related to a bug. area/custom-resources size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 18, 2018
value interface{}
}

func getImplicitFields(u *unstructured.Unstructured) ([]implicitField, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I want to get an initial review on this before adding all fields in objectMeta here.

Copy link
Member Author

Choose a reason for hiding this comment

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

And if we end up setting these fields explicitly, do we still want to round trip through marshalling and unmarshalling or go field by field accumulating into the metadata object by iterating through the metadataMap?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right layer at which to do this. I think the unstructured objectmeta accessor implementations should be adjusted to produce an unstructured object that matches ObjectMeta omitempty semantics

Copy link
Member

Choose a reason for hiding this comment

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

For example, if SetClusterName("") is called, that should remove the clusterName element from the unstructured object

Copy link
Member

@yue9944882 yue9944882 Aug 18, 2018

Choose a reason for hiding this comment

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

tricky one: do we need to do cascading element removal in unstructured accessor for omitempty semantics?

Copy link
Member

Choose a reason for hiding this comment

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

I think all the accessor methods are for top level fields in metadata, so I'm not sure that's an issue

Copy link
Member Author

Choose a reason for hiding this comment

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

updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the unstructured objectmeta accessor implementations should be adjusted to produce an unstructured object that matches ObjectMeta omitempty semantics

The accessors were already modified to support this for some of the fields. Added for the rest.

@nikhita nikhita force-pushed the customresource-subresource-patch branch 3 times, most recently from 3d9ccfa to a331c23 Compare August 18, 2018 18:11
@nikhita
Copy link
Member Author

nikhita commented Aug 18, 2018

I'm wondering if this is fine to cherry-pick to 1.11 or not. If someone uses client-go to interact with unstructured objects, this could lead to breaking changes for them.

@nikhita
Copy link
Member Author

nikhita commented Aug 18, 2018

This additionally fixes two other issues as well: #48211 and #49075. 🎉

@liggitt
Copy link
Member

liggitt commented Aug 18, 2018

I'm wondering if this is fine to cherry-pick to 1.11 or not. If someone uses client-go to interact with unstructured objects, this could lead to breaking changes for them.

It's important to resolve the incrementing resourceVersion issue for 1.11. Were the only fields force-set to an empty value in BeforeCreate/BeforeUpdate the clusterName and initializers fields? We may be able to make a more targeted fix for 1.11 to only force-set those fields to empty values if they contain non-empty values

@nikhita
Copy link
Member Author

nikhita commented Aug 18, 2018

We may be able to make a more targeted fix for 1.11 to only force-set those fields to empty values if they contain non-empty values

👍

Were the only fields force-set to an empty value in BeforeCreate the clusterName and initializers fields?

In BeforeCreate:

  1. Namespace
  2. DeletionTimestamp
  3. DeletionGracePeriodSeconds
  4. Initializers
  5. ClusterName

In BeforeUpdate:

  1. Namespace
  2. Initializers
  3. ClusterName

But I've noticed that we need to include selfLink and resourceVersion as well (they were creating problems in tests). I think this is because they are cleared prior to writing to etcd.

@nikhita
Copy link
Member Author

nikhita commented Aug 18, 2018

I'll create a PR for 1.11 once this looks good to merge.

@@ -179,6 +179,11 @@ func (u *Unstructured) GetOwnerReferences() []metav1.OwnerReference {
}

func (u *Unstructured) SetOwnerReferences(references []metav1.OwnerReference) {
if references == nil {
RemoveNestedField(u.Object, "metadata", "ownerReferences")
Copy link
Member

Choose a reason for hiding this comment

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

hi @nikhita, how about having an additional check to respect the field tag omitempty via reflect? we might only do the removal when it's tagged, amiright?

Copy link
Member

Choose a reason for hiding this comment

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

it's working tho, but a dynamic approach would be better for maintenance

Copy link
Member

Choose a reason for hiding this comment

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

Reflection is slow to use in the normal path. A reflection based unit test to catch drift might be better.

Copy link
Member

Choose a reason for hiding this comment

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

Reflection is slow to use in the normal path.

true. it's a heavy cost to do the reflection every time accessing normal paths. or we could simply have a thread-unsafe map like map[(path)string](isOmitemptyField)bool and put all things we need inside init() call

Copy link
Member Author

@nikhita nikhita Aug 20, 2018

Choose a reason for hiding this comment

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

A reflection based unit test to catch drift might be better.

I added a very simple test and didn't use reflection because all fields in objectMeta are omitempty. Ptal.

how about having an additional check to respect the field tag omitempty via reflect

Given that reflection is costly and the metadata accessors are for metadata and we already know which fields are omitempty in it, I think it's overkill to add the reflection to the accessors.

or we could simply have a thread-unsafe map like map[(path)string](isOmitemptyField)bool

This could be incorporated into the generalized "setNestedField..` funcs in helpers.go. However, IMO if we want to introduce methods which look for the omitempty tag, they should be created as new functions and not be added into the existing helper functions.

Most of the existing ones are used heavily in apiextensions-apiserver where we already know if omitempty is needed or not. Adding reflection to the existing ones will make all these function calls slow.

@nikhita nikhita force-pushed the customresource-subresource-patch branch from 0837659 to 8f53817 Compare August 20, 2018 07:13
// set zero values for all fields in metadata explicitly
// since all fields in objectMeta are omitempty, these fields should never be set
// and the resulting metadata should be an empty metadata
u.SetName("")
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we set random values first?

Copy link
Member Author

@nikhita nikhita Aug 20, 2018

Choose a reason for hiding this comment

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

Added another test for fuzzing. I let the original test for omitempty be as-is just to make sure we don't miss any cases for zero values (fuzzing should catch but better to be safe :))

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to do fuzzing before these to catch missing calls. If a field is added to ObjectMeta now, this list gets incomplete, but we don't notice. If we fuzz before, we would.

@nikhita nikhita force-pushed the customresource-subresource-patch branch from 8f53817 to f36abd1 Compare August 20, 2018 09:12
@@ -790,6 +790,9 @@ func structToUnstructured(sv, dv reflect.Value) error {
if err := toUnstructured(fv, subv); err != nil {
return err
}
if fieldInfo.omitempty && subv.IsNil() {
continue
Copy link
Member Author

@nikhita nikhita Aug 20, 2018

Choose a reason for hiding this comment

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

@sttts @liggitt @yue9944882 ptal.

Without this change,

    emptyMetadata := &metav1.ObjectMeta{}
    metadata, err := runtime.DefaultUnstructuredConverter.ToUnstructured(emptyMetadata)
    if err != nil {
        t.Fatal(err)
    }

will produce metadata as map[creationTimestamp:<nil>]. creationTimestamp has the omitempty tag as well, so this should not be present in the map.

CreationTimestamp is a metav1.Time. The conversion looks for a custom marshaller and gets the null value and then simply returns nil (without looking at the omitempty tag).

func (t Time) MarshalJSON() ([]byte, error) {
if t.IsZero() {
// Encode unset/nil objects as JSON's "null".
return []byte("null"), nil
}

data, err := marshaler.MarshalJSON()
if err != nil {
return err
}
switch {
case len(data) == 0:
return fmt.Errorf("error decoding from json: empty value")
case bytes.Equal(data, nullBytes):
// We're done - we don't need to store anything.

With this change, it looks for the omitempty tag again and does not store the value if it is nil.

Copy link
Member Author

@nikhita nikhita Aug 20, 2018

Choose a reason for hiding this comment

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

Looks like this isnt' right. json.Marshal +Unmarshal will set creationTimestamp: nil and we always need to ensure that it satisfies json marshalling + unmarshalling.

newErr := toUnstructuredViaJSON(obj, &newUnstr)

This will fail the already existing round trip test for unstructured:

func TestRoundTrip(t *testing.T) {

Interesting to note that we also have a round trip test specifically for creationTimestamp but it just checks if conversion is possible and not that the round trip values match.

func TestRoundTripWithEmptyCreationTimestamp(t *testing.T) {

Copy link
Member Author

@nikhita nikhita Aug 20, 2018

Choose a reason for hiding this comment

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

What should be the intended behaviour here? Should creationTimestamp be present in the map or not? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

What should be the intended behaviour here? Should creationTimestamp be present in the map or not? thinking

answering myself: from the docs

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

omitempty has no meaning for a struct, so creationTimestamp should be present in the map. It is weird that we set the value as nil but in any case, it should be present in the unstructured object/map.

Copy link
Member

@yue9944882 yue9944882 Aug 20, 2018

Choose a reason for hiding this comment

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

shouldn't omitempty be removed from CreateTimestamp then? maybe in another pull? it's okay with or without it tho.

Copy link
Member Author

@nikhita nikhita Aug 20, 2018

Choose a reason for hiding this comment

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

shouldn't omitempty be removed from CreateTimestamp then? maybe in another pull?

yeah, I think we should remove it too. But I thought it was suited for another PR (since it would involved schema changes too afaik and would be a breaking change).

Also, wanted to get feedback on the current changes before removing it. :)

@nikhita nikhita force-pushed the customresource-subresource-patch branch 2 times, most recently from 58f3b64 to 63df4e1 Compare August 20, 2018 11:56
@nikhita
Copy link
Member Author

nikhita commented Aug 20, 2018

Updated. Ptal.

@nikhita nikhita force-pushed the customresource-subresource-patch branch 2 times, most recently from 6d33bef to a0584c7 Compare August 20, 2018 12:44
@liggitt
Copy link
Member

liggitt commented Aug 20, 2018

/hold cancel
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 20, 2018
@roycaihw
Copy link
Member

/lgtm

@nikhita
Copy link
Member Author

nikhita commented Aug 21, 2018

/retest

@nikhita
Copy link
Member Author

nikhita commented Aug 21, 2018

For the record:

For ObjectMeta pruning, we round trip through marshalling and
unmarshalling. If the ObjectMeta contained any strings with "" (or other
fields with empty values) _and_ the respective fields are omitempty,
those fields will be lost in the round trip process.

This makes ObjectMeta after the no-op write different from the one
before the write.

Resource version is incremented every time data is written to etcd.
Writes to etcd short-circuit if the bytes being written are identical
to the bytes already present. So this ends up incrementing the
resourceVersion even on no-op writes.

The zero values are set in BeforeCreate and BeforeUpdate. This commit
updates BeforeUpdate such that zero values are only set when the
object does not have a zero value for the respective field.
@nikhita nikhita force-pushed the customresource-subresource-patch branch from 94588bf to d691748 Compare August 21, 2018 10:35
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/apiserver and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 21, 2018
@nikhita
Copy link
Member Author

nikhita commented Aug 21, 2018

Picked the test changes from @sttts's PR: #67637 (as discussed with @sttts on slack on the #sig-api-machinery channel).

@sttts
Copy link
Contributor

sttts commented Aug 21, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2018
@nikhita
Copy link
Member Author

nikhita commented Aug 21, 2018

/retest

@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 816f2a4 into kubernetes:master Aug 21, 2018
k8s-github-robot pushed a commit that referenced this pull request Aug 22, 2018
…ch-04

Automatic merge from submit-queue (batch tested with PRs 67298, 67518, 67635, 67673). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix unstructured metadata accessors to respect omitempty semantics

Fixes #67541
Fixes #48211 
Fixes #49075
Follow up of #67562

`ObjectMeta` has fields with `omitempty` json tags. This means that when the fields have zero values, they should not be persisted in the object.

Before this PR, some of the metadata accessors for unstructured objects did not respect these semantics i.e they would persist a field even if it had a zero value.

This PR updates the accessors so that the field is removed from the unstructured object map if it contains a zero value.

/sig api-machinery
/kind bug
/area custom-resources
/cc sttts liggitt yue9944882 roycaihw 
/assign sttts liggitt 

**Release note**:

```release-note
NONE
```
@nikhita nikhita deleted the customresource-subresource-patch branch August 22, 2018 08:11
k8s-github-robot pushed a commit that referenced this pull request Aug 28, 2018
…2-upstream-release-1.11

Automatic merge from submit-queue.

Automated cherry pick of #67562: Prevent resourceVersion updates for custom resources on no-op writes

Cherry pick of #67562 on release-1.11.

#67562: Prevent resourceVersion updates for custom resources on no-op writes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/custom-resources cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants