From c9e97de46bb76cf868fcb172f286127b9894fbe9 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Tue, 20 Jul 2021 21:29:02 +0000 Subject: [PATCH] Address PR feedback around gvk parser generation ergonomics --- .../meta/v1/unstructured.go | 75 ++++++++++--------- .../client-go/discovery/discovery_client.go | 14 +--- staging/src/k8s.io/client-go/rest/request.go | 21 +++--- .../integration/client/dynamic_client_test.go | 14 ++-- 4 files changed, 61 insertions(+), 63 deletions(-) diff --git a/staging/src/k8s.io/client-go/applyconfigurations/meta/v1/unstructured.go b/staging/src/k8s.io/client-go/applyconfigurations/meta/v1/unstructured.go index c23aab81244..0a12c4f9995 100644 --- a/staging/src/k8s.io/client-go/applyconfigurations/meta/v1/unstructured.go +++ b/staging/src/k8s.io/client-go/applyconfigurations/meta/v1/unstructured.go @@ -13,11 +13,15 @@ import ( "sigs.k8s.io/structured-merge-diff/v4/typed" ) +// openAPISchemaTTL is how frequently we need to check +// whether the open API schema has changed or not. +const openAPISchemaTTL = time.Minute + // UnstructuredExtractor enables extracting the applied configuration state from object for fieldManager into an // unstructured object type. type UnstructuredExtractor interface { - ExtractUnstructured(object *unstructured.Unstructured, fieldManager string) (*unstructured.Unstructured, error) - ExtractUnstructuredStatus(object *unstructured.Unstructured, fieldManager string) (*unstructured.Unstructured, error) + Extract(object *unstructured.Unstructured, fieldManager string) (*unstructured.Unstructured, error) + ExtractStatus(object *unstructured.Unstructured, fieldManager string) (*unstructured.Unstructured, error) } // gvkParserCache caches the GVKParser in order to prevent from having to repeatedly @@ -37,45 +41,42 @@ type gvkParserCache struct { } // regenerateGVKParser builds the parser from the raw OpenAPI schema. -func (c *gvkParserCache) regenerateGVKParser() error { - doc, err := c.discoveryClient.OpenAPISchema() +func regenerateGVKParser(dc discovery.DiscoveryInterface) (*fieldmanager.GvkParser, error) { + doc, err := dc.OpenAPISchema() if err != nil { - return err + return nil, err } - c.lastChecked = time.Now() + //c.lastChecked = time.Now() models, err := proto.NewOpenAPIData(doc) if err != nil { - return err + return nil, err } - gvkParser, err := fieldmanager.NewGVKParser(models, false) - if err != nil { - return err - } + return fieldmanager.NewGVKParser(models, false) + //gvkParser, err := fieldmanager.NewGVKParser(models, false) + //if err != nil { + // return nil, err + //} - c.gvkParser = gvkParser - return nil + //return gvkParser, nil + //return nil } // objectTypeForGVK retrieves the typed.ParseableType for a given gvk from the cache func (c *gvkParserCache) objectTypeForGVK(gvk schema.GroupVersionKind) (*typed.ParseableType, error) { c.mu.Lock() defer c.mu.Unlock() - if c.gvkParser != nil { - // if the ttl on the parser cache has expired, - // recheck the discovery client to see if the Open API schema has changed - if time.Now().After(c.lastChecked.Add(c.ttl)) { - c.lastChecked = time.Now() - if c.discoveryClient.HasOpenAPISchemaChanged() { - // the schema has changed, regenerate the parser - if err := c.regenerateGVKParser(); err != nil { - return nil, err - } + // if the ttl on the openAPISchema has expired, + // recheck the discovery client to see if the Open API schema has changed + if time.Now().After(c.lastChecked.Add(openAPISchemaTTL)) { + c.lastChecked = time.Now() + if c.discoveryClient.HasOpenAPISchemaChanged() { + // the schema has changed, regenerate the parser + parser, err := regenerateGVKParser(c.discoveryClient) + if err != nil { + return nil, err } - } - } else { - if err := c.regenerateGVKParser(); err != nil { - return nil, err + c.gvkParser = parser } } return c.gvkParser.Type(gvk), nil @@ -87,27 +88,31 @@ type extractor struct { // NewUnstructuredExtractor creates the extractor with which you can extract the applied configuration // for a given manager from an unstructured object. -func NewUnstructuredExtractor(dc discovery.DiscoveryInterface) UnstructuredExtractor { +func NewUnstructuredExtractor(dc discovery.DiscoveryInterface) (UnstructuredExtractor, error) { // TODO: expose ttl as an argument if we want to. - defaultTTL := time.Minute + + parser, err := regenerateGVKParser(dc) + if err != nil { + return nil, err + } return &extractor{ cache: &gvkParserCache{ + gvkParser: parser, discoveryClient: dc, - ttl: defaultTTL, }, - } + }, nil } -// ExtractUnstructured extracts the applied configuration owned by fiieldManager from an unstructured object. +// Extract extracts the applied configuration owned by fiieldManager from an unstructured object. // Note that the apply configuration itself is also an unstructured object. -func (e *extractor) ExtractUnstructured(object *unstructured.Unstructured, fieldManager string) (*unstructured.Unstructured, error) { +func (e *extractor) Extract(object *unstructured.Unstructured, fieldManager string) (*unstructured.Unstructured, error) { return e.extractUnstructured(object, fieldManager, "") } -// ExtractUnstructuredStatus is the same as ExtractUnstructured except +// ExtractStatus is the same as ExtractUnstructured except // that it extracts the status subresource applied configuration. // Experimental! -func (e *extractor) ExtractUnstructuredStatus(object *unstructured.Unstructured, fieldManager string) (*unstructured.Unstructured, error) { +func (e *extractor) ExtractStatus(object *unstructured.Unstructured, fieldManager string) (*unstructured.Unstructured, error) { return e.extractUnstructured(object, fieldManager, "status") } diff --git a/staging/src/k8s.io/client-go/discovery/discovery_client.go b/staging/src/k8s.io/client-go/discovery/discovery_client.go index 5eb16d1d455..1aefdb8649b 100644 --- a/staging/src/k8s.io/client-go/discovery/discovery_client.go +++ b/staging/src/k8s.io/client-go/discovery/discovery_client.go @@ -20,7 +20,6 @@ import ( "context" "encoding/json" "fmt" - "net/http" "net/url" "sort" "strings" @@ -139,7 +138,6 @@ type DiscoveryClient struct { restClient restclient.Interface LegacyPrefix string - etag string } // Convert metav1.APIVersions to metav1.APIGroup. APIVersions is used by legacy v1, so @@ -429,20 +427,12 @@ func (d *DiscoveryClient) ServerVersion() (*version.Info, error) { // HasOpenAPISchemaChanged checks whether a HEAD request to openapi endpoint returns // a 304 StatusNotModified meaning it has not changed. func (d *DiscoveryClient) HasOpenAPISchemaChanged() bool { - result := d.restClient.Verb("HEAD").AbsPath("/openapi/v2").SetHeader("If-None-Match", d.etag).SetHeader("Accept", mimePb).Do(context.TODO()) - var status int - result.StatusCode(&status) - if status == http.StatusNotModified { - return false - } - return true + return !d.restClient.Verb("HEAD").AbsPath("/openapi/v2").SetHeader("Accept", mimePb).Do(context.TODO()).FromCache() } // OpenAPISchema fetches the open api schema using a rest client and parses the proto. func (d *DiscoveryClient) OpenAPISchema() (*openapi_v2.Document, error) { - result := d.restClient.Get().AbsPath("/openapi/v2").SetHeader("Accept", mimePb).Do(context.TODO()) - d.etag = result.Etag() - data, err := result.Raw() + data, err := d.restClient.Get().AbsPath("/openapi/v2").SetHeader("Accept", mimePb).Do(context.TODO()).Raw() if err != nil { if errors.IsForbidden(err) || errors.IsNotFound(err) || errors.IsNotAcceptable(err) { // single endpoint not found/registered in old server, try to fetch old endpoint diff --git a/staging/src/k8s.io/client-go/rest/request.go b/staging/src/k8s.io/client-go/rest/request.go index 55a6df55635..37d1a24e6b6 100644 --- a/staging/src/k8s.io/client-go/rest/request.go +++ b/staging/src/k8s.io/client-go/rest/request.go @@ -1140,12 +1140,12 @@ func (r *Request) transformResponse(resp *http.Response, req *http.Request) Resu } } - // store the etag header so that we can check whether the document - // has changed or not. - var etag string - etagHeader, ok := resp.Header["Etag"] - if ok && len(etagHeader) == 1 { - etag = etagHeader[0] + // store the X-From-Cache header so that we can + // return it as part of the result + var fromCache bool + xFromCacheHeader, ok := resp.Header["X-From-Cache"] + if ok { + fromCache = len(xFromCacheHeader) == 1 && xFromCacheHeader[0] == "1" } return Result{ @@ -1154,7 +1154,7 @@ func (r *Request) transformResponse(resp *http.Response, req *http.Request) Resu statusCode: resp.StatusCode, decoder: decoder, warnings: handleWarnings(resp.Header, r.warningHandler), - etag: etag, + fromCache: fromCache, } } @@ -1281,7 +1281,7 @@ type Result struct { contentType string err error statusCode int - etag string + fromCache bool decoder runtime.Decoder } @@ -1318,8 +1318,9 @@ func (r Result) Get() (runtime.Object, error) { return out, nil } -func (r Result) Etag() string { - return r.etag +// FromCache returns whether the response was returned from the cache. +func (r Result) FromCache() bool { + return r.fromCache } // StatusCode returns the HTTP status code of the request. (Only valid if no diff --git a/test/integration/client/dynamic_client_test.go b/test/integration/client/dynamic_client_test.go index 813000bf3a8..90da8b2ba30 100644 --- a/test/integration/client/dynamic_client_test.go +++ b/test/integration/client/dynamic_client_test.go @@ -265,12 +265,15 @@ func TestUnstructuredExtract(t *testing.T) { t.Fatalf("unexpected pod in list. wanted %#v, got %#v", actual, got) } - // extract the object using ExtractUnstructured + // extract the object discoveryClient := discovery.NewDiscoveryClientForConfigOrDie(result.ClientConfig) - extractor := metav1ac.NewUnstructuredExtractor(discoveryClient) - extracted, err := extractor.ExtractUnstructured(got, mgr) + extractor, err := metav1ac.NewUnstructuredExtractor(discoveryClient) if err != nil { - t.Fatalf("unexpected error when extracting") + t.Fatalf("unexpected error when constructing extrator: %v", err) + } + extracted, err := extractor.Extract(got, mgr) + if err != nil { + t.Fatalf("unexpected error when extracting: %v", err) } // modify the object and apply the modified object @@ -300,10 +303,9 @@ func TestUnstructuredExtract(t *testing.T) { if !reflect.DeepEqual(actualModified, gotModified) { t.Fatalf("unexpected pod in list. wanted %#v, got %#v", actualModified, gotModified) } - fmt.Printf("gotModified = %+v\n", gotModified) // extract again to test hitting the object type cache - extracted2, err := extractor.ExtractUnstructured(gotModified, mgr) + extracted2, err := extractor.Extract(gotModified, mgr) if err != nil { t.Fatalf("unexpected error when extracting for the second time") }