-
Notifications
You must be signed in to change notification settings - Fork 1.2k
⚠️ Add native SSA support #3253
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
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman 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 |
@@ -53,6 +54,10 @@ func (c *clientWithFieldValidation) Patch(ctx context.Context, obj Object, patch | |||
return c.client.Patch(ctx, obj, patch, append([]PatchOption{c.validation}, opts...)...) | |||
} | |||
|
|||
func (c *clientWithFieldValidation) Apply(ctx context.Context, obj runtime.ApplyConfiguration, opts ...ApplyOption) error { | |||
return errors.New("Apply is not supported with field validation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: It actually is, just not part of the upstream apply options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, this doesn't work - SSA will error out if there are unknown fields even when FieldValidation=None is set. What is the best way for us to deal with that in this wrapper, just erroring doesn't seem great...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no way to adjust the interfaces to have something separate for Apply without making it awkward to use I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on what you mean by adjust the interfaces to have something separate for Apply
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of this PR adds Appply
to the Client interface right? What if there was an ApplyClient
extension to the interface that embeds the Client
interface? Would that allow us to implement the Apply within only the different client mutators that we are able to support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of that would be that you have two clients, one for apply, one for everything else - You can also achieve that with what we have here aleady by storing the client before wrapping and using that for apply or not?
The best I can think of in these wrappers is a config option to not error when apply is used 🤔
@@ -147,6 +148,10 @@ func (n *namespacedClient) Patch(ctx context.Context, obj Object, patch Patch, o | |||
return n.client.Patch(ctx, obj, patch, opts...) | |||
} | |||
|
|||
func (n *namespacedClient) Apply(ctx context.Context, obj runtime.ApplyConfiguration, opts ...ApplyOption) error { | |||
return errors.New("Apply is not supported on namespaced client") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Can we make this work? All the ACs should have a SetNamespace
, this would be nice to have
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actualy pretty difficult, I'd prefer it if we punt on this for now, it can still be done in a follow-up if there is interest.
func (u *unstructuredApplyConfiguration) IsApplyConfiguration() {} | ||
|
||
// ApplyConfigurationFromUnstructured creates a runtime.ApplyConfiguration from an *unstructured.Unstructured object. | ||
func ApplyConfigurationFromUnstructured(u *unstructured.Unstructured) runtime.ApplyConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this doesn't exist anywhere in the API machinery already today 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upstream didn't want that: kubernetes/kubernetes#132194 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some pretty strong arguments against creating this in the thread there, what would the likely impact to CR users be if we decided that ApplyConfiguration through CR was only supported for concrete types and not unstructured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my main concern would be
It doesn't, as the typed object has zero values which will then be present in the unstructured.
How do we elevate this message to help prevent folks from shooting themselves in the foot?
In general, marshalling zero values is a bad idea, but there's a lot of legacy required fields out there without omitempty assigned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some pretty strong arguments against creating this in the thread there, what would the likely impact to CR users be if we decided that ApplyConfiguration through CR was only supported for concrete types and not unstructured?
We break all existing usages of Apply in controller-runtime and make it impossible to apply objects for which you do not have the AC, for example because you read them from disk.
How do we elevate this message to help prevent folks from shooting themselves in the foot?
Its a nonsensical argument. That issue only exists if someone converts from concrete type to unstructured, there is no reason to do that. Using unstructured.Unstrucutred itself is fine, as it has no zero values. People could also use a serializer to convert from conrete type to applyconfig and the exact same issue would arise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding godoc to the functions here to explain that this should not be used when converting structured types to unstructured would cover us in terms of messaging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what is the reason why anyone would ever do that? And what makes us assume that is any more likely than converting from API type to AC type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, but since it's possible, someone will probably be doing it. Since we know it causes issues, IMO it makes sense to document that this is not something we recommend.
I think I'd put this down to Hyrum's Law
pkg/client/fake/client_test.go
Outdated
|
||
err := cl.Apply(context.Background(), obj, client.FieldOwner("\x00")) | ||
Expect(err).To(HaveOccurred()) | ||
Expect(apierrors.IsInvalid(err)).To(BeTrue()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, with BeTrue()
, when this fails it's hard to understand why, you just get expected False to be True
, if you add a comment after BeTrue()
with the context it's much easier to debug failures
Expect(apierrors.IsInvalid(err)).To(BeTrue()) | |
Expect(apierrors.IsInvalid(err)).To(BeTrue(), "Expected error to be an invalid error") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updated it
@@ -53,6 +54,10 @@ func (c *clientWithFieldValidation) Patch(ctx context.Context, obj Object, patch | |||
return c.client.Patch(ctx, obj, patch, append([]PatchOption{c.validation}, opts...)...) | |||
} | |||
|
|||
func (c *clientWithFieldValidation) Apply(ctx context.Context, obj runtime.ApplyConfiguration, opts ...ApplyOption) error { | |||
return errors.New("Apply is not supported with field validation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no way to adjust the interfaces to have something separate for Apply without making it awkward to use I guess?
|
||
// Force is going to "force" Apply requests. It means user will | ||
// re-acquire conflicting fields owned by other people. | ||
Force *bool `json:"force" protobuf:"varint,2,opt,name=force"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expose a force applyoption already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, there is already ForceOwnership
, I updated it to also support Apply
and not only Patch
This change adds native server-side apply support to the client by extending it with an `Apply` method that takes an `runtime.ApplyConfiguration`.
This reverts commit ad1966a. This doesn't work, the server will always error on additional fields when using SSA, even when fieldValiation=None is configured.
This change adds native server-side apply support to the client by extending it with an
Apply
method that takes aruntime.ApplyConfiguration
.Ref #3183