Addressing latest round of PR feedback

Signed-off-by: Tayler Geiger <[email protected]>

Author: trgeiger

.golangci.yaml

@@ -16,10 +16,6 @@ run:
   # Default timeout is 1m, up to give more room
   timeout: 4m
 
-issues:
-  exclude-dirs:
-    - internal/operator-controller/authorization/internal/kubernetes
-
 linters:
   enable:
   - asciicheck

codecov.yml

@@ -10,5 +10,3 @@ coverage:
     - "cmd/"
     - "internal/"
 
-ignore:
-  - "internal/operator-controller/authorization/internal/kubernetes"

internal/operator-controller/applier/helm_test.go

@@ -17,15 +17,13 @@ import (
 	"helm.sh/helm/v3/pkg/storage/driver"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
-	"github.com/operator-framework/operator-controller/internal/operator-controller/features"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert"
 )
 
@@ -45,6 +43,17 @@ func (p *noOpPreAuthorizer) PreAuthorize(
 	return nil, nil
 }
 
+type errorPreAuthorizer struct{}
+
+func (p *errorPreAuthorizer) PreAuthorize(
+	ctx context.Context,
+	ext *ocv1.ClusterExtension,
+	manifestReader io.Reader,
+) ([]authorization.ScopedPolicyRules, error) {
+	// Always returns no missing rules and an error
+	return nil, errors.New("problem running preauthorization")
+}
+
 func (mp *mockPreflight) Install(context.Context, *release.Release) error {
 	return mp.installErr
 }
@@ -270,7 +279,6 @@ func TestApply_Installation(t *testing.T) {
 }
 
 func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
-	featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true)
 
 	t.Run("fails during dry-run installation", func(t *testing.T) {
 		mockAcg := &mockActionGetter{
@@ -313,18 +321,17 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
 		require.Nil(t, objs)
 	})
 
-	t.Run("fails during installation", func(t *testing.T) {
+	t.Run("fails during installation because of missing RBAC rules", func(t *testing.T) {
 		mockAcg := &mockActionGetter{
 			getClientErr: driver.ErrReleaseNotFound,
-			installErr:   errors.New("failed installing chart"),
 			desiredRel: &release.Release{
 				Info:     &release.Info{Status: release.StatusDeployed},
 				Manifest: validManifest,
 			},
 		}
 		helmApplier := applier.Helm{
 			ActionClientGetter:  mockAcg,
-			PreAuthorizer:       &noOpPreAuthorizer{},
+			PreAuthorizer:       &errorPreAuthorizer{},
 			BundleToHelmChartFn: convert.RegistryV1ToHelmChart,
 		}
 		// Use a ClusterExtension with valid Spec fields.
@@ -338,8 +345,37 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) {
 		}
 		objs, state, err := helmApplier.Apply(context.TODO(), validFS, validCE, testObjectLabels, testStorageLabels)
 		require.Error(t, err)
-		require.ErrorContains(t, err, "installing chart")
-		require.Equal(t, applier.StateNeedsInstall, state)
+		require.ErrorContains(t, err, "problem running preauthorization")
+		require.Equal(t, "", state)
+		require.Nil(t, objs)
+	})
+
+	t.Run("fails during installation because of pre-authorization failure", func(t *testing.T) {
+		mockAcg := &mockActionGetter{
+			getClientErr: driver.ErrReleaseNotFound,
+			desiredRel: &release.Release{
+				Info:     &release.Info{Status: release.StatusDeployed},
+				Manifest: validManifest,
+			},
+		}
+		helmApplier := applier.Helm{
+			ActionClientGetter:  mockAcg,
+			PreAuthorizer:       &errorPreAuthorizer{},
+			BundleToHelmChartFn: convert.RegistryV1ToHelmChart,
+		}
+		// Use a ClusterExtension with valid Spec fields.
+		validCE := &ocv1.ClusterExtension{
+			Spec: ocv1.ClusterExtensionSpec{
+				Namespace: "default",
+				ServiceAccount: ocv1.ServiceAccountReference{
+					Name: "default",
+				},
+			},
+		}
+		objs, state, err := helmApplier.Apply(context.TODO(), validFS, validCE, testObjectLabels, testStorageLabels)
+		require.Error(t, err)
+		require.ErrorContains(t, err, "problem running preauthorization")
+		require.Equal(t, "", state)
 		require.Nil(t, objs)
 	})
 
@@ -488,7 +524,6 @@ func TestApply_Upgrade(t *testing.T) {
 }
 
 func TestApply_InstallationWithSingleOwnNamespaceInstallSupportEnabled(t *testing.T) {
-	featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.SingleOwnNamespaceInstallSupport, true)
 
 	t.Run("generates bundle resources using the configured watch namespace", func(t *testing.T) {
 		var expectedWatchNamespace = "watch-namespace"

internal/operator-controller/features/features.go

@@ -1,6 +1,7 @@
 package features
 
 import (
+	"fmt"
 	"sort"
 
 	"github.com/go-logr/logr"
@@ -51,8 +52,9 @@ func LogFeatureGateStates(log logr.Logger, fg featuregate.FeatureGate) {
 		return string(featureKeys[i]) < string(featureKeys[j]) // Sort by string representation
 	})
 
-	log.Info("Feature Gates Status:")
+	featurePairs := make([]string, 0, len(featureKeys))
 	for _, feature := range featureKeys {
-		log.Info("  ", "feature", string(feature), "enabled", fg.Enabled(feature))
+		featurePairs = append(featurePairs, string(feature), fmt.Sprintf("%v", fg.Enabled(feature)))
 	}
+	log.Info("feature gate status", featurePairs)
 }