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 2 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
6 changes: 4 additions & 2 deletions internal/features/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ 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
// Ex: SomeFeature: {...}
PreflightPermissions: {Default: false, PreRelease: featuregate.Alpha},
Copy link
Contributor

Choose a reason for hiding this comment

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

It is missing LockToDefault: 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.

making it PreAlpha to settle all (most?) concerns

}

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