Address Reviews

Signed-off-by: Varsha Prasad Narsing <[email protected]>

Author: varshaprasad96

api/v1alpha1/clusterextension_types.go

@@ -101,11 +101,9 @@ const (
 	ReasonInstallationSucceeded     = "InstallationSucceeded"
 	ReasonResolutionFailed          = "ResolutionFailed"
 
-	ReasonSuccess                  = "Success"
-	ReasonDeprecated               = "Deprecated"
-	ReasonErrorGettingReleaseState = "ErrorGettingReleaseState"
-	ReasonUpgradeFailed            = "UpgradeFailed"
-	ReasonCreateDynamicWatchFailed = "CreateDynamicWatchFailed"
+	ReasonSuccess       = "Success"
+	ReasonDeprecated    = "Deprecated"
+	ReasonUpgradeFailed = "UpgradeFailed"
 )
 
 func init() {
@@ -124,14 +122,14 @@ func init() {
 		ReasonInstallationSucceeded,
 		ReasonResolutionFailed,
 		ReasonInstallationFailed,
-		ReasonInstallationStatusUnknown,
 		ReasonSuccess,
 		ReasonDeprecated,
-		ReasonErrorGettingReleaseState,
 		ReasonUpgradeFailed,
-		ReasonCreateDynamicWatchFailed,
 		ReasonBundleLoadFailed,
 		ReasonErrorGettingClient,
+		// TODO: this reason is not being used in the reconciler, it will be removed
+		// when we fix the tests. Avoiding removal here, to reduce diffs.
+		ReasonInstallationStatusUnknown,
 	)
 }
 

cmd/manager/main.go

@@ -77,14 +77,12 @@ func main() {
 		probeAddr                   string
 		cachePath                   string
 		operatorControllerVersion   bool
-		httpExternalAddr            string
 		systemNamespace             string
 		unpackImage                 string
 		provisionerStorageDirectory 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.")
-	flag.StringVar(&httpExternalAddr, "http-external-address", "http://localhost:8080", "The external address at which the http server is reachable.")
 	flag.BoolVar(&enableLeaderElection, "leader-elect", false,
 		"Enable leader election for controller manager. "+
 			"Enabling this will ensure there is only one active controller manager.")
@@ -177,15 +175,9 @@ func main() {
 		os.Exit(1)
 	}
 
-	storageURL, err := url.Parse(fmt.Sprintf("%s/bundles/", httpExternalAddr))
-	if err != nil {
-		setupLog.Error(err, "unable to parse bundle content server URL")
-		os.Exit(1)
-	}
-
 	localStorage := &storage.LocalDirectory{
 		RootDirectory: provisionerStorageDirectory,
-		URL:           *storageURL,
+		URL:           url.URL{},
 	}
 
 	if err = (&controllers.ClusterExtensionReconciler{

internal/catalogmetadata/types.go

@@ -73,7 +73,7 @@ func (b *Bundle) loadPackage() error {
 	if b.semVersion == nil {
 		semVer, err := bsemver.Parse(b.bundlePackage.Version)
 		if err != nil {
-			return fmt.Errorf("could not parse semver %q for bundle '%s': %s", b.bundlePackage.Version, b.Bundle.Name, err)
+			return fmt.Errorf("could not parse semver %q for bundle '%s': %s", b.bundlePackage.Version, b.Name, err)
 		}
 		b.semVersion = &semVer
 	}

internal/controllers/clusterextension_controller.go

@@ -46,6 +46,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
 	apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
@@ -63,6 +64,7 @@ import (
 	catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1"
 	helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
 	"github.com/operator-framework/operator-registry/alpha/declcfg"
+	"github.com/operator-framework/operator-registry/alpha/property"
 	rukpakv1alpha2 "github.com/operator-framework/rukpak/api/v1alpha2"
 	helmpredicate "github.com/operator-framework/rukpak/pkg/helm-operator-plugins/predicate"
 	rukpaksource "github.com/operator-framework/rukpak/pkg/source"
@@ -106,6 +108,8 @@ type ClusterExtensionReconciler struct {
 //+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=catalogs,verbs=list;watch
 //+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=catalogmetadata,verbs=list;watch
 
+// The operator controller needs to watch all the bundle objects and reconcile accordingly. Though not ideal, but these permissions are required.
+// This has been taken from rukpak, and an issue was created before to discuss it: https://github.com/operator-framework/rukpak/issues/800.
 func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	l := log.FromContext(ctx).WithName("operator-controller")
 	l.V(1).Info("starting")
@@ -210,6 +214,15 @@ func (r *ClusterExtensionReconciler) handleResolutionErrors(ext *ocv1alpha1.Clus
 // But in the future we might update this function
 // to return different results (e.g. requeue).
 //
+/* The reconcile functions performs the following major tasks:
+1. Resolution: Run the resolution to find the bundle from the catalog which needs to be installed.
+2. Validate: Ensure that the bundle returned from the resolution for install meets our requirements.
+3. Unpack: Unpack the contents from the bundle and store in a localdir in the pod.
+4. Install: The process of installing involves:
+4.1 Converting the CSV in the bundle into a set of plain k8s objects.
+4.2 Generating a chart from k8s objects.
+4.3 Apply the release on cluster.
+*/
 //nolint:unparam
 func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (ctrl.Result, error) {
 	l := log.FromContext(ctx).WithName("operator-controller")
@@ -220,6 +233,12 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 		return r.handleResolutionErrors(ext, err)
 	}
 
+	if err := r.validateBundle(bundle); err != nil {
+		setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
+		setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
+		return ctrl.Result{}, err
+	}
+
 	bundleVersion, err := bundle.Version()
 	if err != nil {
 		ext.Status.ResolvedBundle = nil
@@ -229,14 +248,12 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 		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())
 
-	// Unpack contents into a fs based on the bundle.
-	// Considering only image source.
-
-	// Generate a BundleDeployment from the ClusterExtension to Unpack
+	// Generate a BundleDeployment from the ClusterExtension to Unpack.
+	// Note: The BundleDeployment here is not a k8s API, its a simple Go struct which
+	// necessary embedded values.
 	bd := r.generateBundleDeploymentForUnpack(bundle.Image, ext)
 	unpackResult, err := r.Unpacker.Unpack(ctx, bd)
 	if err != nil {
@@ -253,6 +270,8 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 		updateStatusUnpacking(&ext.Status, unpackResult)
 		return ctrl.Result{}, nil
 	case rukpaksource.StateUnpacked:
+		// TODO: Add finalizer to clean the stored bundles, after https://github.com/operator-framework/rukpak/pull/897
+		// merges.
 		if err := r.Storage.Store(ctx, ext, unpackResult.Bundle); err != nil {
 			return ctrl.Result{}, updateStatusUnpackFailing(&ext.Status, err)
 		}
@@ -302,7 +321,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 
 	rel, state, err := r.getReleaseState(ac, ext, chrt, values, post)
 	if err != nil {
-		setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonErrorGettingReleaseState, err), ext.Generation)
+		setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", rukpakv1alpha2.ReasonErrorGettingReleaseState, err), ext.Generation)
 		return ctrl.Result{}, err
 	}
 
@@ -343,14 +362,14 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 
 	relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name))
 	if err != nil {
-		setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err), ext.Generation)
+		setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", rukpakv1alpha2.ReasonCreateDynamicWatchFailed, err), ext.Generation)
 		return ctrl.Result{}, err
 	}
 
 	for _, obj := range relObjects {
 		uMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
 		if err != nil {
-			setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err), ext.Generation)
+			setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", rukpakv1alpha2.ReasonCreateDynamicWatchFailed, err), ext.Generation)
 			return ctrl.Result{}, err
 		}
 
@@ -372,17 +391,17 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 			return nil
 		}(); err != nil {
 			ext.Status.InstalledBundle = nil
-			setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonCreateDynamicWatchFailed, err), ext.Generation)
+			setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", rukpakv1alpha2.ReasonCreateDynamicWatchFailed, err), ext.Generation)
 			return ctrl.Result{}, err
 		}
 	}
 	ext.Status.InstalledBundle = bundleMetadataFor(bundle)
 	setInstalledStatusConditionSuccess(&ext.Status.Conditions, fmt.Sprintf("Instantiated bundle %s successfully", ext.GetName()), ext.Generation)
 
-	// set the status of the cluster extension based on the respective bundle deployment status conditions.
 	return ctrl.Result{}, nil
 }
 
+// resolve returns a Bundle from the catalog that needs to get installed on the cluster.
 func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) {
 	allBundles, err := r.BundleProvider.Bundles(ctx)
 	if err != nil {
@@ -663,17 +682,14 @@ func clusterExtensionRequestsForCatalog(c client.Reader, logger logr.Logger) crh
 	}
 }
 
+// getInstalledVersion fetches the installed version of a particular bundle from the cluster. To do so, we read the release label
+// which are added during install.
 func (r *ClusterExtensionReconciler) getInstalledVersion(ctx context.Context, clusterExtension ocv1alpha1.ClusterExtension) (*bsemver.Version, error) {
 	cl, err := r.ActionClientGetter.ActionClientFor(ctx, &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 && !errors.Is(err, driver.ErrReleaseNotFound) {
 		return nil, err
@@ -682,7 +698,6 @@ func (r *ClusterExtensionReconciler) getInstalledVersion(ctx context.Context, cl
 		return nil, nil
 	}
 
-	// TODO: when the chart is created these annotations are to be added.
 	existingVersion, ok := release.Labels[labels.BundleVersionKey]
 	if !ok {
 		return nil, fmt.Errorf("release %q: missing bundle version", release.Name)
@@ -804,3 +819,22 @@ func bundleMetadataFor(bundle *catalogmetadata.Bundle) *ocv1alpha1.BundleMetadat
 		Version: ver.String(),
 	}
 }
+
+func (r *ClusterExtensionReconciler) validateBundle(bundle *catalogmetadata.Bundle) error {
+	unsupportedProps := sets.New(
+		property.TypePackageRequired,
+		property.TypeGVKRequired,
+		property.TypeConstraint,
+	)
+	for i := range bundle.Properties {
+		if unsupportedProps.Has(bundle.Properties[i].Type) {
+			return fmt.Errorf(
+				"bundle %q has a dependency declared via property %q which is currently not supported",
+				bundle.Name,
+				bundle.Properties[i].Type,
+			)
+		}
+	}
+
+	return nil
+}

internal/controllers/clusterextension_registryv1_validation_test.go

@@ -31,6 +31,7 @@ func TestClusterExtensionRegistryV1DisallowDependencies(t *testing.T) {
 		name    string
 		bundle  *catalogmetadata.Bundle
 		wantErr string
+		skip    bool
 	}{
 		{
 			name: "package with no dependencies",
@@ -45,6 +46,9 @@ func TestClusterExtensionRegistryV1DisallowDependencies(t *testing.T) {
 				},
 				CatalogName: "fake-catalog",
 			},
+			// Skipping the happy path, since it requires us to mock unpacker, store and the
+			// entire installation. This should be handled in an e2e instead.
+			skip: true,
 		},
 		{
 			name: "package with olm.package.required property",
@@ -96,15 +100,19 @@ func TestClusterExtensionRegistryV1DisallowDependencies(t *testing.T) {
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
-			t.Skip("Skip and replace with e2e for BundleDeployment")
 			defer func() {
 				require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
 			}()
 
+			if tt.skip {
+				return
+			}
+
 			fakeCatalogClient := testutil.NewFakeCatalogClient([]*catalogmetadata.Bundle{tt.bundle})
 			reconciler := &controllers.ClusterExtensionReconciler{
-				Client:         cl,
-				BundleProvider: &fakeCatalogClient,
+				Client:             cl,
+				BundleProvider:     &fakeCatalogClient,
+				ActionClientGetter: acg,
 			}
 
 			installNamespace := fmt.Sprintf("test-ns-%s", rand.String(8))

internal/controllers/common_controller.go

@@ -75,3 +75,23 @@ func setInstalledStatusConditionFailed(conditions *[]metav1.Condition, message s
 		ObservedGeneration: generation,
 	})
 }
+
+// setDeprecationStatusesUnknown sets the deprecation status conditions to unknown.
+func setDeprecationStatusesUnknown(conditions *[]metav1.Condition, message string, generation int64) {
+	conditionTypes := []string{
+		ocv1alpha1.TypeDeprecated,
+		ocv1alpha1.TypePackageDeprecated,
+		ocv1alpha1.TypeChannelDeprecated,
+		ocv1alpha1.TypeBundleDeprecated,
+	}
+
+	for _, conditionType := range conditionTypes {
+		apimeta.SetStatusCondition(conditions, metav1.Condition{
+			Type:               conditionType,
+			Reason:             ocv1alpha1.ReasonDeprecated,
+			Status:             metav1.ConditionUnknown,
+			Message:            message,
+			ObservedGeneration: generation,
+		})
+	}
+}