Skip to content
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

✨ WIP: Create a Go pkg with a managed content caching layer #1001

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
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
143 changes: 143 additions & 0 deletions internal/contentmanager/contentmanager.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
package contentmanager

import (
"context"
"errors"
"fmt"

"github.com/operator-framework/operator-controller/api/v1alpha1"

Check failure on line 8 in internal/contentmanager/contentmanager.go

View workflow job for this annotation

GitHub Actions / lint

File is not `gci`-ed with --skip-generated -s standard -s dot -s default -s prefix(github.com/operator-framework) -s prefix(github.com/operator-framework/operator-controller) --custom-order (gci)
oclabels "github.com/operator-framework/operator-controller/internal/labels"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/source"

Check failure on line 20 in internal/contentmanager/contentmanager.go

View workflow job for this annotation

GitHub Actions / lint

File is not `gci`-ed with --skip-generated -s standard -s dot -s default -s prefix(github.com/operator-framework) -s prefix(github.com/operator-framework/operator-controller) --custom-order (gci)
)

type ContentManager interface {
// ManageContent will:
// 1. Create a new controller-runtime cache.Cache belonging to the provided ClusterExtension
// 2. For each object provided:
// A. Use the provided controller.Controller to establish a watch for the resource
ManageContent(context.Context, controller.Controller, *v1alpha1.ClusterExtension, []client.Object) error
// RemoveManagedContent will:
// 1. Remove/stop cache and any sources/informers for the provided ClusterExtension
RemoveManagedContent(*v1alpha1.ClusterExtension) error
}

type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error)

type extensionCacheData struct {
Cache cache.Cache
Cancel context.CancelFunc
}

type instance struct {
rcm RestConfigMapper
baseCfg *rest.Config
extensionCaches map[string]extensionCacheData
mapper meta.RESTMapper
}

func New(rcm RestConfigMapper, cfg *rest.Config, mapper meta.RESTMapper) ContentManager {
return &instance{
rcm: rcm,
baseCfg: cfg,
extensionCaches: make(map[string]extensionCacheData),
mapper: mapper,
}
}

func (i *instance) ManageContent(ctx context.Context, ctrl controller.Controller, ce *v1alpha1.ClusterExtension, objs []client.Object) error {
cfg, err := i.rcm(ctx, ce, i.baseCfg)
if err != nil {
return fmt.Errorf("getting rest.Config for ClusterExtension %q: %w", ce.Name, err)
}

// TODO: add a http.RoundTripper to the config to ensure it is always using an up
// to date authentication token for the ServiceAccount token provided in the ClusterExtension.
// Maybe this should be handled by the RestConfigMapper
Comment on lines +63 to +65
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are thinking this should indeed be done as part of the RestConfigMapper implementation so that everything utilizing it can take advantage of automated token rotation


// Assumptions: all objects received by the function will have the Object metadata specfically,
// ApiVersion and Kind set. Failure to which the code will panic when adding the types to the scheme
scheme := runtime.NewScheme()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I was playing around with integrating this logic into the controller, I ran into an issue that it couldn't properly configure a handler for the ClusterExtension resource as an owner because the API didn't exist in the scheme. I had to add the following code:

err = v1alpha1.AddToScheme(scheme)
if err != nil {
  return fmt.Errorf("adding ClusterExtension APIs to scheme: %w", err)
}

for _, obj := range objs {
gvk := obj.GetObjectKind().GroupVersionKind()
listKind := obj.GetObjectKind().GroupVersionKind().Kind + "List"

if gvk.Kind == "" || gvk.Version == "" {
return errors.New("object Kind or Version is not defined")
}

if !scheme.Recognizes(gvk) {
u := &unstructured.Unstructured{}
u.SetGroupVersionKind(gvk)
ul := &unstructured.UnstructuredList{}

ul.SetGroupVersionKind(gvk.GroupVersion().WithKind(listKind))

scheme.AddKnownTypeWithName(gvk, u)
scheme.AddKnownTypeWithName(gvk.GroupVersion().WithKind(listKind), ul)
metav1.AddToGroupVersion(scheme, gvk.GroupVersion())
}
}

tgtLabels := labels.Set{
oclabels.OwnerKindKey: v1alpha1.ClusterExtensionKind,
oclabels.OwnerNameKey: ce.GetName(),
}

c, err := cache.New(cfg, cache.Options{
Scheme: scheme,
DefaultLabelSelector: tgtLabels.AsSelector(),
})
if err != nil {
return fmt.Errorf("creating cache for ClusterExtension %q: %w", ce.Name, err)
}

for _, obj := range objs {
err = ctrl.Watch(
source.Kind(
c,
obj,
handler.TypedEnqueueRequestForOwner[client.Object](
scheme,
i.mapper,
ce,
),
nil,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This nil being passed in here results in a nil pointer dereference. I don't recall why it was put there but it isn't needed and should be removed.

),
)
if err != nil {
return fmt.Errorf("creating watch for ClusterExtension %q managed resource %s: %w", ce.Name, obj.GetObjectKind().GroupVersionKind(), err)
}
}
everettraven marked this conversation as resolved.
Show resolved Hide resolved

if data, ok := i.extensionCaches[ce.GetName()]; ok {
data.Cancel()
}

ctx, cancel := context.WithCancel(ctx)
go c.Start(ctx)

Check failure on line 127 in internal/contentmanager/contentmanager.go

View workflow job for this annotation

GitHub Actions / lint

Error return value of `c.Start` is not checked (errcheck)
i.extensionCaches[ce.Name] = extensionCacheData{
Cache: c,
Cancel: cancel,
}
Comment on lines +122 to +131
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an initial pass, we are thinking to just blow away and recreate a new cache every time this is called. This is definitely not ideal but gives us a starting point for integration and making sure the core logic functions appropriately. We will create a follow up issue to track optimizing the implementation such that the same cache is used to add and remove informers as necessary based on changes to the set of resources managed.


return nil
}

func (i *instance) RemoveManagedContent(ce *v1alpha1.ClusterExtension) error {
if data, ok := i.extensionCaches[ce.GetName()]; ok {
data.Cancel()
delete(i.extensionCaches, ce.GetName())
}

return nil
}
Loading