Skip to content

Commit

Permalink
helm: re-enable timeout flag (#2658)
Browse files Browse the repository at this point in the history 
* Honor (hidden) timeout flag for applying helm charts
* Set only internally used structs to private

---------

Signed-off-by: Daniel Weiße <[email protected]>
  • Loading branch information
@daniel-weisse
daniel-weisse authored Nov 29, 2023
1 parent e06848c commit b3c734b
Show file tree
Hide file tree
Showing 11 changed files with 137 additions and 129 deletions.
20 changes: 10 additions & 10 deletions cli/internal/cmd/apply.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,12 @@ func NewApplyCmd() *cobra.Command {
cmd.Flags().Bool("merge-kubeconfig", false, "merge Constellation kubeconfig file with default kubeconfig file in $HOME/.kube/config")
cmd.Flags().BoolP("yes", "y", false, "run command without further confirmation\n"+
"WARNING: the command might delete or update existing resources without additional checks. Please read the docs.\n")
cmd.Flags().Duration("timeout", 5*time.Minute, "change helm upgrade timeout\n"+
cmd.Flags().Duration("helm-timeout", 10*time.Minute, "change helm install/upgrade timeout\n"+
"Might be useful for slow connections or big clusters.")
cmd.Flags().StringSlice("skip-phases", nil, "comma-separated list of upgrade phases to skip\n"+
fmt.Sprintf("one or multiple of %s", formatSkipPhases()))

must(cmd.Flags().MarkHidden("timeout"))
must(cmd.Flags().MarkHidden("helm-timeout"))

must(cmd.RegisterFlagCompletionFunc("skip-phases", skipPhasesCompletion))
return cmd
Expand All @@ -140,12 +140,12 @@ func NewApplyCmd() *cobra.Command {
// applyFlags defines the flags for the apply command.
type applyFlags struct {
rootFlags
yes bool
conformance bool
mergeConfigs bool
upgradeTimeout time.Duration
helmWaitMode helm.WaitMode
skipPhases skipPhases
yes bool
conformance bool
mergeConfigs bool
helmTimeout time.Duration
helmWaitMode helm.WaitMode
skipPhases skipPhases
}

// parse the apply command flags.
Expand Down Expand Up @@ -174,9 +174,9 @@ func (f *applyFlags) parse(flags *pflag.FlagSet) error {
return fmt.Errorf("getting 'yes' flag: %w", err)
}

f.upgradeTimeout, err = flags.GetDuration("timeout")
f.helmTimeout, err = flags.GetDuration("helm-timeout")
if err != nil {
return fmt.Errorf("getting 'timeout' flag: %w", err)
return fmt.Errorf("getting 'helm-timeout' flag: %w", err)
}

f.conformance, err = flags.GetBool("conformance")
Expand Down
14 changes: 7 additions & 7 deletions cli/internal/cmd/apply_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ func TestParseApplyFlags(t *testing.T) {
"default flags": {
flags: defaultFlags(),
wantFlags: applyFlags{
helmWaitMode: helm.WaitModeAtomic,
upgradeTimeout: 5 * time.Minute,
helmWaitMode: helm.WaitModeAtomic,
helmTimeout: 10 * time.Minute,
},
},
"skip phases": {
Expand All @@ -108,9 +108,9 @@ func TestParseApplyFlags(t *testing.T) {
return flags
}(),
wantFlags: applyFlags{
skipPhases: newPhases(skipHelmPhase, skipK8sPhase),
helmWaitMode: helm.WaitModeAtomic,
upgradeTimeout: 5 * time.Minute,
skipPhases: newPhases(skipHelmPhase, skipK8sPhase),
helmWaitMode: helm.WaitModeAtomic,
helmTimeout: 10 * time.Minute,
},
},
"skip helm wait": {
Expand All @@ -120,8 +120,8 @@ func TestParseApplyFlags(t *testing.T) {
return flags
}(),
wantFlags: applyFlags{
helmWaitMode: helm.WaitModeNone,
upgradeTimeout: 5 * time.Minute,
helmWaitMode: helm.WaitModeNone,
helmTimeout: 10 * time.Minute,
},
},
}
Expand Down
1 change: 1 addition & 0 deletions cli/internal/cmd/applyhelm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ func (a *applyCmd) runHelmApply(
Force: a.flags.force,
Conformance: a.flags.conformance,
HelmWaitMode: a.flags.helmWaitMode,
ApplyTimeout: a.flags.helmTimeout,
AllowDestructive: helm.DenyDestructive,
}
helmApplier, err := a.newHelmClient(constants.AdminConfFilename, a.log)
Expand Down
36 changes: 16 additions & 20 deletions internal/helm/action.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,7 @@ import (
"helm.sh/helm/v3/pkg/cli"
)

const (
// timeout is the maximum time given per helm action.
timeout = 10 * time.Minute
applyRetryInterval = 30 * time.Second
)
const applyRetryInterval = 30 * time.Second

type applyAction interface {
Apply(context.Context) error
Expand All @@ -45,12 +41,12 @@ func newActionConfig(kubeconfig string, logger debugLog) (*action.Configuration,
return actionConfig, nil
}

func newHelmInstallAction(config *action.Configuration, release Release) *action.Install {
func newHelmInstallAction(config *action.Configuration, release release, timeout time.Duration) *action.Install {
action := action.NewInstall(config)
action.Namespace = constants.HelmNamespace
action.Timeout = timeout
action.ReleaseName = release.ReleaseName
setWaitMode(action, release.WaitMode)
action.ReleaseName = release.releaseName
setWaitMode(action, release.waitMode)
return action
}

Expand All @@ -73,7 +69,7 @@ func setWaitMode(a *action.Install, waitMode WaitMode) {
// installAction is an action that installs a helm chart.
type installAction struct {
preInstall func(context.Context) error
release Release
release release
helmAction *action.Install
postInstall func(context.Context) error
log debugLog
Expand Down Expand Up @@ -104,21 +100,21 @@ func (a *installAction) SaveChart(chartsDir string, fileHandler file.Handler) er
}

func (a *installAction) apply(ctx context.Context) error {
_, err := a.helmAction.RunWithContext(ctx, a.release.Chart, a.release.Values)
_, err := a.helmAction.RunWithContext(ctx, a.release.chart, a.release.values)
return err
}

// ReleaseName returns the release name.
func (a *installAction) ReleaseName() string {
return a.release.ReleaseName
return a.release.releaseName
}

// IsAtomic returns true if the action is atomic.
func (a *installAction) IsAtomic() bool {
return a.helmAction.Atomic
}

func newHelmUpgradeAction(config *action.Configuration) *action.Upgrade {
func newHelmUpgradeAction(config *action.Configuration, timeout time.Duration) *action.Upgrade {
action := action.NewUpgrade(config)
action.Namespace = constants.HelmNamespace
action.Timeout = timeout
Expand All @@ -131,7 +127,7 @@ func newHelmUpgradeAction(config *action.Configuration) *action.Upgrade {
type upgradeAction struct {
preUpgrade func(context.Context) error
postUpgrade func(context.Context) error
release Release
release release
helmAction *action.Upgrade
log debugLog
}
Expand Down Expand Up @@ -160,26 +156,26 @@ func (a *upgradeAction) SaveChart(chartsDir string, fileHandler file.Handler) er
}

func (a *upgradeAction) apply(ctx context.Context) error {
_, err := a.helmAction.RunWithContext(ctx, a.release.ReleaseName, a.release.Chart, a.release.Values)
_, err := a.helmAction.RunWithContext(ctx, a.release.releaseName, a.release.chart, a.release.values)
return err
}

// ReleaseName returns the release name.
func (a *upgradeAction) ReleaseName() string {
return a.release.ReleaseName
return a.release.releaseName
}

// IsAtomic returns true if the action is atomic.
func (a *upgradeAction) IsAtomic() bool {
return a.helmAction.Atomic
}

func saveChart(release Release, chartsDir string, fileHandler file.Handler) error {
if err := saveChartToDisk(release.Chart, chartsDir, fileHandler); err != nil {
return fmt.Errorf("saving chart %s to %q: %w", release.ReleaseName, chartsDir, err)
func saveChart(release release, chartsDir string, fileHandler file.Handler) error {
if err := saveChartToDisk(release.chart, chartsDir, fileHandler); err != nil {
return fmt.Errorf("saving chart %s to %q: %w", release.releaseName, chartsDir, err)
}
if err := fileHandler.WriteYAML(filepath.Join(chartsDir, release.Chart.Metadata.Name, "overrides.yaml"), release.Values); err != nil {
return fmt.Errorf("saving override values for chart %s to %q: %w", release.ReleaseName, filepath.Join(chartsDir, release.Chart.Metadata.Name), err)
if err := fileHandler.WriteYAML(filepath.Join(chartsDir, release.chart.Metadata.Name, "overrides.yaml"), release.values); err != nil {
return fmt.Errorf("saving override values for chart %s to %q: %w", release.releaseName, filepath.Join(chartsDir, release.chart.Metadata.Name), err)
}

return nil
Expand Down
55 changes: 30 additions & 25 deletions internal/helm/actionfactory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"errors"
"fmt"
"strings"
"time"

"github.com/edgelesssys/constellation/v2/internal/compatibility"
"github.com/edgelesssys/constellation/v2/internal/semver"
Expand Down Expand Up @@ -45,17 +46,19 @@ func newActionFactory(kubeClient crdClient, lister releaseVersionLister, actionC
}

// GetActions returns a list of actions to apply the given releases.
func (a actionFactory) GetActions(releases []Release, configTargetVersion semver.Semver, force, allowDestructive bool) (actions []applyAction, includesUpgrade bool, err error) {
func (a actionFactory) GetActions(
releases []release, configTargetVersion semver.Semver, force, allowDestructive bool, timeout time.Duration,
) (actions []applyAction, includesUpgrade bool, err error) {
upgradeErrs := []error{}
for _, release := range releases {
err := a.appendNewAction(release, configTargetVersion, force, allowDestructive, &actions)
err := a.appendNewAction(release, configTargetVersion, force, allowDestructive, timeout, &actions)
var invalidUpgrade *compatibility.InvalidUpgradeError
if errors.As(err, &invalidUpgrade) {
upgradeErrs = append(upgradeErrs, err)
continue
}
if err != nil {
return actions, includesUpgrade, fmt.Errorf("creating action for %s: %w", release.ReleaseName, err)
return actions, includesUpgrade, fmt.Errorf("creating action for %s: %w", release.releaseName, err)
}
}
for _, action := range actions {
Expand All @@ -67,41 +70,43 @@ func (a actionFactory) GetActions(releases []Release, configTargetVersion semver
return actions, includesUpgrade, errors.Join(upgradeErrs...)
}

func (a actionFactory) appendNewAction(release Release, configTargetVersion semver.Semver, force, allowDestructive bool, actions *[]applyAction) error {
newVersion, err := semver.New(release.Chart.Metadata.Version)
func (a actionFactory) appendNewAction(
release release, configTargetVersion semver.Semver, force, allowDestructive bool, timeout time.Duration, actions *[]applyAction,
) error {
newVersion, err := semver.New(release.chart.Metadata.Version)
if err != nil {
return fmt.Errorf("parsing chart version: %w", err)
}
cliSupportsConfigVersion := configTargetVersion.Compare(newVersion) != 0

currentVersion, err := a.versionLister.currentVersion(release.ReleaseName)
currentVersion, err := a.versionLister.currentVersion(release.releaseName)
if errors.Is(err, errReleaseNotFound) {
// Don't install a new release if the user's config specifies a different version than the CLI offers.
if !force && isCLIVersionedRelease(release.ReleaseName) && cliSupportsConfigVersion {
if !force && isCLIVersionedRelease(release.releaseName) && cliSupportsConfigVersion {
return compatibility.NewInvalidUpgradeError(
currentVersion.String(),
configTargetVersion.String(),
fmt.Errorf("this CLI only supports installing microservice version %s", newVersion),
)
}

a.log.Debugf("Release %s not found, adding to new releases...", release.ReleaseName)
*actions = append(*actions, a.newInstall(release))
a.log.Debugf("release %s not found, adding to new releases...", release.releaseName)
*actions = append(*actions, a.newInstall(release, timeout))
return nil
}
if err != nil {
return fmt.Errorf("getting version for %s: %w", release.ReleaseName, err)
return fmt.Errorf("getting version for %s: %w", release.releaseName, err)
}
a.log.Debugf("Current %s version: %s", release.ReleaseName, currentVersion)
a.log.Debugf("New %s version: %s", release.ReleaseName, newVersion)
a.log.Debugf("Current %s version: %s", release.releaseName, currentVersion)
a.log.Debugf("New %s version: %s", release.releaseName, newVersion)

if !force {
// For charts we package ourselves, the version is equal to the CLI version (charts are embedded in the binary).
// We need to make sure this matches with the version in a user's config, if an upgrade should be applied.
if isCLIVersionedRelease(release.ReleaseName) {
if isCLIVersionedRelease(release.releaseName) {
// If target version is not a valid upgrade, don't upgrade any charts.
if err := configTargetVersion.IsUpgradeTo(currentVersion); err != nil {
return fmt.Errorf("invalid upgrade for %s: %w", release.ReleaseName, err)
return fmt.Errorf("invalid upgrade for %s: %w", release.releaseName, err)
}
// Target version is newer than current version, so we should perform an upgrade.
// Now make sure the target version is equal to the the CLI version.
Expand All @@ -117,32 +122,32 @@ func (a actionFactory) appendNewAction(release Release, configTargetVersion semv
if err := newVersion.IsUpgradeTo(currentVersion); err != nil {
// TODO(3u13r): Remove when Constellation v2.14 is released.
// We need to ignore that we jump from Cilium v1.12 to v1.15-pre. We have verified that this works.
if !(errors.Is(err, compatibility.ErrMinorDrift) && release.ReleaseName == "cilium") {
return fmt.Errorf("invalid upgrade for %s: %w", release.ReleaseName, err)
if !(errors.Is(err, compatibility.ErrMinorDrift) && release.releaseName == "cilium") {
return fmt.Errorf("invalid upgrade for %s: %w", release.releaseName, err)
}
}
}
}

if !allowDestructive &&
release.ReleaseName == certManagerInfo.releaseName {
release.releaseName == certManagerInfo.releaseName {
return ErrConfirmationMissing
}
a.log.Debugf("Upgrading %s from %s to %s", release.ReleaseName, currentVersion, newVersion)
*actions = append(*actions, a.newUpgrade(release))
a.log.Debugf("Upgrading %s from %s to %s", release.releaseName, currentVersion, newVersion)
*actions = append(*actions, a.newUpgrade(release, timeout))
return nil
}

func (a actionFactory) newInstall(release Release) *installAction {
action := &installAction{helmAction: newHelmInstallAction(a.cfg, release), release: release, log: a.log}
func (a actionFactory) newInstall(release release, timeout time.Duration) *installAction {
action := &installAction{helmAction: newHelmInstallAction(a.cfg, release, timeout), release: release, log: a.log}
return action
}

func (a actionFactory) newUpgrade(release Release) *upgradeAction {
action := &upgradeAction{helmAction: newHelmUpgradeAction(a.cfg), release: release, log: a.log}
if release.ReleaseName == constellationOperatorsInfo.releaseName {
func (a actionFactory) newUpgrade(release release, timeout time.Duration) *upgradeAction {
action := &upgradeAction{helmAction: newHelmUpgradeAction(a.cfg, timeout), release: release, log: a.log}
if release.releaseName == constellationOperatorsInfo.releaseName {
action.preUpgrade = func(ctx context.Context) error {
if err := a.updateCRDs(ctx, release.Chart); err != nil {
if err := a.updateCRDs(ctx, release.chart); err != nil {
return fmt.Errorf("updating operator CRDs: %w", err)
}
return nil
Expand Down
Loading

0 comments on commit b3c734b

Please sign in to comment.