Skip to content

✨ Set up right watches and all labels to postrenderer #763

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
merged 1 commit into from
Apr 18, 2024
Merged

✨ Set up right watches and all labels to postrenderer #763

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
82 changes: 74 additions & 8 deletions internal/controllers/clusterextension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ 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"
apierrors "k8s.io/apimachinery/pkg/api/errors"
apimeta "k8s.io/apimachinery/pkg/api/meta"
Expand All @@ -49,6 +50,7 @@ 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"
crhandler "sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/log"
Expand Down Expand Up @@ -156,6 +158,12 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
return ctrl.Result{}, err
}

bundleVersion, err := bundle.Version()
if err != nil {
setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", "unable to get resolved bundle version", err), ext.Generation)
return ctrl.Result{}, err
}

// Now we can set the Resolved Condition, and the resolvedBundleSource field to the bundle.Image value.
ext.Status.ResolvedBundle = bundleMetadataFor(bundle)
setResolvedStatusConditionSuccess(&ext.Status.Conditions, fmt.Sprintf("resolved to %q", bundle.Image), ext.GetGeneration())
Expand All @@ -164,7 +172,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
// Considering only image source.

// Generate a BundleSource, and then pass this and the ClusterExtension to Unpack
bs := r.GenerateExpectedBundleSource(*ext, bundle.Image)
bs := r.GenerateExpectedBundleSource(bundle.Image)
unpackResult, err := r.Unpacker.Unpack(ctx, bs, ext)
if err != nil {
return ctrl.Result{}, updateStatusUnpackFailing(&ext.Status, fmt.Errorf("source bundle content: %v", err))
Expand Down Expand Up @@ -223,8 +231,11 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp

post := &postrenderer{
labels: map[string]string{
util.CoreOwnerKindKey: ocv1alpha1.ClusterExtensionKind,
util.CoreOwnerNameKey: ext.GetName(),
util.CoreOwnerKindKey: ocv1alpha1.ClusterExtensionKind,
util.CoreOwnerNameKey: ext.GetName(),
util.ResolvedbundleName: bundle.Name,
util.ResolvedbundlePackageName: bundle.Package,
util.ResolvedbundleVersion: bundleVersion.String(),
Comment on lines +236 to +238
Copy link
Member

Choose a reason for hiding this comment

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

No catalogID here? Can we drop that constant?

Copy link
Contributor

Choose a reason for hiding this comment

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

#765

},
}

Expand Down Expand Up @@ -390,7 +401,7 @@ func SetDeprecationStatus(ext *ocv1alpha1.ClusterExtension, bundle *catalogmetad
}
}

func (r *ClusterExtensionReconciler) GenerateExpectedBundleSource(o ocv1alpha1.ClusterExtension, bundlePath string) *rukpakapi.BundleSource {
func (r *ClusterExtensionReconciler) GenerateExpectedBundleSource(bundlePath string) *rukpakapi.BundleSource {
return &rukpakapi.BundleSource{
Type: rukpakapi.SourceTypeImage,
Image: rukpakapi.ImageSource{
Expand Down Expand Up @@ -449,7 +460,7 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error {
For(&ocv1alpha1.ClusterExtension{}).
Watches(&catalogd.Catalog{},
crhandler.EnqueueRequestsFromMapFunc(clusterExtensionRequestsForCatalog(mgr.GetClient(), mgr.GetLogger()))).
Owns(&rukpakv1alpha2.BundleDeployment{}).
Watches(&corev1.Pod{}, mapOwneeToOwnerHandler(mgr.GetClient(), mgr.GetLogger(), &ocv1alpha1.ClusterExtension{})).
Build(r)

if err != nil {
Expand All @@ -460,6 +471,58 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error {
return nil
}

func mapOwneeToOwnerHandler(cl client.Client, log logr.Logger, owner client.Object) crhandler.EventHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a second to "get" this name. Is "ownee" even a word? :)

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
}
owneeGVK, err := apiutil.GVKForObject(obj, cl.Scheme())
if err != nil {
log.Error(err, "map ownee to owner: lookup GVK for ownee")
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
}

if err := cl.Get(ctx, oi.key, owner); client.IgnoreNotFound(err) != nil {
log.Info("map ownee to owner: get owner",
"ownee", client.ObjectKeyFromObject(obj),
"owneeKind", owneeGVK,
"owner", oi.key,
"ownerKind", oi.gvk,
"error", err.Error(),
)
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 {
Expand Down Expand Up @@ -513,7 +576,7 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, clusterExtensi
var installedVersion string
// Do not include bundle versions older than currently installed unless UpgradeConstraintPolicy = 'Ignore'
if clusterExtension.Spec.UpgradeConstraintPolicy != ocv1alpha1.UpgradeConstraintPolicyIgnore {
installedVersionSemver, err := r.getInstalledVersion(ctx, clusterExtension)
installedVersionSemver, err := r.getInstalledVersion(clusterExtension)
if err != nil && !apierrors.IsNotFound(err) {
return nil, err
}
Expand Down Expand Up @@ -548,14 +611,17 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, clusterExtensi
return resultSet[0], nil
}

func (r *ClusterExtensionReconciler) getInstalledVersion(ctx context.Context, clusterExtension ocv1alpha1.ClusterExtension) (*bsemver.Version, error) {
func (r *ClusterExtensionReconciler) getInstalledVersion(clusterExtension ocv1alpha1.ClusterExtension) (*bsemver.Version, error) {
cl, err := r.ActionClientGetter.ActionClientFor(&clusterExtension)
if err != nil {
return nil, err
}

// Clarify - Every release will have a unique name as the cluster extension?
// Also filter relases whose owner is the operator controller?
// I think this should work, given we are setting the release Name to the clusterExtension name.
// If not, the other option is to get the Helm secret in the release namespace, list all the releases,
// get the chart annotations.
release, err := cl.Get(clusterExtension.GetName())
if err != nil {
return nil, err
Expand All @@ -570,7 +636,7 @@ func (r *ClusterExtensionReconciler) getInstalledVersion(ctx context.Context, cl
}

// TODO: when the chart is created these annotations are to be added.
existingVersion, ok := chart.Metadata.Annotations[bundleVersionKey]
existingVersion, ok := chart.Metadata.Annotations[util.ResolvedbundleVersion]
if !ok {
return nil, fmt.Errorf("chart %q: missing bundle version", chart.Name())
}
Expand Down
2 changes: 1 addition & 1 deletion internal/rukpak/handler/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const (
manifestsDir = "manifests"
)

func HandleClusterExtension(ctx context.Context, fsys fs.FS, ext *ocv1alpha1.ClusterExtension) (*chart.Chart, chartutil.Values, error) {
func HandleClusterExtension(_ context.Context, fsys fs.FS, ext *ocv1alpha1.ClusterExtension) (*chart.Chart, chartutil.Values, error) {
plainFS, err := convert.RegistryV1ToPlain(fsys, ext.Spec.WatchNamespaces)
if err != nil {
return nil, nil, fmt.Errorf("convert registry+v1 bundle to plain+v0 bundle: %v", err)
Expand Down
8 changes: 6 additions & 2 deletions internal/rukpak/util/labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package util

const (
CoreOwnerKindKey = "core.rukpak.io/owner-kind"
CoreOwnerNameKey = "core.rukpak.io/owner-name"
CoreOwnerKindKey = "core.clusterextension.io/owner-kind"
CoreOwnerNameKey = "core.clusterextension.io/owner-name"
ResolvedBundleCatalogID = "core.clusterextension.io/catalog-id"
ResolvedbundlePackageName = "core.clusterextension.io/package-name"
ResolvedbundleName = "core.clusterextension.io/bundle-name"
ResolvedbundleVersion = "core.clusterextension.io/bundle-version"
Comment on lines +4 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe we "own" the clusterextension.io domain, but this is a PoC...

Copy link
Member

Choose a reason for hiding this comment

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

+1 these should definitely be updated before 1.0.0

I'd suggest olm.operatorframework.io/*

Copy link
Member

Choose a reason for hiding this comment

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

Also a few nits on naming:

  1. CamelCase words in the identifier (e.g. Resolvedbundle should be ResolvedBundle)
  2. Consistently use Key suffix to all of the identifiers to clarify that these are to be used for label/annotation keys.
  3. Drop Core prefix.

Overall, align the identifier name with the value of the constant. e.g.

  • "core.clusterextension.io/catalog-id" would map to CatalogIDKey
  • "core.clusterextension.io/package-name" would map to PackageNameKey
  • etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a PoC branch, so it's not going into main as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

#765

)