Address PR feedback around gvk parser generation ergonomics

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)
+	Extract(object *unstructured.Unstructured, fieldManager string) (*unstructured.Unstructured, error)
-	ExtractUnstructuredStatus(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 {
+func regenerateGVKParser(dc discovery.DiscoveryInterface) (*fieldmanager.GvkParser, error) {
-	doc, err := c.discoveryClient.OpenAPISchema()
+	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)
+	return fieldmanager.NewGVKParser(models, false)
-	if err != nil {
+	//gvkParser, err := fieldmanager.NewGVKParser(models, false)
-		return err
+	//if err != nil {
-	}
+	//	return nil, err
+	//}
 
-	c.gvkParser = gvkParser
+	//return gvkParser, nil
-	return 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 openAPISchema has expired,
-		// if the ttl on the parser cache has expired,
+	// recheck the discovery client to see if the Open API schema has changed
-		// recheck the discovery client to see if the Open API schema has changed
+	if time.Now().After(c.lastChecked.Add(openAPISchemaTTL)) {
-		if time.Now().After(c.lastChecked.Add(c.ttl)) {
+		c.lastChecked = time.Now()
-			c.lastChecked = time.Now()
+		if c.discoveryClient.HasOpenAPISchemaChanged() {
-			if c.discoveryClient.HasOpenAPISchemaChanged() {
+			// the schema has changed, regenerate the parser
-				// the schema has changed, regenerate the parser
+			parser, err := regenerateGVKParser(c.discoveryClient)
-				if err := c.regenerateGVKParser(); err != nil {
+			if err != nil {
-					return nil, err
+				return nil, err
-				}
 			}
-		}
+			c.gvkParser = parser
-	} else {
-		if err := c.regenerateGVKParser(); err != nil {
-			return nil, err
 		}
 	}
 	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")
 }
 

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())
+	return !d.restClient.Verb("HEAD").AbsPath("/openapi/v2").SetHeader("Accept", mimePb).Do(context.TODO()).FromCache()
-	var status int
-	result.StatusCode(&status)
-	if status == http.StatusNotModified {
-		return false
-	}
-	return true
 }
 
 // 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())
+	data, err := d.restClient.Get().AbsPath("/openapi/v2").SetHeader("Accept", mimePb).Do(context.TODO()).Raw()
-	d.etag = result.Etag()
-	data, err := result.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

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
+	// store the X-From-Cache header so that we can
-	// has changed or not.
+	// return it as part of the result
-	var etag string
+	var fromCache bool
-	etagHeader, ok := resp.Header["Etag"]
+	xFromCacheHeader, ok := resp.Header["X-From-Cache"]
-	if ok && len(etagHeader) == 1 {
+	if ok {
-		etag = etagHeader[0]
+		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 {
+// FromCache returns whether the response was returned from the cache.
-	return r.etag
+func (r Result) FromCache() bool {
+	return r.fromCache
 }
 
 // StatusCode returns the HTTP status code of the request. (Only valid if no

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)
+	extractor, err := metav1ac.NewUnstructuredExtractor(discoveryClient)
-	extracted, err := extractor.ExtractUnstructured(got, mgr)
 	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")
 	}