Address Reviews

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")
@@ -220,6 +224,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
@@ -302,7 +312,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 +353,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,7 +382,7 @@ 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
 		}
 	}
@@ -804,3 +814,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,
+		})
+	}
+}