Skip to content

Commit

Permalink
feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@emosbaugh
emosbaugh committed Oct 29, 2024
1 parent 6f752f4 commit f04c955
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 96 deletions.
75 changes: 40 additions & 35 deletions pkg/store/kotsstore/downstream_store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/replicatedhq/kots/pkg/kotsutil"
"github.com/replicatedhq/kots/pkg/logger"
"github.com/replicatedhq/kots/pkg/persistence"
"github.com/replicatedhq/kots/pkg/store"
"github.com/replicatedhq/kots/pkg/store/types"
"github.com/replicatedhq/kots/pkg/tasks"
"github.com/replicatedhq/kots/pkg/util"
Expand Down Expand Up @@ -426,7 +427,10 @@ func (s *KOTSStore) GetDownstreamVersions(appID string, clusterID string, downlo
if err := s.AddDownstreamVersionDetails(appID, clusterID, v, false); err != nil {
return nil, errors.Wrap(err, "failed to add details to latest downloaded version")
}
v.IsDeployable, v.NonDeployableCause = isAppVersionDeployable(appID, v, result, license.Spec.IsSemverRequired, nil, nil)
v.IsDeployable, v.NonDeployableCause, err = isAppVersionDeployable(s, appID, v, result, license.Spec.IsSemverRequired)
if err != nil {
return nil, errors.Wrapf(err, "failed to check if version %s is deployable", v.VersionLabel)
}
break
}

Expand Down Expand Up @@ -679,20 +683,10 @@ func (s *KOTSStore) AddDownstreamVersionsDetails(appID string, clusterID string,
}

for _, v := range versions {
var currentECConfig, newECConfig []byte
if util.IsEmbeddedCluster() {
if allVersions.CurrentVersion != nil {
currentECConfig, err = s.getRawEmbeddedClusterConfigForVersion(appID, allVersions.CurrentVersion.Sequence)
if err != nil {
return errors.Wrapf(err, "failed to get embedded cluster config for current version %d", allVersions.CurrentVersion.Sequence)
}
}
newECConfig, err = s.getRawEmbeddedClusterConfigForVersion(appID, v.Sequence)
if err != nil {
return errors.Wrapf(err, "failed to get embedded cluster config for version %d", v.Sequence)
}
v.IsDeployable, v.NonDeployableCause, err = isAppVersionDeployable(s, appID, v, allVersions, license.Spec.IsSemverRequired)
if err != nil {
return errors.Wrapf(err, "failed to check if version %s is deployable", v.VersionLabel)
}
v.IsDeployable, v.NonDeployableCause = isAppVersionDeployable(appID, v, allVersions, license.Spec.IsSemverRequired, currentECConfig, newECConfig)
}
}

Expand Down Expand Up @@ -883,30 +877,31 @@ func isSameUpstreamRelease(v1 *downstreamtypes.DownstreamVersion, v2 *downstream
}

func isAppVersionDeployable(
versionStore store.VersionStore,
appID string, version *downstreamtypes.DownstreamVersion, appVersions *downstreamtypes.DownstreamVersions,
isSemverRequired bool, currentECConfig []byte, versionECConfig []byte,
) (bool, string) {
isSemverRequired bool,
) (bool, string, error) {
if version.HasFailingStrictPreflights {
return false, "Deployment is disabled as a strict analyzer in this version's preflight checks has failed or has not been run."
return false, "Deployment is disabled as a strict analyzer in this version's preflight checks has failed or has not been run.", nil
}

if version.Status == types.VersionPendingDownload {
return false, "Version is pending download."
return false, "Version is pending download.", nil
}

if version.Status == types.VersionPendingConfig {
return false, "Version is pending configuration."
return false, "Version is pending configuration.", nil
}

if appVersions.CurrentVersion == nil {
// no version has been deployed yet, treat as an initial install where any version can be deployed at first.
return true, ""
return true, "", nil
}

if version.Sequence == appVersions.CurrentVersion.Sequence {
// version is currently deployed, so previous required versions should've already been deployed.
// also, we shouldn't block re-deploying if a previous release is edited later by the vendor to be required.
return true, ""
return true, "", nil
}

// rollback support is determined across all versions from all channels
Expand Down Expand Up @@ -937,17 +932,27 @@ func isAppVersionDeployable(
continue
}
if v.KOTSKinds == nil || !v.KOTSKinds.KotsApplication.Spec.AllowRollback {
return false, "Rollback is not supported."
return false, "Rollback is not supported.", nil
}
break
}

if util.IsEmbeddedCluster() && currentECConfig != nil {
// Compare the embedded cluster config of the version specified to the currently
// deployed version to check if it has changed. If it has, then we do not allow
// rollbacks.
if !bytes.Equal(currentECConfig, versionECConfig) {
return false, "Rollback is not supported, cluster configuration has changed."
if util.IsEmbeddedCluster() && appVersions.CurrentVersion != nil {
currentECConfig, err := getRawEmbeddedClusterConfigForVersion(versionStore, appID, appVersions.CurrentVersion.Sequence)
if err != nil {
return false, "", errors.Wrapf(err, "failed to get embedded cluster config for current version %d", appVersions.CurrentVersion.Sequence)
}
newECConfig, err := getRawEmbeddedClusterConfigForVersion(versionStore, appID, version.Sequence)
if err != nil {
return false, "", errors.Wrapf(err, "failed to get embedded cluster config for version %d", version.Sequence)
}
if util.IsEmbeddedCluster() && currentECConfig != nil {
// Compare the embedded cluster config of the version specified to the currently
// deployed version to check if it has changed. If it has, then we do not allow
// rollbacks.
if !bytes.Equal(currentECConfig, newECConfig) {
return false, "Rollback is not supported, cluster configuration has changed.", nil
}
}
}
}
Expand Down Expand Up @@ -983,7 +988,7 @@ func isAppVersionDeployable(

if deployedVersionIndex == -1 {
// the deployed version is from a different channel
return true, ""
return true, "", nil
}

// find required versions between the deployed version and the desired version
Expand All @@ -1001,7 +1006,7 @@ ALL_VERSIONS_LOOP:
// this is a past version
// >= because if the deployed version is required, rolling back isn't allowed
if i >= deployedVersionIndex && i < versionIndex {
return false, "One or more non-reversible versions have been deployed since this version."
return false, "One or more non-reversible versions have been deployed since this version.", nil
}
continue
}
Expand Down Expand Up @@ -1029,16 +1034,16 @@ ALL_VERSIONS_LOOP:
}
versionLabelsStr := strings.Join(versionLabels, ", ")
if len(requiredVersions) == 1 {
return false, fmt.Sprintf("This version cannot be deployed because version %s is required and must be deployed first.", versionLabelsStr)
return false, fmt.Sprintf("This version cannot be deployed because version %s is required and must be deployed first.", versionLabelsStr), nil
}
return false, fmt.Sprintf("This version cannot be deployed because versions %s are required and must be deployed first.", versionLabelsStr)
return false, fmt.Sprintf("This version cannot be deployed because versions %s are required and must be deployed first.", versionLabelsStr), nil
}

return true, ""
return true, "", nil
}

func (s *KOTSStore) getRawEmbeddedClusterConfigForVersion(appID string, sequence int64) ([]byte, error) {
currentConf, err := s.GetEmbeddedClusterConfigForVersion(appID, sequence)
func getRawEmbeddedClusterConfigForVersion(versionStore store.VersionStore, appID string, sequence int64) ([]byte, error) {
currentConf, err := versionStore.GetEmbeddedClusterConfigForVersion(appID, sequence)
if err != nil {
return nil, errors.Wrap(err, "failed to get embedded cluster config")
}
Expand Down
87 changes: 39 additions & 48 deletions pkg/store/kotsstore/downstream_store_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
package kotsstore

import (
"encoding/json"
"testing"

"github.com/blang/semver"
"github.com/golang/mock/gomock"
embeddedclusterv1beta1 "github.com/replicatedhq/embedded-cluster/kinds/apis/v1beta1"
downstreamtypes "github.com/replicatedhq/kots/pkg/api/downstream/types"
"github.com/replicatedhq/kots/pkg/cursor"
"github.com/replicatedhq/kots/pkg/kotsutil"
mock_store "github.com/replicatedhq/kots/pkg/store/mock"
"github.com/replicatedhq/kots/pkg/store/types"
kotsv1beta1 "github.com/replicatedhq/kotskinds/apis/kots/v1beta1"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -253,11 +254,10 @@ func Test_isAppVersionDeployable(t *testing.T) {
version *downstreamtypes.DownstreamVersion
appVersions *downstreamtypes.DownstreamVersions
isSemverRequired bool
currentECConfig *embeddedclusterv1beta1.Config
versionECConfig *embeddedclusterv1beta1.Config
setup func(t *testing.T)
setup func(t *testing.T, mockStore *mock_store.MockStore)
expectedIsDeployable bool
expectedCause string
wantErr bool
}{
{
name: "failing strict preflights",
Expand Down Expand Up @@ -3630,8 +3630,19 @@ func Test_isAppVersionDeployable(t *testing.T) {
/* ---- Embedded cluster config tests start here ---- */
{
name: "embedded cluster config change should not allow rollbacks",
setup: func(t *testing.T) {
setup: func(t *testing.T, mockStore *mock_store.MockStore) {
t.Setenv("EMBEDDED_CLUSTER_ID", "1234")

mockStore.EXPECT().GetEmbeddedClusterConfigForVersion("APPID", int64(0)).Return(&embeddedclusterv1beta1.Config{
Spec: embeddedclusterv1beta1.ConfigSpec{
Version: "1.0.0-ec.0",
},
}, nil)
mockStore.EXPECT().GetEmbeddedClusterConfigForVersion("APPID", int64(1)).Return(&embeddedclusterv1beta1.Config{
Spec: embeddedclusterv1beta1.ConfigSpec{
Version: "1.0.0-ec.1",
},
}, nil)
},
version: &downstreamtypes.DownstreamVersion{
VersionLabel: "1.0.0",
Expand Down Expand Up @@ -3664,23 +3675,24 @@ func Test_isAppVersionDeployable(t *testing.T) {
},
},
},
currentECConfig: &embeddedclusterv1beta1.Config{
Spec: embeddedclusterv1beta1.ConfigSpec{
Version: "1.0.0-ec.1",
},
},
versionECConfig: &embeddedclusterv1beta1.Config{
Spec: embeddedclusterv1beta1.ConfigSpec{
Version: "1.0.0-ec.0",
},
},
expectedIsDeployable: false,
expectedCause: "Rollback is not supported, cluster configuration has changed.",
},
{
name: "embedded cluster config no change should allow rollbacks",
setup: func(t *testing.T) {
setup: func(t *testing.T, mockStore *mock_store.MockStore) {
t.Setenv("EMBEDDED_CLUSTER_ID", "1234")

mockStore.EXPECT().GetEmbeddedClusterConfigForVersion("APPID", int64(0)).Return(&embeddedclusterv1beta1.Config{
Spec: embeddedclusterv1beta1.ConfigSpec{
Version: "1.0.0-ec.0",
},
}, nil)
mockStore.EXPECT().GetEmbeddedClusterConfigForVersion("APPID", int64(1)).Return(&embeddedclusterv1beta1.Config{
Spec: embeddedclusterv1beta1.ConfigSpec{
Version: "1.0.0-ec.0",
},
}, nil)
},
version: &downstreamtypes.DownstreamVersion{
VersionLabel: "1.0.0",
Expand Down Expand Up @@ -3713,22 +3725,12 @@ func Test_isAppVersionDeployable(t *testing.T) {
},
},
},
currentECConfig: &embeddedclusterv1beta1.Config{
Spec: embeddedclusterv1beta1.ConfigSpec{
Version: "1.0.0-ec.1",
},
},
versionECConfig: &embeddedclusterv1beta1.Config{
Spec: embeddedclusterv1beta1.ConfigSpec{
Version: "1.0.0-ec.1",
},
},
expectedIsDeployable: true,
expectedCause: "",
},
{
name: "embedded cluster, allowRollback = false should not allow rollbacks",
setup: func(t *testing.T) {
setup: func(t *testing.T, mockStore *mock_store.MockStore) {
t.Setenv("EMBEDDED_CLUSTER_ID", "1234")
},
version: &downstreamtypes.DownstreamVersion{
Expand Down Expand Up @@ -3762,16 +3764,6 @@ func Test_isAppVersionDeployable(t *testing.T) {
},
},
},
currentECConfig: &embeddedclusterv1beta1.Config{
Spec: embeddedclusterv1beta1.ConfigSpec{
Version: "1.0.0-ec.1",
},
},
versionECConfig: &embeddedclusterv1beta1.Config{
Spec: embeddedclusterv1beta1.ConfigSpec{
Version: "1.0.0-ec.0",
},
},
expectedIsDeployable: false,
expectedCause: "Rollback is not supported.",
},
Expand Down Expand Up @@ -3811,21 +3803,20 @@ func Test_isAppVersionDeployable(t *testing.T) {
}
}

var currentECConfig, versionECConfig []byte
if test.currentECConfig != nil {
currentECConfig, err = json.Marshal(test.currentECConfig)
require.NoError(t, err)
}
if test.versionECConfig != nil {
versionECConfig, err = json.Marshal(test.versionECConfig)
require.NoError(t, err)
}
ctrl := gomock.NewController(t)
defer ctrl.Finish()

mockStore := mock_store.NewMockStore(ctrl)
if test.setup != nil {
test.setup(t)
test.setup(t, mockStore)
}

isDeployable, cause := isAppVersionDeployable("APPID", test.version, test.appVersions, test.isSemverRequired, currentECConfig, versionECConfig)
isDeployable, cause, err := isAppVersionDeployable(mockStore, "APPID", test.version, test.appVersions, test.isSemverRequired)
if test.wantErr {
require.Error(t, err)
} else {
require.NoError(t, err)
}
assert.Equal(t, test.expectedIsDeployable, isDeployable)
assert.Equal(t, test.expectedCause, cause)
})
Expand Down
3 changes: 3 additions & 0 deletions pkg/store/kotsstore/kots_store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
kotsadmtypes "github.com/replicatedhq/kots/pkg/kotsadm/types"
"github.com/replicatedhq/kots/pkg/logger"
"github.com/replicatedhq/kots/pkg/persistence"
"github.com/replicatedhq/kots/pkg/store"
"github.com/replicatedhq/kots/pkg/util"
kotsscheme "github.com/replicatedhq/kotskinds/client/kotsclientset/scheme"
troubleshootscheme "github.com/replicatedhq/troubleshoot/pkg/client/troubleshootclientset/scheme"
Expand All @@ -34,6 +35,8 @@ type KOTSStore struct {
}

func init() {
store.SetStore(StoreFromEnv())

kotsscheme.AddToScheme(scheme.Scheme)
veleroscheme.AddToScheme(scheme.Scheme)
troubleshootscheme.AddToScheme(scheme.Scheme)
Expand Down
15 changes: 2 additions & 13 deletions pkg/store/store.go
View file
Original file line number Diff line number Diff line change
@@ -1,38 +1,27 @@
package store

import (
"github.com/replicatedhq/kots/pkg/store/kotsstore"
"github.com/replicatedhq/kots/pkg/util"
)

var (
hasStore = false
globalStore Store
)

var _ Store = (*kotsstore.KOTSStore)(nil)

func GetStore() Store {
if util.IsUpgradeService() {
panic("store cannot not be used in the upgrade service")
}
if !hasStore {
globalStore = storeFromEnv()
hasStore = true
if globalStore == nil {
panic("store not initialized")
}
return globalStore
}

func storeFromEnv() Store {
return kotsstore.StoreFromEnv()
}

func SetStore(s Store) {
if s == nil {
hasStore = false
globalStore = nil
return
}
hasStore = true
globalStore = s
}

0 comments on commit f04c955

Please sign in to comment.