Skip to content

🐛 respect context in unstructured client #812

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

Merged
merged 3 commits into from
Feb 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 9 additions & 14 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,12 @@ package client
import (
"context"
"fmt"
"reflect"

"k8s.io/apimachinery/pkg/api/meta"
"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"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
Expand Down Expand Up @@ -70,25 +68,22 @@ func New(config *rest.Config, options Options) (Client, error) {
}
}

dynamicClient, err := dynamic.NewForConfig(config)
if err != nil {
return nil, err
clientcache := &clientCache{
config: config,
scheme: options.Scheme,
mapper: options.Mapper,
codecs: serializer.NewCodecFactory(options.Scheme),
resourceByType: make(map[schema.GroupVersionKind]*resourceMeta),
}

c := &client{
typedClient: typedClient{
cache: clientCache{
config: config,
scheme: options.Scheme,
mapper: options.Mapper,
codecs: serializer.NewCodecFactory(options.Scheme),
resourceByType: make(map[reflect.Type]*resourceMeta),
},
cache: clientcache,
paramCodec: runtime.NewParameterCodec(options.Scheme),
},
unstructuredClient: unstructuredClient{
client: dynamicClient,
restMapper: options.Mapper,
cache: clientcache,
paramCodec: noConversionParamCodec{},
},
}

Expand Down
23 changes: 10 additions & 13 deletions pkg/client/client_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package client

import (
"reflect"
"strings"
"sync"

Expand Down Expand Up @@ -45,19 +44,14 @@ type clientCache struct {
codecs serializer.CodecFactory

// resourceByType caches type metadata
resourceByType map[reflect.Type]*resourceMeta
resourceByType map[schema.GroupVersionKind]*resourceMeta
mu sync.RWMutex
}

// newResource maps obj to a Kubernetes Resource and constructs a client for that Resource.
// If the object is a list, the resource represents the item's type instead.
func (c *clientCache) newResource(obj runtime.Object) (*resourceMeta, error) {
gvk, err := apiutil.GVKForObject(obj, c.scheme)
if err != nil {
return nil, err
}

if strings.HasSuffix(gvk.Kind, "List") && meta.IsListType(obj) {
func (c *clientCache) newResource(gvk schema.GroupVersionKind, isList bool) (*resourceMeta, error) {
if strings.HasSuffix(gvk.Kind, "List") && isList {
// if this was a list, treat it as a request for the item's resource
gvk.Kind = gvk.Kind[:len(gvk.Kind)-4]
}
Expand All @@ -76,12 +70,15 @@ func (c *clientCache) newResource(obj runtime.Object) (*resourceMeta, error) {
// getResource returns the resource meta information for the given type of object.
// If the object is a list, the resource represents the item's type instead.
func (c *clientCache) getResource(obj runtime.Object) (*resourceMeta, error) {
typ := reflect.TypeOf(obj)
gvk, err := apiutil.GVKForObject(obj, c.scheme)
if err != nil {
return nil, err
}

// It's better to do creation work twice than to not let multiple
// people make requests at once
c.mu.RLock()
r, known := c.resourceByType[typ]
r, known := c.resourceByType[gvk]
c.mu.RUnlock()

if known {
Expand All @@ -91,11 +88,11 @@ func (c *clientCache) getResource(obj runtime.Object) (*resourceMeta, error) {
// Initialize a new Client
c.mu.Lock()
defer c.mu.Unlock()
r, err := c.newResource(obj)
r, err = c.newResource(gvk, meta.IsListType(obj))
if err != nil {
return nil, err
}
c.resourceByType[typ] = r
c.resourceByType[gvk] = r
return r, err
}

Expand Down
51 changes: 51 additions & 0 deletions pkg/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -827,6 +827,57 @@ var _ = Describe("Client", func() {
close(done)
})

It("should update status and preserve type information", func(done Done) {
cl, err := client.New(cfg, client.Options{})
Expect(err).NotTo(HaveOccurred())
Expect(cl).NotTo(BeNil())

By("initially creating a Deployment")
dep, err := clientset.AppsV1().Deployments(ns).Create(dep)
Expect(err).NotTo(HaveOccurred())

By("updating the status of Deployment")
u := &unstructured.Unstructured{}
dep.Status.Replicas = 1
Expect(scheme.Convert(dep, u, nil)).To(Succeed())
err = cl.Status().Update(context.TODO(), u)
Expect(err).NotTo(HaveOccurred())

By("validating updated Deployment has type information")
Expect(u.GroupVersionKind()).To(Equal(depGvk))
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 we should also get the deployment with the generated clientset and verify that its status got updated/patched 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added it for patch, I think it's there for the update calls already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why they're organized this way, but I guess the tests are broken out fairly granular -- one test only does the type validation but the other two validate the actual status server side?

Copy link
Member

Choose a reason for hiding this comment

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

Oh you are right. Indeed very granular. Well, that definitely helps when debugging :)


close(done)
})

It("should patch status and preserve type information", func(done Done) {
cl, err := client.New(cfg, client.Options{})
Expect(err).NotTo(HaveOccurred())
Expect(cl).NotTo(BeNil())

By("initially creating a Deployment")
dep, err := clientset.AppsV1().Deployments(ns).Create(dep)
Expect(err).NotTo(HaveOccurred())

By("patching the status of Deployment")
u := &unstructured.Unstructured{}
depPatch := client.MergeFrom(dep.DeepCopy())
dep.Status.Replicas = 1
Expect(scheme.Convert(dep, u, nil)).To(Succeed())
err = cl.Status().Patch(context.TODO(), u, depPatch)
Expect(err).NotTo(HaveOccurred())

By("validating updated Deployment has type information")
Expect(u.GroupVersionKind()).To(Equal(depGvk))

By("validating patched Deployment has new status")
actual, err := clientset.AppsV1().Deployments(ns).Get(dep.Name, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
Expect(actual).NotTo(BeNil())
Expect(actual.Status.Replicas).To(BeEquivalentTo(1))

close(done)
})

It("should not update spec of an existing object", func(done Done) {
cl, err := client.New(cfg, client.Options{})
Expect(err).NotTo(HaveOccurred())
Expand Down
24 changes: 24 additions & 0 deletions pkg/client/codec.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package client

import (
"errors"
"net/url"

"k8s.io/apimachinery/pkg/conversion/queryparams"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
)

var _ runtime.ParameterCodec = noConversionParamCodec{}

// noConversionParamCodec is a no-conversion codec for serializing parameters into URL query strings.
// it's useful in scenarios with the unstructured client and arbitrary resouces.
type noConversionParamCodec struct{}

func (noConversionParamCodec) EncodeParameters(obj runtime.Object, to schema.GroupVersion) (url.Values, error) {
return queryparams.Convert(obj)
}

func (noConversionParamCodec) DecodeParameters(parameters url.Values, from schema.GroupVersion, into runtime.Object) error {
return errors.New("DecodeParameters not implemented on noConversionParamCodec")
}
2 changes: 1 addition & 1 deletion pkg/client/typed_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
// client is a client.Client that reads and writes directly from/to an API server. It lazily initializes
// new clients at the time they are used, and caches the client.
type typedClient struct {
cache clientCache
cache *clientCache
paramCodec runtime.ParameterCodec
}

Expand Down
Loading