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.

Sign up for GitHub

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

Jump to bottom

⚠ Client Server side apply #2702

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

⚠ Client Server side apply #2702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions pkg/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/runtime/serializer"
Expand Down Expand Up @@ -347,6 +348,21 @@ func (c *client) Patch(ctx context.Context, obj Object, patch Patch, opts ...Pat
}
}

func (c *client) Apply(ctx context.Context, obj Object, fieldOwner string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Pretty heavily opposed to do anything in here before kubernetes/kubernetes#118138 is done because:

  • The only type that can be used in here is unstructured.Unstructured
  • The converting as you do it down below doesn't fix the issue that the normal types end up putting zero values in fields
  • Which is the reason why applytypes exist, but you can not actually use applytypes here, because they do not fulfill client.Object
    /hold

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely hold, and no strong opinions on the feedback beyond understanding the gaps that exists. Waiting on this make sense and provided the support going forward, strategy around this seems crucial. Here for that conversation.

Copy link
Member

Choose a reason for hiding this comment

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

The whole topic boils down to "someone needs to do the work":

  1. The upstream issue I mentioned needs fixing, upstream signaled that they are ok with the idea but no one has shown up to actually put the work in
  2. This PR needs to be finished: [WIP] ✨ Generation of typed apply clients using upstream generator controller-tools#818
  3. A change like this in controller-runtime that adds an Apply method that only accepts apply types

var err error
switch obj.(type) {
case runtime.Unstructured:
return c.Patch(ctx, obj, Apply, ForceOwnership, FieldOwner(fieldOwner))
default:
u := &unstructured.Unstructured{}
u.Object, err = runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
if err != nil {
return err
}
return c.Patch(ctx, u, Apply, ForceOwnership, FieldOwner(fieldOwner))
}
}

// Get implements client.Client.
func (c *client) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
if isUncached, err := c.shouldBypassCache(obj); err != nil {
Expand Down
25 changes: 24 additions & 1 deletion pkg/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,7 +795,30 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC
})
})
})

Describe("Server side apply", func() {
Context("with a core k8s object", func() {
It("should not error", func() {
cl, err := client.New(cfg, client.Options{})
Expect(err).NotTo(HaveOccurred())
Expect(cl).NotTo(BeNil())
cm := &corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "ConfigMap",
},
ObjectMeta: metav1.ObjectMeta{
Name: "config-map",
Namespace: "default",
},
Data: map[string]string{
"key": "value",
},
}
err = cl.Apply(ctx, cm, "test-client")
Expect(err).NotTo(HaveOccurred())
})
})
})
Describe("SubResourceClient", func() {
Context("with structured objects", func() {
It("should be able to read the Scale subresource", func() {
Expand Down
4 changes: 4 additions & 0 deletions pkg/client/dryrun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ func (c *dryRunClient) Patch(ctx context.Context, obj Object, patch Patch, opts
return c.client.Patch(ctx, obj, patch, append(opts, DryRunAll)...)
}

func (c *dryRunClient) Apply(ctx context.Context, obj Object, fieldOwner string) error {
return c.client.Apply(ctx, obj, fieldOwner)
}

// Get implements client.Client.
func (c *dryRunClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
return c.client.Get(ctx, key, obj, opts...)
Expand Down
4 changes: 4 additions & 0 deletions pkg/client/fake/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,10 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.
return c.patch(obj, patch, opts...)
}

func (c *fakeClient) Apply(ctx context.Context, obj client.Object, fieldOwner string) error {
return c.Patch(ctx, obj, client.Apply, client.ForceOwnership, client.FieldOwner(fieldOwner))
}

func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
patchOptions := &client.PatchOptions{}
patchOptions.ApplyOptions(opts)
Expand Down
8 changes: 8 additions & 0 deletions pkg/client/interceptor/intercept.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type Funcs struct {
DeleteAllOf func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteAllOfOption) error
Update func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error
Patch func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error
Apply func(ctx context.Context, client client.WithWatch, obj client.Object, fieldOwner string) error
Watch func(ctx context.Context, client client.WithWatch, obj client.ObjectList, opts ...client.ListOption) (watch.Interface, error)
SubResource func(client client.WithWatch, subResource string) client.SubResourceClient
SubResourceGet func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, subResource client.Object, opts ...client.SubResourceGetOption) error
Expand Down Expand Up @@ -92,6 +93,13 @@ func (c interceptor) Patch(ctx context.Context, obj client.Object, patch client.
return c.client.Patch(ctx, obj, patch, opts...)
}

func (c interceptor) Apply(ctx context.Context, obj client.Object, fieldOwner string) error {
if c.funcs.Patch != nil {
return c.funcs.Apply(ctx, c.client, obj, fieldOwner)
}
return c.client.Apply(ctx, obj, fieldOwner)
}

func (c interceptor) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
if c.funcs.DeleteAllOf != nil {
return c.funcs.DeleteAllOf(ctx, c.client, obj, opts...)
Expand Down
4 changes: 4 additions & 0 deletions pkg/client/interceptor/intercept_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,10 @@ func (d dummyClient) Patch(ctx context.Context, obj client.Object, patch client.
return nil
}

func (d dummyClient) Apply(ctx context.Context, obj client.Object, fieldOwner string) error {
return nil
}

func (d dummyClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
return nil
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/client/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ type Writer interface {
// struct pointer so that obj can be updated with the content returned by the Server.
Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error

// Apply patches the given object in the Kubernetes cluster with a server side
// apply. obj must be a struct pointer so that obj can be updated with the
// content returned by the Server
Apply(ctx context.Context, obj Object, fieldOwner string) error

// DeleteAllOf deletes all objects of the given type matching the given options.
DeleteAllOf(ctx context.Context, obj Object, opts ...DeleteAllOfOption) error
}
Expand Down
17 changes: 17 additions & 0 deletions pkg/client/namespaced_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,23 @@ 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 Object, fieldOwner string) error {
isNamespaceScoped, err := n.IsObjectNamespaced(obj)
if err != nil {
return fmt.Errorf("error finding the scope of the object: %w", err)
}

objectNamespace := obj.GetNamespace()
if objectNamespace != n.namespace && objectNamespace != "" {
return fmt.Errorf("namespace %s of the object %s does not match the namespace %s on the client", objectNamespace, obj.GetName(), n.namespace)
}

if isNamespaceScoped && objectNamespace == "" {
obj.SetNamespace(n.namespace)
}
return n.client.Apply(ctx, obj, fieldOwner)
}

// Get implements client.Client.
func (n *namespacedClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
isNamespaceScoped, err := n.IsObjectNamespaced(obj)
Expand Down
21 changes: 21 additions & 0 deletions pkg/client/typed_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"context"

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/json"
)

var _ Reader = &typedClient{}
Expand Down Expand Up @@ -132,6 +134,25 @@ func (c *typedClient) Patch(ctx context.Context, obj Object, patch Patch, opts .
Into(obj)
}

func (c *typedClient) Apply(ctx context.Context, obj Object, fieldOwner string) error {
o, err := c.resources.getObjMeta(obj)
if err != nil {
return err
}

data, err := json.Marshal(o)
if err != nil {
return err
}
return o.Patch(types.ApplyPatchType).
NamespaceIfScoped(o.GetNamespace(), o.isNamespaced()).
Resource(o.resource()).
Name(o.GetName()).
Body(data).
Do(ctx).
Into(obj)
}

// Get implements client.Client.
func (c *typedClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
r, err := c.resources.getResource(obj)
Expand Down
26 changes: 26 additions & 0 deletions pkg/client/unstructured_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"strings"

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/json"
)

var _ Reader = &unstructuredClient{}
Expand Down Expand Up @@ -166,6 +168,30 @@ func (uc *unstructuredClient) Patch(ctx context.Context, obj Object, patch Patch
Into(obj)
}

func (uc *unstructuredClient) Apply(ctx context.Context, obj Object, fieldOwner string) error {
if _, ok := obj.(runtime.Unstructured); !ok {
return fmt.Errorf("unstructured client did not understand object: %T", obj)
}

o, err := uc.resources.getObjMeta(obj)
if err != nil {
return err
}

data, err := json.Marshal(obj)
if err != nil {
return err
}

return o.Patch(types.ApplyPatchType).
NamespaceIfScoped(o.GetNamespace(), o.isNamespaced()).
Resource(o.resource()).
Name(o.GetName()).
Body(data).
Do(ctx).
Into(obj)
}

// Get implements client.Client.
func (uc *unstructuredClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
u, ok := obj.(runtime.Unstructured)
Expand Down