Skip to content

✨ remove unnecessary flag, optimize catalog watch handler, stop watching non-existent unpack pods#941

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.

Sign up for GitHub

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
tmshort merged 1 commit into from
Jun 17, 2024
Merged

✨ remove unnecessary flag, optimize catalog watch handler, stop watching non-existent unpack pods #941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
74 changes: 45 additions & 29 deletions cmd/manager/main.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,7 +17,7 @@ limitations under the License.
package main

import (
"crypto/x509"
"context"
"flag"
"fmt"
"net/url"
Expand All@@ -38,13 +38,12 @@ import (
"sigs.k8s.io/controller-runtime/pkg/metrics/server"

helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
"github.com/operator-framework/rukpak/pkg/finalizer"
registryv1handler "github.com/operator-framework/rukpak/pkg/handler"
"github.com/operator-framework/rukpak/pkg/provisioner/registry"
"github.com/operator-framework/rukpak/pkg/source"
"github.com/operator-framework/rukpak/pkg/storage"

"github.com/operator-framework/operator-controller/api/v1alpha1"
ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/catalogmetadata/cache"
catalogclient "github.com/operator-framework/operator-controller/internal/catalogmetadata/client"
"github.com/operator-framework/operator-controller/internal/controllers"
Expand DownExpand Up@@ -74,14 +73,13 @@ func podNamespace() string {

func main() {
var (
metricsAddr string
enableLeaderElection bool
probeAddr string
cachePath string
operatorControllerVersion bool
systemNamespace string
provisionerStorageDirectory string
caCert string
metricsAddr string
enableLeaderElection bool
probeAddr string
cachePath string
operatorControllerVersion bool
systemNamespace string
caCert string
)
flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
Expand All@@ -92,7 +90,6 @@ func main() {
flag.StringVar(&cachePath, "cache-path", "/var/cache", "The local directory path used for filesystem based caching")
flag.BoolVar(&operatorControllerVersion, "version", false, "Prints operator-controller version information")
flag.StringVar(&systemNamespace, "system-namespace", "", "Configures the namespace that gets used to deploy system resources.")
flag.StringVar(&provisionerStorageDirectory, "provisioner-storage-dir", storage.DefaultBundleCacheDir, "The directory that is used to store bundle contents.")
opts := zap.Options{
Development: true,
}
Expand All@@ -114,7 +111,7 @@ func main() {
systemNamespace = podNamespace()
}

dependentRequirement, err := k8slabels.NewRequirement(labels.OwnerKindKey, selection.In, []string{v1alpha1.ClusterExtensionKind})
dependentRequirement, err := k8slabels.NewRequirement(labels.OwnerKindKey, selection.In, []string{ocv1alpha1.ClusterExtensionKind})
if err != nil {
setupLog.Error(err, "unable to create dependent label selector for cache")
os.Exit(1)
Expand All@@ -130,7 +127,7 @@ func main() {
LeaderElectionID: "9c4404e7.operatorframework.io",
Cache: crcache.Options{
ByObject: map[client.Object]crcache.ByObject{
&v1alpha1.ClusterExtension{}: {},
&ocv1alpha1.ClusterExtension{}: {},
},
DefaultNamespaces: map[string]crcache.Config{
systemNamespace: {},
Expand DownExpand Up@@ -162,9 +159,14 @@ func main() {
cl := mgr.GetClient()
catalogClient := catalogclient.New(cl, cache.NewFilesystemCache(cachePath, httpClient))

cfgGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(), helmclient.StorageNamespaceMapper(func(o client.Object) (string, error) {
return systemNamespace, nil
}))
installNamespaceMapper := helmclient.ObjectToStringMapper(func(obj client.Object) (string, error) {
ext := obj.(*ocv1alpha1.ClusterExtension)
return ext.Spec.InstallNamespace, nil
})
cfgGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(),
helmclient.StorageNamespaceMapper(installNamespaceMapper),
helmclient.ClientNamespaceMapper(installNamespaceMapper),
)
if err != nil {
setupLog.Error(err, "unable to config for creating helm client")
os.Exit(1)
Expand All@@ -176,37 +178,45 @@ func main() {
os.Exit(1)
}

bundleFinalizers := crfinalizer.NewFinalizers()
unpacker, err := source.NewDefaultUnpacker(mgr, systemNamespace, filepath.Join(cachePath, "unpack"), (*x509.CertPool)(nil))
if err != nil {
setupLog.Error(err, "unable to create unpacker")
os.Exit(1)
clusterExtensionFinalizers := crfinalizer.NewFinalizers()
unpacker:= &source.ImageRegistry{
BaseCachePath: filepath.Join(cachePath, "unpack"),
// TODO: This needs to be derived per extension via ext.Spec.InstallNamespace
AuthNamespace: systemNamespace,
}

if err := bundleFinalizers.Register(finalizer.CleanupUnpackCacheKey, &finalizer.CleanupUnpackCache{Unpacker: unpacker}); err != nil {
setupLog.Error(err, "unable to register finalizer", "finalizerKey", finalizer.CleanupUnpackCacheKey)
domain := ocv1alpha1.GroupVersion.Group
cleanupUnpackCacheKey := fmt.Sprintf("%s/cleanup-unpack-cache", domain)
deleteCachedBundleKey := fmt.Sprintf("%s/delete-cached-bundle", domain)
if err := clusterExtensionFinalizers.Register(cleanupUnpackCacheKey, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
ext := obj.(*ocv1alpha1.ClusterExtension)
return crfinalizer.Result{}, os.RemoveAll(filepath.Join(unpacker.BaseCachePath, ext.GetName()))
})); err != nil {
setupLog.Error(err, "unable to register finalizer", "finalizerKey", cleanupUnpackCacheKey)
os.Exit(1)
}

localStorage := &storage.LocalDirectory{
RootDirectory: provisionerStorageDirectory,
RootDirectory: filepath.Join(cachePath, "bundles"),
URL: url.URL{},
}

if err := bundleFinalizers.Register(finalizer.DeleteCachedBundleKey, &finalizer.DeleteCachedBundle{Storage: localStorage}); err != nil {
setupLog.Error(err, "unable to register finalizer", "finalizerKey", finalizer.DeleteCachedBundleKey)
if err := clusterExtensionFinalizers.Register(deleteCachedBundleKey, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
ext := obj.(*ocv1alpha1.ClusterExtension)
return crfinalizer.Result{}, localStorage.Delete(ctx, ext)
})); err != nil {
setupLog.Error(err, "unable to register finalizer", "finalizerKey", deleteCachedBundleKey)
os.Exit(1)
}

if err = (&controllers.ClusterExtensionReconciler{
Client: cl,
ReleaseNamespace: systemNamespace,
BundleProvider: catalogClient,
ActionClientGetter: acg,
Unpacker: unpacker,
Storage: localStorage,
InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
Handler: registryv1handler.HandlerFunc(registry.HandleBundleDeployment),
Finalizers: clusterExtensionFinalizers,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
os.Exit(1)
Expand All@@ -229,3 +239,9 @@ func main() {
os.Exit(1)
}
}

type finalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Result, error)

func (f finalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
return f(ctx, obj)
}
18 changes: 4 additions & 14 deletions config/base/rbac/role.yaml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -27,25 +27,15 @@ rules:
- apiGroups:
- ""
resources:
- configmaps
verbs:
- list
- watch
- apiGroups:
- ""
resources:
- pods
- secrets
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- ""
resources:
- pods/log
verbs:
- get
- apiGroups:
- olm.operatorframework.io
resources:
Expand Down
78 changes: 29 additions & 49 deletions internal/controllers/clusterextension_controller.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -36,7 +36,6 @@ import (
"helm.sh/helm/v3/pkg/postrender"
"helm.sh/helm/v3/pkg/release"
"helm.sh/helm/v3/pkg/storage/driver"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All@@ -48,9 +47,9 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
crcontroller "sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/event"
crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
crhandler "sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/predicate"
Expand DownExpand Up@@ -80,7 +79,6 @@ import (
// ClusterExtensionReconciler reconciles a ClusterExtension object
type ClusterExtensionReconciler struct {
client.Client
ReleaseNamespace string
BundleProvider BundleProvider
Unpacker rukpaksource.Unpacker
ActionClientGetter helmclient.ActionClientGetter
Expand All@@ -91,6 +89,7 @@ type ClusterExtensionReconciler struct {
controller crcontroller.Controller
cache cache.Cache
InstalledBundleGetter InstalledBundleGetter
Finalizers crfinalizer.Finalizers
}

type InstalledBundleGetter interface {
Expand All@@ -104,9 +103,7 @@ const (
//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions,verbs=get;list;watch
//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions/status,verbs=update;patch
//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions/finalizers,verbs=update
//+kubebuilder:rbac:groups=core,resources=pods,verbs=list;watch;create;delete
//+kubebuilder:rbac:groups=core,resources=configmaps,verbs=list;watch
//+kubebuilder:rbac:groups=core,resources=pods/log,verbs=get
//+kubebuilder:rbac:groups=core,resources=secrets,verbs=create;update;patch;delete;get;list;watch
//+kubebuilder:rbac:groups=*,resources=*,verbs=*

//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=clustercatalogs,verbs=list;watch
Expand DownExpand Up@@ -196,6 +193,25 @@ func checkForUnexpectedFieldChange(a, b ocv1alpha1.ClusterExtension) bool {
*/
//nolint:unparam
func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (ctrl.Result, error) {
finalizeResult, err := r.Finalizers.Finalize(ctx, ext)
if err != nil {
// TODO: For now, this error handling follows the pattern of other error handling.
// Namely: zero just about everything out, throw our hands up, and return an error.
// This is not ideal, and we should consider a more nuanced approach that resolves
// as much status as possible before returning, or at least keeps previous state if
// it is properly labeled with its observed generation.
ext.Status.ResolvedBundle = nil
ext.Status.InstalledBundle = nil
setResolvedStatusConditionFailed(ext, err.Error())
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonResolutionFailed, err.Error())
return ctrl.Result{}, err
}
if finalizeResult.Updated || finalizeResult.StatusUpdated {
// On create: make sure the finalizer is applied before we do anything
// On delete: make sure we do nothing after the finalizer is removed
return ctrl.Result{}, nil
}

// run resolution
bundle, err := r.resolve(ctx, *ext)
if err != nil {
Expand DownExpand Up@@ -300,7 +316,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp

switch state {
case stateNeedsInstall:
rel, err = ac.Install(ext.GetName(), r.ReleaseNamespace, chrt, values, func(install *action.Install) error {
rel, err = ac.Install(ext.GetName(), ext.Spec.InstallNamespace, chrt, values, func(install *action.Install) error {
install.CreateNamespace = false
install.Labels = map[string]string{labels.BundleNameKey: bundle.Name, labels.PackageNameKey: bundle.Package, labels.BundleVersionKey: bundleVersion.String()}
return nil
Expand All@@ -310,7 +326,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
return ctrl.Result{}, err
}
case stateNeedsUpgrade:
rel, err = ac.Upgrade(ext.GetName(), r.ReleaseNamespace, chrt, values, helmclient.AppendUpgradePostRenderer(post))
rel, err = ac.Upgrade(ext.GetName(), ext.Spec.InstallNamespace, chrt, values, helmclient.AppendUpgradePostRenderer(post))
if err != nil {
setInstalledStatusConditionFailed(ext, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonUpgradeFailed, err))
return ctrl.Result{}, err
Expand DownExpand Up@@ -574,7 +590,6 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error {
return true
},
}).
Watches(&corev1.Pod{}, mapOwneeToOwnerHandler(mgr.GetClient(), mgr.GetLogger(), &ocv1alpha1.ClusterExtension{})).

@varshaprasad96varshaprasad96Jun 14, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we're on it - we should also be able to remove the rbac to watch pods:

operator-controller/internal/controllers/clusterextension_controller.go

Lines 105 to 107 in cb63023

//+kubebuilder:rbac:groups=core,resources=pods,verbs=list;watch;create;delete
//+kubebuilder:rbac:groups=core,resources=configmaps,verbs=list;watch
//+kubebuilder:rbac:groups=core,resources=pods/log,verbs=get
.

If not in this PR, it can be done in follow up too

@joelanfordjoelanfordJun 14, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I can do it now.

@joelanfordjoelanfordJun 14, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Pushed a change. I removed the pods stuff, and changed the configmaps RBAC (what was that for?) to secrets since the helm stuff needs to manage release secrets.

It's all sorta moot though because:

  • We give ourselves */*/* in the next line.
  • We'll eventually drop the secrets permission here because we'll expect the SA to have the necessary permission to handle their own release secret.

@varshaprasad96varshaprasad96Jun 14, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

and changed the configmaps RBAC (what was that for?)

I think it may have gotten ported over from Extension, where Kapp needed CRUD permissions for configmaps.

Build(r)

if err != nil {
Expand All@@ -587,47 +602,12 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error {
return nil
}

func mapOwneeToOwnerHandler(cl client.Client, log logr.Logger, owner client.Object) crhandler.EventHandler {
return crhandler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request {
ownerGVK, err := apiutil.GVKForObject(owner, cl.Scheme())
if err != nil {
log.Error(err, "map ownee to owner: lookup GVK for owner")
return nil
}

type ownerInfo struct {
key types.NamespacedName
gvk schema.GroupVersionKind
}
var oi *ownerInfo

for _, ref := range obj.GetOwnerReferences() {
gv, err := schema.ParseGroupVersion(ref.APIVersion)
if err != nil {
log.Error(err, fmt.Sprintf("map ownee to owner: parse ownee's owner reference group version %q", ref.APIVersion))
return nil
}
refGVK := gv.WithKind(ref.Kind)
if refGVK == ownerGVK && ref.Controller != nil && *ref.Controller {
oi = &ownerInfo{
key: types.NamespacedName{Name: ref.Name},
gvk: ownerGVK,
}
break
}
}
if oi == nil {
return nil
}
return []reconcile.Request{{NamespacedName: oi.key}}
})
}

// Generate reconcile requests for all cluster extensions affected by a catalog change
func clusterExtensionRequestsForCatalog(c client.Reader, logger logr.Logger) crhandler.MapFunc {
return func(ctx context.Context, _ client.Object) []reconcile.Request {
// no way of associating an extension to a catalog so create reconcile requests for everything
clusterExtensions := ocv1alpha1.ClusterExtensionList{}
clusterExtensions := metav1.PartialObjectMetadataList{}
clusterExtensions.SetGroupVersionKind(ocv1alpha1.GroupVersion.WithKind("ClusterExtensionList"))
err := c.List(ctx, &clusterExtensions)
if err != nil {
logger.Error(err, "unable to enqueue cluster extensions for catalog reconcile")
Expand DownExpand Up@@ -655,16 +635,16 @@ const (
stateError releaseState = "Error"
)

func (r *ClusterExtensionReconciler) getReleaseState(cl helmclient.ActionInterface, obj metav1.Object, chrt *chart.Chart, values chartutil.Values, post *postrenderer) (*release.Release, releaseState, error) {
currentRelease, err := cl.Get(obj.GetName())
func (r *ClusterExtensionReconciler) getReleaseState(cl helmclient.ActionInterface, ext *ocv1alpha1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post *postrenderer) (*release.Release, releaseState, error) {
currentRelease, err := cl.Get(ext.GetName())
if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
return nil, stateError, err
}
if errors.Is(err, driver.ErrReleaseNotFound) {
return nil, stateNeedsInstall, nil
}

desiredRelease, err := cl.Upgrade(obj.GetName(), r.ReleaseNamespace, chrt, values, func(upgrade *action.Upgrade) error {
desiredRelease, err := cl.Upgrade(ext.GetName(), ext.Spec.InstallNamespace, chrt, values, func(upgrade *action.Upgrade) error {
upgrade.DryRun = true
return nil
}, helmclient.AppendUpgradePostRenderer(post))
Expand Down
2 changes: 2 additions & 0 deletions internal/controllers/clusterextension_registryv1_validation_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -14,6 +14,7 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/rand"
ctrl "sigs.k8s.io/controller-runtime"
crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"

"github.com/operator-framework/operator-registry/alpha/declcfg"
"github.com/operator-framework/operator-registry/alpha/property"
Expand DownExpand Up@@ -116,6 +117,7 @@ func TestClusterExtensionRegistryV1DisallowDependencies(t *testing.T) {
ActionClientGetter: helmClientGetter,
Unpacker: unpacker,
InstalledBundleGetter: mockInstalledBundleGetter,
Finalizers: crfinalizer.NewFinalizers(),

@joelanfordjoelanfordJun 15, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's a bit disconcerting to me that so much of the ClusterExtensionReconciler can be constructed so differently than the real one can. We should discuss further, but off the top of my head, I wonder if we can use the exact same reconciler as is configured in main.go.

  1. For catalogd we can use httptest to setup a mock catalogd server and use the real ClusterExtension http client.
  2. An image registry is an http server. I wonder if there is an in-process implementation that would be easy to run for test purposes.
  3. Use t.TempDir() subdirectories for any necessary local storage and caching that happens during reconcile.
  4. For the kubernetes cluster, we use envtest.

If we had that setup, we wouldn't actually need to configure the ClusterExtension reconciler any differently.

@joelanfordjoelanfordJun 15, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

More immediately, we should have unit tests that make sure finalizers are actually being handled properly during reconcile.

@varshaprasad96varshaprasad96Jun 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a bit disconcerting to me that so much of the ClusterExtensionReconciler can be constructed so differently than the real one can.

This is definitely up for discussion and needs separate set of effort. The whole point of these unit tests is to make sure that the reconciler code path traversed during various instances is as expected. Which is why we ensure that the mock unpacker returns quickly as expected. The code coverage for unpacking and storage has separate unit tests (currently in rukpak).

I'm not sure if we immediately need to focus on this - rather if possible try finishing up #879 as immediately as possible (even if it requires us to mock some of the components) to ensure we don't skip any nuances in the reconciler code.

}

installNamespace := fmt.Sprintf("test-ns-%s", rand.String(8))
Expand Down
2 changes: 2 additions & 0 deletions internal/controllers/suite_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -32,6 +32,7 @@ import (
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/envtest"
crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"

helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
"github.com/operator-framework/rukpak/api/v1alpha2"
Expand DownExpand Up@@ -126,6 +127,7 @@ func newClientAndReconciler(t *testing.T, bundle *ocv1alpha1.BundleMetadata) (cl
Unpacker: unpacker,
Storage: store,
InstalledBundleGetter: mockInstalledBundleGetter,
Finalizers: crfinalizer.NewFinalizers(),
}
return cl, reconciler
}
Expand Down
Loading