Why not move apply to the server via a formal API?

@bgrant0607 asked a similar question.

My hope as that the steps in this proposal will make moving the logic to an API easier by reducing the complexity of the existing solution and making it a modular package that does not depend on cobra or other kubectl libraries to diff & update objects.

I'd be open to exposing the re-architected libraries as an API.

Because that's how we got strategic merge :P In the general case, the server can't know whether particular operations should be "replace" or "merge", and trying to capture all that flexibility in the API has created exactly the sort of complexity that motivated this doc. The alternative is just to do it client-side, where naturally there is more context available to be able to implement the exact semantics intended by the client.

Any examples in the API schema where it shouldn't be obvious? In the few ordered lists we have (e.g., command, args), those need to be replaced. All other cases should be merged. In the case of no conflict between the configured state and the live state, a merge and a replacement should be the same.

do you still plan to record last-applied as an annotation?
discussion of the versioning issues (apply v1alpha1, time passes, want to apply v1beta2) would be good as well

Yes, this proposal does not change what is stored for any of the objects, and works exactly the same with the exception that it uses PUT instead of PATCH to perform the update. It might best to solve the issues around versioning in a server-side API. I imagine the server side API would type convert all 3 objects (recorded from annotation, local from request and remote from live-lookup) to the same version before performing the diff-merge-update.

using what merge strategies?

The merge strategy / behavior would be exactly the same as the current version of apply, though more easily extended due to code structuring.

Could you expand on the readability? Is this discussing strategic merge patch? Is there special syntax in the typical case other than for deletions? Where has the deletion syntax been made unreadable?

Yes this is strategic merge patch. I think there are 2 main problems, there are several different directives now (delete, delete-primitive, ordering), and how the patch will be applied is communicated out of band. There are still a number of cases that are not supported by strategic patch that may require additional directives to support if done client side (e.g. how do we change the merge key for a field)

Directives:

• deletion of complex objects from lists
• deletion of primitive objects from lists (looks different)
• absolute ordering of items in a list

Additional issues with readability:

• merge keys for lists are not contained in the patch itself - unclear from looking at a patch whether it is adding an item to a list that looks similar to another item, or if it is updating the existing item in a list
• merge strategy not clear - are items in a list being replace, or are they being merged, (or using retain keys)?
• server silently drops directives that it doesn't recognize, so they may or may-not be used by the server - e.g. ordering may be randomized even with a directive specifying the ordering

retainKeys is also a directive that you didn't mentioned above.

We would version it by adding new content types, which I believe we also do with proto encodings.

But discoverability of supported versions is a good point.

Re. discoverability: It's possible to specify supported content types for each verb X resource via OpenAPI

Please add something about versioning the patch API in the future.

Why is dry run useless? Because it is also buggy and incomplete? Because it is implemented in the client rather than in the server?

It doesn't show the user what they really want to know - what will be the new object in the cluster after apply is run. It somewhat shows the changes being made, but without the context of what they are being made on.

• I would expect apply users care less about the patch format itself, and care more about the object in the cluster that will result from the patch.
• The patch output obfuscates validation errors caused by the values set by patch conflicting with other values set on the live object missing from the config (e.g. rolloutStrategy for Deployment)
• The patch itself is buggy and broken cases are hard to see just by looking at a patch (e.g. container ports merge key being incorrect)
• The server may apply the patch differently than the client, so running the patch locally against the file may result in something different than what the server does (e.g. the server doesn't support optional directives because it is skewed)

FWIW, I don't understand what apply's current patch shows me when I do --dry-run -o json. It looks like it is just showing me the local object without applying the patch.

Links to exemplary bugs?

For me: links to bugs are below.

True.

However:

• My impression is that most of the complexity is not in the directives but in the API schema and defaulting behaviors, as I mentioned in the server-side apply proposal.
• The schema needs to be accurately communicated to the client from the server (e.g., via extended OpenAPI) for client-side patch operations in order to support API aggregation and CRD, which re-introduces versioning and skew issues until the syntax stabilizes, but actually would improve skew behavior once it does.
• We need to support these patch behaviors independently of apply.

My impression is that most of the complexity is not in the directives but in the API schema and defaulting behaviors, as I mentioned in the server-side apply proposal.

Constructing a set of directives to order and delete items from a list results in more complicated code than just deleting the objects from the list.

The schema needs to be accurately communicated to the client from the server (e.g., via extended OpenAPI) for client-side patch operations in order to support API aggregation and CRD, which re-introduces versioning and skew issues until the syntax stabilizes, but actually would improve skew behavior once it does.

Agreed. This proposal should also work independently of whether it is done on the client or server. The focus of this proposal is on rewriting the 3-way diff code to be maintainable. The inputs will be the 3 versions of the object and the output will be the new object.

We need to support these patch behaviors independently of apply.

Yes, but why do we need to use them in apply?

I'm not sure that's the case. I thought resourceVersion could be provided in patches.

Specifically, GET, merge, and then try to PUT again.

last-applied-configuration?

We had also discussed sending the full configured resource decorated with deletion directives, which would have been slightly larger.

I want overlays to work, so I think all of these problems still need to be solved for strategic merge patch.

Agreed. We should talk offline about this, since the requirements for overlays may be a subset of what is need for apply to work. The semantics of apply are a bit different (though there is a lot of overlap) since apply is merging 3 modified versions of the same object to produce an updated object. Whereas overlays do work in this context.

Note that this would still change user-visible behavior. While technically bug fixes, we should consider how to communicate such changes to users, whether we need to retain prior behavior, and how we should transition to implementations with different behavior.

Yes this makes sense.

In some cases the fixes purely fix something that was broken or unspecified (e.g. maintaining ordering for lists when merging is strictly more correct than randomizing the ordering). In other cases I think it is pretty clearly an improvement, but the change should be communicated (e.g. deleting an item from a primitive lists now deletes it).

The cases more related to your point would be fixing an unintended / broken merge-key or merge-strategy.

It may be worth to point out, the size will be doubled, but I think it's fine.
Before: object in annotaion (1) + patch (delta) ~ 1
After: object in annotation (1) + new object (1) = 2

the live resources based on the file contents

Update currently and previously configured fields, without clobbering fields set by other means, such as imperative kubectl commands, other deployment and management tools, admission controllers, initializers, horizontal and vertical autoscalers, operators, and other controllers.

I would move this below. It's a detail of field updates.

Much of this pain has come from trying to paper over API schema complexity in an ad hoc incremental fashion using imperative patch directives.

I thought we agreed that client-server was not the main issue.

In order to make apply sufficiently maintainable and extensible to new API types, as well as to make its behavior more intuitive for users, the merge behavior, including how it is specified in the API schema, must be systematically redesigned and more thoroughly tested.

For me: links to bugs are below.

has been frequently expanded

It doesn't have to be this way.

Again, please rephrase. The schema MUST come from the server, in order to properly work with version skew, API aggregation, CRDs, etc.

Please add something about versioning the patch API in the future.

retry back to step 1?

+1

That was Brian :)