Skip to content
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.

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

✨ Add feature gate for preflight permissions #1666

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions internal/applier/helm.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
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/features"
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
"github.com/operator-framework/operator-controller/internal/rukpak/convert"
"github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety"
"github.com/operator-framework/operator-controller/internal/rukpak/util"
Expand Down Expand Up @@ -160,6 +161,10 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE
return nil
}, helmclient.AppendInstallPostRenderer(post))
if err != nil {
if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
_ = struct{}{} // minimal no-op to satisfy linter
// probably need to break out this error as it's the one for helm dry-run as opposed to any returned later
}
return nil, nil, StateError, err
}
return nil, desiredRelease, StateNeedsInstall, nil
Expand Down
67 changes: 67 additions & 0 deletions internal/applier/helm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@ import (
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/release"
"helm.sh/helm/v3/pkg/storage/driver"
featuregatetesting "k8s.io/component-base/featuregate/testing"
"sigs.k8s.io/controller-runtime/pkg/client"

helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"

v1 "github.com/operator-framework/operator-controller/api/v1"
"github.com/operator-framework/operator-controller/internal/applier"
"github.com/operator-framework/operator-controller/internal/features"
)

type mockPreflight struct {
Expand Down Expand Up @@ -226,6 +228,71 @@ 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{
getClientErr: driver.ErrReleaseNotFound,
dryRunInstallErr: errors.New("failed attempting to dry-run install chart"),
}
helmApplier := applier.Helm{ActionClientGetter: mockAcg}

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
require.Error(t, err)
require.ErrorContains(t, err, "attempting to dry-run install chart")
require.Nil(t, objs)
require.Empty(t, state)
})

t.Run("fails during pre-flight installation", func(t *testing.T) {
mockAcg := &mockActionGetter{
getClientErr: driver.ErrReleaseNotFound,
installErr: errors.New("failed installing chart"),
}
mockPf := &mockPreflight{installErr: errors.New("failed during install pre-flight check")}
helmApplier := applier.Helm{ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf}}

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
require.Error(t, err)
require.ErrorContains(t, err, "install pre-flight check")
require.Equal(t, applier.StateNeedsInstall, state)
require.Nil(t, objs)
})

t.Run("fails during installation", func(t *testing.T) {
mockAcg := &mockActionGetter{
getClientErr: driver.ErrReleaseNotFound,
installErr: errors.New("failed installing chart"),
}
helmApplier := applier.Helm{ActionClientGetter: mockAcg}

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
require.Error(t, err)
require.ErrorContains(t, err, "installing chart")
require.Equal(t, applier.StateNeedsInstall, state)
require.Nil(t, objs)
})

t.Run("successful installation", 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}

objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels)
require.NoError(t, err)
require.Equal(t, applier.StateNeedsInstall, state)
require.NotNil(t, objs)
assert.Equal(t, "service-a", objs[0].GetName())
assert.Equal(t, "service-b", objs[1].GetName())
})
}

func TestApply_Upgrade(t *testing.T) {
testCurrentRelease := &release.Release{
Info: &release.Info{Status: release.StatusDeployed},
Expand Down
11 changes: 9 additions & 2 deletions internal/features/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,20 @@ import (
)

const (
// Add new feature gates constants (strings)
// Ex: SomeFeature featuregate.Feature = "SomeFeature"
// Add new feature gates constants (strings)
// Ex: SomeFeature featuregate.Feature = "SomeFeature"
PreflightPermissions featuregate.Feature = "PreflightPermissions"
)

var operatorControllerFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
// Add new feature gate definitions
// if you're adding a feature gate for fellow developers to use in ongoing development, use PreAlpha
// Ex: SomeFeature: {...}
PreflightPermissions: {
Default: false,
PreRelease: featuregate.PreAlpha, // keep this PreAlpha until done with feature development
LockToDefault: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we set LockToDefault to true, @ankitathomas reports that you cannot access the feature-gated code for testing by then setting the emulated version. So LockToDefault true would render it untestable.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do not set it true:

},
}

var OperatorControllerFeatureGate featuregate.MutableFeatureGate = featuregate.NewFeatureGate()
Expand Down
Loading