Skip to content

✨ Rebase kcp modifications onto v0.16.2 #42

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 1 commit into from
Oct 12, 2023
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
13 changes: 13 additions & 0 deletions .prow.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
presubmits:
- name: pull-controller-runtime-everything
always_run: true
decorate: true
clone_uri: "ssh://git@github.com/kcp-dev/controller-runtime.git"
labels:
preset-goproxy: "true"
spec:
containers:
- image: ghcr.io/kcp-dev/infra/build:1.20.9-1
command:
- make
- test
8 changes: 8 additions & 0 deletions DOWNSTREAM_OWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
approvers:
- davidfestal
- ncdc
- stevekuznetsov
- sttts
Copy link
Member

Choose a reason for hiding this comment

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

We should change this, follow-up or here.

reviewers:
- fabianvf
- varshaprasad96
Empty file added DOWNSTREAM_OWNERS_ALIASES
Empty file.
9 changes: 5 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,11 @@ $(GO_APIDIFF): $(TOOLS_DIR)/go.mod # Build go-apidiff from tools folder.
$(CONTROLLER_GEN): $(TOOLS_DIR)/go.mod # Build controller-gen from tools folder.
cd $(TOOLS_DIR) && go build -tags=tools -o bin/controller-gen sigs.k8s.io/controller-tools/cmd/controller-gen

$(GOLANGCI_LINT): .github/workflows/golangci-lint.yml # Download golanci-lint using hack script into tools folder.
hack/ensure-golangci-lint.sh \
-b $(TOOLS_BIN_DIR) \
$(shell cat .github/workflows/golangci-lint.yml | grep "version: v" | sed 's/.*version: //')
$(GOLANGCI_LINT): .github/workflows/golangci-lint.yml # Download golangci-lint using hack script into tools folder.
GOBIN=$(abspath $(TOOLS_BIN_DIR)) go install github.com/golangci/golangci-lint/cmd/golangci-lint@$(shell cat .github/workflows/golangci-lint.yml | grep "version: v" | sed 's/.*version: //')

.PHONY: tools
tools: $(GO_APIDIFF) $(CONTROLLER_GEN) $(GOLANGCI_LINT)
Copy link
Member

Choose a reason for hiding this comment

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

why are we diverting here from upstream?

Copy link
Author

Choose a reason for hiding this comment

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

I carried these changes over from the kcp-1.26 branch, where the commits just said

UPSTREAM: : make: use go install for lint
UPSTREAM: : make: add tools target

If you want I can remove them and see if something breaks, as I am also not sure why exactly we added these.

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinion, was just curious.


## --------------------------------------
## Linting
Expand Down
4 changes: 4 additions & 0 deletions examples/scratch-env/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -936,6 +936,10 @@ github.com/julienschmidt/httprouter v1.3.0/go.mod h1:JR6WtHb+2LUe8TCKY3cZOxFyyO8
github.com/jung-kurt/gofpdf v1.0.0/go.mod h1:7Id9E/uU8ce6rXgefFLlgrJj/GYY22cpxn+r32jIOes=
github.com/jung-kurt/gofpdf v1.0.3-0.20190309125859-24315acbbda5/go.mod h1:7Id9E/uU8ce6rXgefFLlgrJj/GYY22cpxn+r32jIOes=
github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8=
github.com/kcp-dev/apimachinery/v2 v2.0.0-alpha.0.0.20230926071920-57d168bcbe34 h1:tom0JX5OmAeOOmkGv8LaYHDtA1xAKDiQL5U0vhYYgdM=
github.com/kcp-dev/apimachinery/v2 v2.0.0-alpha.0.0.20230926071920-57d168bcbe34/go.mod h1:cWoaYGHl1nlzdEM2xvMzIASkEZJZLSf5nhe17M7wDhw=
github.com/kcp-dev/logicalcluster/v3 v3.0.4 h1:q7KngML/QM7sWl8aVzmfZF0TPMnBwYNxsPKfwUvvBvU=
github.com/kcp-dev/logicalcluster/v3 v3.0.4/go.mod h1:EWBUBxdr49fUB1cLMO4nOdBWmYifLbP1LfoL20KkXYY=
github.com/kisielk/errcheck v1.1.0/go.mod h1:EZBBE59ingxPouuu3KfxchcWSUPOHkagtvWXihfKN4Q=
github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8=
github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ require (
github.com/go-logr/zapr v1.2.4
github.com/google/go-cmp v0.5.9
github.com/google/gofuzz v1.2.0
github.com/kcp-dev/apimachinery/v2 v2.0.0-alpha.0.0.20230926071920-57d168bcbe34
github.com/kcp-dev/logicalcluster/v3 v3.0.4
github.com/onsi/ginkgo/v2 v2.11.0
github.com/onsi/gomega v1.27.10
github.com/prometheus/client_golang v1.16.0
Expand Down
4 changes: 4 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,10 @@ github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnr
github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo=
github.com/jstemmer/go-junit-report v0.0.0-20190106144839-af01ea7f8024/go.mod h1:6v2b51hI/fHJwM22ozAgKL4VKDeJcHhJFhtBdhmNjmU=
github.com/jstemmer/go-junit-report v0.9.1/go.mod h1:Brl9GWCQeLvo8nXZwPNNblvFj/XSXhF0NWZEnDohbsk=
github.com/kcp-dev/apimachinery/v2 v2.0.0-alpha.0.0.20230926071920-57d168bcbe34 h1:tom0JX5OmAeOOmkGv8LaYHDtA1xAKDiQL5U0vhYYgdM=
github.com/kcp-dev/apimachinery/v2 v2.0.0-alpha.0.0.20230926071920-57d168bcbe34/go.mod h1:cWoaYGHl1nlzdEM2xvMzIASkEZJZLSf5nhe17M7wDhw=
github.com/kcp-dev/logicalcluster/v3 v3.0.4 h1:q7KngML/QM7sWl8aVzmfZF0TPMnBwYNxsPKfwUvvBvU=
github.com/kcp-dev/logicalcluster/v3 v3.0.4/go.mod h1:EWBUBxdr49fUB1cLMO4nOdBWmYifLbP1LfoL20KkXYY=
github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8=
github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
Expand Down
18 changes: 14 additions & 4 deletions pkg/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"net/http"
"time"

"github.com/kcp-dev/apimachinery/v2/third_party/informers"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/fields"
Expand Down Expand Up @@ -159,6 +160,14 @@ type Options struct {
// instead of `reconcile.Result{}`.
SyncPeriod *time.Duration

// NewInformerFunc is a function that is used to create SharedIndexInformers.
// Defaults to cache.NewSharedIndexInformer from client-go
NewInformerFunc client.NewInformerFunc

// Indexers is the indexers that the informers will be configured to use.
// Will always have the standard NamespaceIndex.
Indexers toolscache.Indexers

// ReaderFailOnMissingInformer configures the cache to return a ErrResourceNotCached error when a user
// requests, using Get() and List(), a resource the cache does not already have an informer for.
//
Expand Down Expand Up @@ -201,9 +210,6 @@ type Options struct {
// ByObject restricts the cache's ListWatch to the desired fields per GVK at the specified object.
// object, this will fall through to Default* settings.
ByObject map[client.Object]ByObject

// newInformer allows overriding of NewSharedIndexInformer for testing.
newInformer *func(toolscache.ListerWatcher, runtime.Object, time.Duration, toolscache.Indexers) toolscache.SharedIndexInformer
Copy link
Author

Choose a reason for hiding this comment

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

Not sure super why this is originally a *func, as regular funcs can already be nil. As our previous fork (kcp-1.26) already introduced NewInformerFunc client.NewInformerFunc, I removed this one here and adjusted the related code to not expect a function pointer.

Copy link
Member

Choose a reason for hiding this comment

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

is this different func type than client.NewInformerFunc?

}

// ByObject offers more fine-grained control over the cache's ListWatch by object.
Expand Down Expand Up @@ -354,7 +360,7 @@ func newCache(restConfig *rest.Config, opts Options) newCacheFunc {
},
Transform: config.Transform,
UnsafeDisableDeepCopy: pointer.BoolDeref(config.UnsafeDisableDeepCopy, false),
NewInformer: opts.newInformer,
NewInformer: opts.NewInformerFunc,
}),
readerFailOnMissingInformer: opts.ReaderFailOnMissingInformer,
}
Expand Down Expand Up @@ -438,6 +444,10 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) {
if opts.SyncPeriod == nil {
opts.SyncPeriod = &defaultSyncPeriod
}

if opts.NewInformerFunc == nil {
opts.NewInformerFunc = informers.NewSharedIndexInformer
}
return opts, nil
}

Expand Down
7 changes: 4 additions & 3 deletions pkg/cache/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"strings"
"time"

kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -545,13 +546,13 @@ func NonBlockingGetTest(createCacheFunc func(config *rest.Config, opts cache.Opt

By("creating the informer cache")
v := reflect.ValueOf(&opts).Elem()
newInformerField := v.FieldByName("newInformer")
newFakeInformer := func(_ kcache.ListerWatcher, _ runtime.Object, _ time.Duration, _ kcache.Indexers) kcache.SharedIndexInformer {
newInformerField := v.FieldByName("NewInformerFunc")
newFakeInformer := func(_ kcache.ListerWatcher, _ runtime.Object, _ time.Duration, _ kcache.Indexers) kcpcache.ScopeableSharedIndexInformer {
return &controllertest.FakeInformer{Synced: false}
}
reflect.NewAt(newInformerField.Type(), newInformerField.Addr().UnsafePointer()).
Elem().
Set(reflect.ValueOf(&newFakeInformer))
Set(reflect.ValueOf(newFakeInformer))
informerCache, err = createCacheFunc(cfg, opts)
Expect(err).NotTo(HaveOccurred())
By("running the cache and waiting for it to sync")
Expand Down
14 changes: 12 additions & 2 deletions pkg/cache/informer_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import (
"sigs.k8s.io/controller-runtime/pkg/cache/internal"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"

"github.com/kcp-dev/logicalcluster/v3"
)

var (
Expand Down Expand Up @@ -222,6 +224,13 @@ func indexByField(informer Informer, field string, extractValue client.IndexerFu
}
ns := meta.GetNamespace()

keyFunc := internal.KeyToNamespacedKey
if clusterName := logicalcluster.From(obj); !clusterName.Empty() {
keyFunc = func(ns, val string) string {
return internal.KeyToClusteredKey(clusterName.String(), ns, val)
}
}

rawVals := extractValue(obj)
var vals []string
if ns == "" {
Expand All @@ -231,14 +240,15 @@ func indexByField(informer Informer, field string, extractValue client.IndexerFu
// if we need to add non-namespaced versions too, double the length
vals = make([]string, len(rawVals)*2)
}

for i, rawVal := range rawVals {
// save a namespaced variant, so that we can ask
// "what are all the object matching a given index *in a given namespace*"
vals[i] = internal.KeyToNamespacedKey(ns, rawVal)
vals[i] = keyFunc(ns, rawVal)
if ns != "" {
// if we have a namespace, also inject a special index key for listing
// regardless of the object namespace
vals[i+len(rawVals)] = internal.KeyToNamespacedKey("", rawVal)
vals[i+len(rawVals)] = keyFunc("", rawVal)
}
}

Expand Down
42 changes: 35 additions & 7 deletions pkg/cache/internal/cache_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"fmt"
"reflect"

kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"

apierrors "k8s.io/apimachinery/pkg/api/errors"
apimeta "k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/labels"
Expand All @@ -30,6 +32,7 @@ import (

"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/internal/field/selector"
"sigs.k8s.io/controller-runtime/pkg/kontext"
)

// CacheReader is a client.Reader.
Expand All @@ -53,11 +56,11 @@ 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, _ ...client.GetOption) error {
func (c *CacheReader) Get(ctx context.Context, key client.ObjectKey, out client.Object, _ ...client.GetOption) error {
if c.scopeName == apimeta.RESTScopeNameRoot {
key.Namespace = ""
}
storeKey := objectKeyToStoreKey(key)
storeKey := objectKeyToStoreKey(ctx, key)

// Lookup the object from the indexer cache
obj, exists, err := c.indexer.GetByKey(storeKey)
Expand Down Expand Up @@ -104,7 +107,7 @@ func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Ob
}

// List lists items out of the indexer and writes them to out.
func (c *CacheReader) List(_ context.Context, out client.ObjectList, opts ...client.ListOption) error {
func (c *CacheReader) List(ctx context.Context, out client.ObjectList, opts ...client.ListOption) error {
var objs []interface{}
var err error

Expand All @@ -115,6 +118,8 @@ func (c *CacheReader) List(_ context.Context, out client.ObjectList, opts ...cli
return fmt.Errorf("continue list option is not supported by the cache")
}

clusterName, _ := kontext.ClusterFrom(ctx)

switch {
case listOpts.FieldSelector != nil:
// TODO(directxman12): support more complicated field selectors by
Expand All @@ -126,11 +131,23 @@ func (c *CacheReader) List(_ context.Context, out client.ObjectList, opts ...cli
// list all objects by the field selector. If this is namespaced and we have one, ask for the
// namespaced index key. Otherwise, ask for the non-namespaced variant by using the fake "all namespaces"
// namespace.
objs, err = c.indexer.ByIndex(FieldIndexName(field), KeyToNamespacedKey(listOpts.Namespace, val))
if clusterName.Empty() {
objs, err = c.indexer.ByIndex(FieldIndexName(field), KeyToNamespacedKey(listOpts.Namespace, val))
} else {
objs, err = c.indexer.ByIndex(FieldIndexName(field), KeyToClusteredKey(clusterName.String(), listOpts.Namespace, val))
}
case listOpts.Namespace != "":
objs, err = c.indexer.ByIndex(cache.NamespaceIndex, listOpts.Namespace)
if clusterName.Empty() {
objs, err = c.indexer.ByIndex(cache.NamespaceIndex, listOpts.Namespace)
} else {
objs, err = c.indexer.ByIndex(kcpcache.ClusterAndNamespaceIndexName, kcpcache.ClusterAndNamespaceIndexKey(clusterName, listOpts.Namespace))
}
default:
objs = c.indexer.List()
if clusterName.Empty() {
objs = c.indexer.List()
} else {
objs, err = c.indexer.ByIndex(kcpcache.ClusterIndexName, kcpcache.ClusterIndexKey(clusterName))
}
}
if err != nil {
return err
Expand Down Expand Up @@ -182,7 +199,12 @@ func (c *CacheReader) List(_ context.Context, out client.ObjectList, opts ...cli
// It's akin to MetaNamespaceKeyFunc. It's separate from
// String to allow keeping the key format easily in sync with
// MetaNamespaceKeyFunc.
func objectKeyToStoreKey(k client.ObjectKey) string {
func objectKeyToStoreKey(ctx context.Context, k client.ObjectKey) string {
cluster, ok := kontext.ClusterFrom(ctx)
if ok {
return kcpcache.ToClusterAwareKey(cluster.String(), k.Namespace, k.Name)
}

if k.Namespace == "" {
return k.Name
}
Expand All @@ -206,3 +228,9 @@ func KeyToNamespacedKey(ns string, baseKey string) string {
}
return allNamespacesNamespace + "/" + baseKey
}

// KeyToClusteredKey prefixes the given index key with a cluster name
// for use in field selector indexes.
func KeyToClusteredKey(clusterName string, ns string, baseKey string) string {
return clusterName + "/" + KeyToNamespacedKey(ns, baseKey)
}
10 changes: 6 additions & 4 deletions pkg/cache/internal/informers.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (
"sync"
"time"

kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"
"github.com/kcp-dev/apimachinery/v2/third_party/informers"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -45,17 +47,17 @@ type InformersOpts struct {
Mapper meta.RESTMapper
ResyncPeriod time.Duration
Namespace string
NewInformer *func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) cache.SharedIndexInformer
NewInformer func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) kcpcache.ScopeableSharedIndexInformer
Selector Selector
Transform cache.TransformFunc
UnsafeDisableDeepCopy bool
}

// NewInformers creates a new InformersMap that can create informers under the hood.
func NewInformers(config *rest.Config, options *InformersOpts) *Informers {
newInformer := cache.NewSharedIndexInformer
newInformer := informers.NewSharedIndexInformer
if options.NewInformer != nil {
newInformer = *options.NewInformer
newInformer = options.NewInformer
}
return &Informers{
config: config,
Expand Down Expand Up @@ -158,7 +160,7 @@ type Informers struct {
unsafeDisableDeepCopy bool

// NewInformer allows overriding of the shared index informer constructor for testing.
newInformer func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) cache.SharedIndexInformer
newInformer func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) kcpcache.ScopeableSharedIndexInformer
}

// Start calls Run on each of the informers and sets started to true. Blocks on the context.
Expand Down
8 changes: 8 additions & 0 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,14 @@ type CacheOptions struct {
// NewClientFunc allows a user to define how to create a client.
type NewClientFunc func(config *rest.Config, options Options) (Client, error)

// NewAPIReaderFunc allows a user to define how to create an API server reader.
type NewAPIReaderFunc func(config *rest.Config, options Options) (Reader, error)

// NewAPIReader creates a new API server reader.
func NewAPIReader(config *rest.Config, options Options) (Reader, error) {
return New(config, options)
}

// New returns a new Client using the provided config and Options.
// The returned client reads *and* writes directly from the server
// (it doesn't use object caches). It understands how to work with
Expand Down
7 changes: 7 additions & 0 deletions pkg/client/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ package client

import (
"context"
"time"

kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/tools/cache"

"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -44,6 +47,10 @@ type Patch interface {
Data(obj Object) ([]byte, error)
}

// NewInformerFunc describes a function that creates SharedIndexInformers.
// Its signature matches cache.NewSharedIndexInformer from client-go.
type NewInformerFunc func(cache.ListerWatcher, runtime.Object, time.Duration, cache.Indexers) kcpcache.ScopeableSharedIndexInformer

// TODO(directxman12): is there a sane way to deal with get/delete options?

// Reader knows how to read and list Kubernetes objects.
Expand Down
11 changes: 10 additions & 1 deletion pkg/cluster/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ type Options struct {
// By default, the client will use the cache for reads and direct calls for writes.
Client client.Options

// NewAPIReaderFunc is the function that creates the APIReader client to be
// used by the manager. If not set this will use the default new APIReader
// function.
NewAPIReader client.NewAPIReaderFunc

// NewClient is the func that creates the client to be used by the manager.
// If not set this will create a Client backed by a Cache for read operations
// and a direct Client for write operations.
Expand Down Expand Up @@ -230,7 +235,7 @@ func New(config *rest.Config, opts ...Option) (Cluster, error) {
}

// Create the API Reader, a client with no cache.
clientReader, err := client.New(config, client.Options{
clientReader, err := options.NewAPIReader(config, client.Options{
HTTPClient: options.HTTPClient,
Scheme: options.Scheme,
Mapper: mapper,
Expand Down Expand Up @@ -280,6 +285,10 @@ func setOptionsDefaults(options Options, config *rest.Config) (Options, error) {
options.MapperProvider = apiutil.NewDynamicRESTMapper
}

if options.NewAPIReader == nil {
options.NewAPIReader = client.NewAPIReader
}

// Allow users to define how to create a new client
if options.NewClient == nil {
options.NewClient = client.New
Expand Down
Loading