Kcp hacking #1

fabianvf · 2022-02-02T20:18:55Z

No description provided.

Clusterscope client

????

Add cluster scoped caches

pass clustername in context

Looks like all we need to do is modify the config to look at /clusters/*, that makes the controller watch events across admin and logical clusters.

Co-authored-by: Nick Hale <njohnhale@gmail.com>

Create minimal clusterscoped cache

update to latest kcp changes

ncdc · 2022-05-19T17:02:25Z

pkg/cache/cache.go

@@ -112,6 +112,9 @@ type Options struct {
 	// Default watches all namespaces
 	Namespace string

+	// ClusterName restricts cache's ListeWatch to the desired clusterscope
+	ClusterName string


ncdc · 2022-05-19T17:04:39Z

pkg/cache/clusterscoped_minimal_cache.go

+
+import "k8s.io/client-go/rest"
+
+func MultiClusterMinCache() NewCacheFunc {


Can either remove and require user to pass in a rest config that is * scoped, or move to kcp-dev/apimachinery

ncdc · 2022-05-19T17:06:26Z

pkg/client/client.go

@@ -60,6 +60,9 @@ type Options struct {
 	// Opts is used to configure the warning handler responsible for
 	// surfacing and handling warnings messages sent by the API server.
 	Opts WarningHandlerOptions
+
+	// Cluster refers to the name of the cluster this request is scoped to
+	Cluster string


Can probably remove this

ncdc · 2022-05-19T17:06:32Z

pkg/client/options.go

@@ -330,6 +330,9 @@ type ListOptions struct {
 	// non-namespaced objects, or to list across all namespaces.
 	Namespace string

+	// ClusterName refers to cluster from which to list objects
+	ClusterName string


ncdc · 2022-05-19T17:13:57Z

pkg/client/typed_client.go

 	createOpts := &CreateOptions{}
 	createOpts.ApplyOptions(opts)
 	return o.Post().
+		Cluster(clusterName).


Instead of doing all of this to inject the cluster name here, see if instead we can supply a custom *http.Client to some Options struct, and in pkg/client/apiutil/apimachinery.go near RESTClientForGVK, add a new method that is RESTClientForGVKAndClient. That way we can plumb through the kcp custom round tripper.

ncdc · 2022-05-19T17:18:01Z

pkg/client/unstructured_client.go

 	createOpts := &CreateOptions{}
 	createOpts.ApplyOptions(opts)
 	result := o.Post().
+		Cluster(clusterName).


Same as typed_client

ncdc · 2022-05-19T17:18:20Z

pkg/client/watch.go

@@ -33,10 +33,14 @@ func NewWithWatch(config *rest.Config, options Options) (WithWatch, error) {
 	if err != nil {
 		return nil, err
 	}
-	dynamicClient, err := dynamic.NewForConfig(config)
+


ncdc · 2022-05-19T17:20:00Z

pkg/client/watch.go

 	if err != nil {
 		return nil, err
 	}
+
 	return &watchingClient{client: client, dynamic: dynamicClient}, nil


Need to see what, if any, adjustments need to be made to work with * or cluster-specific watches.

ncdc · 2022-05-19T17:21:49Z

pkg/handler/enqueue.go

@@ -41,25 +42,16 @@ func (e *EnqueueRequestForObject) Create(evt event.CreateEvent, q workqueue.Rate
 		enqueueLog.Error(nil, "CreateEvent received with no metadata", "event", evt)
 		return
 	}
-	q.Add(reconcile.Request{NamespacedName: types.NamespacedName{


Maybe write our own cluster-aware handlers in a separate kcp repo

ncdc · 2022-05-19T17:45:48Z

Additional ideas from our chat:

Create a separate kcp-controller-runtime repo (not a fork) that has a ClusterManager, ClusterClient, ClusterCacheReader, and copies of all the enqueuing handlers
Need to modify client.ObjectKey and/or types.NamespacedName (in kube apimachinery) to add ClusterName (maybe logicalcluster.Name, maybe string)

ncdc · 2022-05-19T17:50:04Z

Long term, if we want controller-runtime to work with kcp w/o any go.mod replace directives, controller-runtime will need

to account for clusterName in client.ObjectKey in some way. Could be by changing it to an extensible interface, or switching away from NamespacedName to a controller-runtime struct/interface.
to modify all code that uses an indexer directly to make sure it supports customization. Although if we write our own ClusterCacheReader, this could be kept outside (but it does mean we need to keep the impls in sync).
informer_cache.go does some indexing by field. Need to make sure that supports clusters, or raise it up to our separate kcp-controller-runtime repo maybe

fabianvf · 2022-05-23T19:15:57Z

Edit: Ignore literally everything I say here

Extending ObjectKey in a non-breaking way is proving to be pretty difficult due to the way embedded structs work, currently the usage/behavior is:

type ObjectKey = types.NamespaceName

ObjectKey{NamespacedName: types.NamespacedName{Name: "name", Namespace: "namespace"}

ObjectKey.Name  == "name"
ObjectKey.Namespace == "namespace"

If I try to extend it in any way, I don't think it's possible to replicate that behavior. Ex:

type Object struct {
  types.NamespaceName
  AdditionalIdentifer string
}

ObjectKey{NamespacedName: types.NamespacedName{Name: "name", Namespace: "namespace"}

ObjectKey.Name  == "name" // cannot use promoted field Name
ObjectKey.Namespace == "namespace" // cannot use promoted field Namespace

type Object struct {
  Name string
  Namespace string

  AdditionalIdentifer string
}

// Invalid construction, no NamespacedName field
ObjectKey{NamespacedName: types.NamespacedName{Name: "name", Namespace: "namespace"}

type Object struct {
  NamespacedName: types.NamespacedName

  AdditionalIdentifer string
}

ObjectKey{NamespacedName: types.NamespacedName{Name: "name", Namespace: "namespace"}

ObjectKey.Name  // doesn't exist
ObjectKey.Namespace // doesn't exist

type Object struct {
  NamespacedName: types.NamespacedName
  Name string
  Namespace string

  AdditionalIdentifer string
}

ObjectKey{NamespacedName: types.NamespacedName{Name: "name", Namespace: "namespace"}

ObjectKey.Name  // empty
ObjectKey.Namespace // empty

Obviously incredibly easy to work around if we don't care about breaking changes but might make it hard to move this upstream as that's a pretty far-reaching change

fabianvf · 2022-05-23T20:17:20Z

examples/kcp/main.go

+	ctrl.SetLogger(zap.New())
+
+	cfg := ctrl.GetConfigOrDie()
+	httpClient, err := rest.HTTPClientFor(cfg)


should probably make a helper for this, like kcpclient.NewClientForConfig(cfg) that gives you an http.Client with the transport set.

ncdc · 2022-05-24T18:13:00Z

examples/kcp/main.go

+	ctrl.SetLogger(zap.New())
+
+	cfg := ctrl.GetConfigOrDie()
+	httpClient, err := rest.HTTPClientFor(cfg)


ncdc · 2022-05-24T18:14:12Z

pkg/cache/cache.go

+		httpclient, err := rest.HTTPClientFor(config)
+		if err != nil {
+			log.WithName("setup").Error(err, "Failed to create HTTP client")
+			return opts, fmt.Errorf("Could not create HTTPClient from config")


Suggested change

return opts, fmt.Errorf("Could not create HTTPClient from config")

return opts, fmt.Errorf("could not create HTTPClient from config: %w", err)

ncdc · 2022-05-24T18:14:30Z

pkg/cache/cache.go

+	if opts.HTTPClient == nil {
+		httpclient, err := rest.HTTPClientFor(config)
+		if err != nil {
+			log.WithName("setup").Error(err, "Failed to create HTTP client")


Not sure if we need to both log and return an error?

ncdc · 2022-05-24T18:14:38Z

pkg/cache/cache_test.go

@@ -26,7 +26,6 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
-


ncdc · 2022-05-24T18:14:48Z

pkg/cache/internal/cache_reader.go

@@ -54,7 +54,7 @@ type CacheReader struct {
 }

 // Get checks the indexer for the object and writes a copy of it if found.
-func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Object) error {
+func (c *CacheReader) Get(ctx context.Context, key client.ObjectKey, out client.Object) error {


ncdc · 2022-05-24T18:17:20Z

pkg/cache/internal/informers_map.go

@@ -54,9 +55,12 @@ func newSpecificInformersMap(config *rest.Config,
 	namespace string,
 	selectors SelectorsByGVK,
 	disableDeepCopy DisableDeepCopyByGVK,
-	createListWatcher createListWatcherFunc) *specificInformersMap {
+	createListWatcher createListWatcherFunc,
+	httpclient *http.Client) *specificInformersMap {


Suggested change

httpclient *http.Client) *specificInformersMap {

httpClient *http.Client) *specificInformersMap {

ncdc · 2022-05-24T18:17:47Z

pkg/client/apiutil/apimachinery.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2018 The Kubernetes Authors.
+opyright 2018 The Kubernetes Authors.


ncdc · 2022-05-24T18:18:24Z

pkg/client/client_cache.go

@@ -35,6 +36,9 @@ type clientCache struct {
 	// config is the rest.Config to talk to an apiserver
 	config *rest.Config

+	// httpclient is the httpClient to talk to an apiserver
+	httpclient *http.Client


Suggested change

httpclient *http.Client

httpClient *http.Client

ncdc · 2022-05-24T18:18:35Z

pkg/client/client_cache.go

@@ -35,6 +36,9 @@ type clientCache struct {
 	// config is the rest.Config to talk to an apiserver
 	config *rest.Config

+	// httpclient is the httpClient to talk to an apiserver


Suggested change

// httpclient is the httpClient to talk to an apiserver

// httpClient is the http.Client to talk to an apiserver

ncdc · 2022-05-24T18:20:00Z

pkg/kcp/enqueue.go

@@ -0,0 +1,85 @@
+/*
+Copyright 2018 The Kubernetes Authors.


Change copyright to kcp. Do you want to keep this in controller-runtime for now, or move to a kcp-owned repo?

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

more kcp plumbing

Controller-runtime allows building kcp-aware controllers

joelanford and others added 3 commits January 19, 2022 14:13

update NewWithWatch to support dynamic client scope

7686849

remove g

82579ed

add kcp example

ec25409

fabianvf force-pushed the kcp-hacking branch from eeebd81 to ec25409 Compare February 4, 2022 19:35

varshaprasad96 and others added 26 commits February 6, 2022 11:25

test

02e83ef

testing controller-runtime scope changes

f3fd6ef

add clusterScope and test clients

9aaf63b

modify gomod to point to feature-logical-branch-1.22

68aca41

add clusterName to key

6ff6cd9

Merge pull request #2 from varshaprasad96/clusterscope-client

33fcbc7

Clusterscope client

First stab at getting watch to use cluster name

f88559a

hacked with informer and caches

94146df

modify cache to accept clusterName

91bb3df

fix errors - clusterCaching works

19cf5db

????

2425666

wip updating example

dbefa63

remove clustername from cachekey

9fd8151

i guess it works?

27955fb

example working

1cae937

Merge pull request #1 from fabianvf/cluster/cache

6aab6ba

????

clean - working cluster scope changes

db6c528

Merge pull request #4 from varshaprasad96/cluster/cache

4a14c9c

Add cluster scoped caches

pass clustername in context

abbefd8

I think this worksish

5d0a60e

Merge pull request #5 from fabianvf/context-cluster

98bf814

pass clustername in context

Create minimal clusterscoped cache

902b80f

Looks like all we need to do is modify the config to look at /clusters/*, that makes the controller watch events across admin and logical clusters.

Update examples/kcp/main.go

5ff5897

Co-authored-by: Nick Hale <njohnhale@gmail.com>

Merge pull request #6 from varshaprasad96/fix-watch-admin-cluster

f3ffab7

Create minimal clusterscoped cache

wip update to latest kcp changes

9656343

updated

8ea115b

Merge pull request #7 from varshaprasad96/update-kcp

076653a

update to latest kcp changes

ncdc reviewed May 19, 2022

View reviewed changes

fabianvf force-pushed the kcp-hacking branch from e3e09f0 to 27af36d Compare May 23, 2022 18:19

Undo unwanted changes

660d16a

fabianvf force-pushed the kcp-hacking branch from 27af36d to 660d16a Compare May 23, 2022 18:23

update objectkey to include clustername

93f1f43

fabianvf force-pushed the kcp-hacking branch from 6eab100 to 93f1f43 Compare May 23, 2022 19:52

fabianvf added 2 commits May 23, 2022 15:59

Plumb httpclient through cache options

29c24dc

Example now uses kcp-aware transport

81b572d

fabianvf commented May 23, 2022

View reviewed changes

add kcp enqueue

b2eebc2

ncdc reviewed May 24, 2022

View reviewed changes

fabianvf force-pushed the kcp-hacking branch from c3b3734 to b2eebc2 Compare May 25, 2022 16:04

fabianvf and others added 3 commits May 25, 2022 13:18

Plumb keyfunc through

e587482

more kcp plumbing

2c01bd9

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

Merge pull request #8 from ncdc/andy-kcp-hacking

99fc178

more kcp plumbing

fabianvf pushed a commit that referenced this pull request May 25, 2022

Merge pull request #1 from fabianvf/make-kcp-aware

3f3188c

Controller-runtime allows building kcp-aware controllers

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kcp hacking #1

Kcp hacking #1

fabianvf commented Feb 2, 2022

ncdc May 19, 2022

ncdc May 19, 2022

ncdc May 19, 2022

ncdc May 19, 2022

ncdc May 19, 2022

ncdc May 19, 2022

ncdc May 19, 2022

ncdc May 19, 2022

ncdc May 19, 2022

ncdc commented May 19, 2022

ncdc commented May 19, 2022

fabianvf commented May 23, 2022 •

edited

fabianvf May 23, 2022

ncdc May 24, 2022

ncdc May 24, 2022

ncdc May 24, 2022

ncdc May 24, 2022

ncdc May 24, 2022

ncdc May 24, 2022

ncdc May 24, 2022

ncdc May 24, 2022

ncdc May 24, 2022

ncdc May 24, 2022

ncdc May 24, 2022


		import "k8s.io/client-go/rest"

		func MultiClusterMinCache() NewCacheFunc {

	return opts, fmt.Errorf("Could not create HTTPClient from config")
	return opts, fmt.Errorf("could not create HTTPClient from config: %w", err)

	httpclient http.Client) specificInformersMap {
	httpClient http.Client) specificInformersMap {

	// httpclient is the httpClient to talk to an apiserver
	// httpClient is the http.Client to talk to an apiserver

Kcp hacking #1

Are you sure you want to change the base?

Kcp hacking #1

Conversation

fabianvf commented Feb 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented May 19, 2022

ncdc commented May 19, 2022

fabianvf commented May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianvf commented May 23, 2022 •

edited