🐛 Fakeclient: Honor AllowUnconditionalUpdate and AllowCreateOnUpdate for resources that support it

[detiber]

As of v0.6.0, the fake client is rejecting Updates where the ResourceVersion is not set on the object where previously it did not.

It looks like this was introduced as part of #832 to reject Updates where the ResourceVersion does not match, however it didn't special case the case where ResourceVersion is not set on the resource provided to Update but the existing resource has ResourceVersion set.

This PR adds support for calling Update without a Resource version for resources that allow unconditional updates and calling Update for resources that allow create on update if they do not already exist.

[detiber]

Ok, so it turns out that this behavior is resource dependent and whether or not the resource is configured to allow "Unconditional Updates". ConfigMap happens to be one of the resources that does this: https://github.com/kubernetes/kubernetes/blob/37d9c22abe5921da1d9c61bc92d054eebfb01515/pkg/registry/core/configmap/strategy.go#L99-L101

Open to ideas on if (and potentially how) we'd want to replicate this, since the current behavior is definitely broken for ConfigMaps, but is correct for resources that are configured to return false for AllowUnconditionalUpdate()

[alvaroaleman]

IMHO doing UPDATE with an empty resourceVersion is patch in disguise. I do now know why this is possible on some resources but I think if ppl want to do Patch, they should do that, rather than an Update with an empty resourceVersion. WDYT?

[detiber]

Following up with a bit of additional context from the Slack thread on this topic

The per-resource behavior for update when ResourceVersion is not set is dependent on the value passed back by the AllowUnconditionalUpdate() method. For ConfigMaps that is defined here

I've yet to find any way to introspect this value from the scheme or openapi data published.

I don't think we really want to import the registry code into controller-runtime, which leaves us with the following options that were proposed by @alvaroaleman on how we should support this in the fake client:
a) Make the fake client differ from the real client in that it always allows that
b) Make the fake client differ from the real client in that it never allows that
c) Replicating the list somehow (import? hardcode?)

Since this only applies to built-in types, replicating this via hard-coding is probably not the worst thing we could do, we could also likely implement it in a way that we'd only have to special case the types that return true for AllowUnconditionalUpdate() (such as ConfigMap).

[shawn-hurley]

Since this only applies to built-in types, replicating this via hard-coding is probably not the worst thing we could do, we could also likely implement it in a way that we'd only have to special case the types that return true for AllowUnconditionalUpdate() (such as ConfigMap).

Who is responsible for keeping this in sync? I worry about it changing from allowed to disallowed and folks who write their tests get runtime errors.

b) Make the fake client differ from the real client in that it never allows that

This has the added benefit of always forcing the user to do something that will work on clusters even if in future versions this method returns a different answer for a given internal type.

[detiber]

Who is responsible for keeping this in sync? I worry about it changing from allowed to disallowed and folks who write their tests get runtime errors.

I don't expect it to affect many resources, but I have yet to dig into the exact numbers yet, and I wouldn't expect it to change for any resources (outside of an update to the apiversion for the resource), since it would be a breaking change to k8s itself if it does.

This has the added benefit of always forcing the user to do something that will work on clusters even if in future versions this method returns a different answer for a given internal type.

Considering this would be a breaking change with k8s itself, I wouldn't expect future versions to return a different answer for a given internal type outside of an apiversion update for that resource.

I'm not a huge fan of trying to replicate the behavior, but if we don't then the alternative is a fake client that is of less value to users otherwise.

Telling users to update their code (that is doing something completely valid against a real API server) to be able to use the fake client doesn't seem like the right approach to me, especially since the behavior that we are discussing was just recently introduced and breaks existing consumers.

[detiber]

I just went through k/k to determine which resources would be affected, and it is more than I would have expected. I also documented the list of resources that allow for Create on Update while I was trolling the code.

AllowUnconditionalUpdate() returns true:
apps.ControllerRevision
apps.DaemonSet
apps.Deployment
apps.ReplicaSet
apps.StatefulSet
autoscaling.HorizontalPodAutoscaler
batch.CronJob
batch.Job
certificates.Certificates
core.ConfigMap
core.Endpoint
core.Event
core.LimitRange
core.Namespace
core.Node
core.PersistentVolume
core.PersistentVolumeClaim
core.Pod
core.PodTemplate
core.ReplicationController
core.ResourceQuota
core.Secret
core.Service
core.ServiceAccount
core.EndpointSlice
events.Event
flowcontrol.FlowSchema
flowcontrol.PriorityLevelConfiguration
networking.Ingress
networking.IngressClass
networking.NetworkPolicy
policy.PodSecurityPolicy
rbac.ClusterRole
rbac.ClusterRoleBinding
rbac.Role
rbac.RoleBinding
scheduling.PriorityClass
settings.PodPreset
storage.StorageClass

There is also another method for registry strategy AllowCreateOnUpdate that we should likely take into account as well.
AllowCreateOnUpdate returns true:
coordination.Lease
core.Endpoint
core.Event
core.LimitRange
core.Service
node.RuntimeClass
rbac.ClusterRole
rbac.ClusterRoleBinding
rbac.Role
rbac.RoleBinding

[detiber]

As an interesting side note, since there is only a single storage version, these settings apply for all resources regardless of user-provided version for a particular api group and type

[detiber]

Additional data point: kubernetes/kubernetes#21330

My take away from that issue is that we shouldn't expect any new resources to set AllowUnconditionalUpdate, but the current resources that allow it will likely keep allowing it until v2 of their resource is released.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[alvaroaleman]

@detiber it would be good if this would get finished. Do you think you will be able to do that? Otherwise we should maybe create an issue to see if someone from the community can pick it up?

[detiber]

@alvaroaleman I should be able to pick this back up next week, thanks for the reminder.

[detiber]

@alvaroaleman updated, ptal.

[alvaroaleman]

Thanks!
/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, detiber

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:

• OWNERS [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment