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

feat(helm): add ability for --dry-run to do lookup functions #9426

Merged
merged 14 commits into from Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 26 additions & 1 deletion cmd/helm/install.go
Expand Up @@ -142,6 +142,9 @@ func newInstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
}
client.SetRegistryClient(registryClient)

if client.DryRunOption == "" {
client.DryRunOption = "none"
}
Comment on lines +145 to +147
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is unreachable code. With NoOptDefVal being set you shouldn't run into a value of "" here.

rel, err := runInstall(args, client, valueOpts, out)
if err != nil {
return errors.Wrap(err, "INSTALLATION FAILED")
Expand All @@ -160,7 +163,8 @@ func newInstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {

func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Install, valueOpts *values.Options) {
f.BoolVar(&client.CreateNamespace, "create-namespace", false, "create the release namespace if not present")
f.BoolVar(&client.DryRun, "dry-run", false, "simulate an install")
f.StringVar(&client.DryRunOption, "dry-run", "", "simulate an install. If --dry-run is set with no option being specified or as '--dry-run=client', it will not attempt cluster connections. Setting '--dry-run=server' allows attempting cluster connections.")
f.Lookup("dry-run").NoOptDefVal = "client"
f.BoolVar(&client.Force, "force", false, "force resource updates through a replacement strategy")
f.BoolVar(&client.DisableHooks, "no-hooks", false, "prevent hooks from running during install")
f.BoolVar(&client.Replace, "replace", false, "re-use the given name, only if that name is a deleted release which remains in the history. This is unsafe in production")
Expand Down Expand Up @@ -268,6 +272,11 @@ func runInstall(args []string, client *action.Install, valueOpts *values.Options

client.Namespace = settings.Namespace()

// Validate DryRunOption member is one of the allowed values
if err := validateDryRunOptionFlag(client.DryRunOption); err != nil {
return nil, err
}

// Create context and prepare the handle of SIGTERM
ctx := context.Background()
ctx, cancel := context.WithCancel(ctx)
Expand Down Expand Up @@ -308,3 +317,19 @@ func compInstall(args []string, toComplete string, client *action.Install) ([]st
}
return nil, cobra.ShellCompDirectiveNoFileComp
}

func validateDryRunOptionFlag(dryRunOptionFlagValue string) error {
mattfarina marked this conversation as resolved.
Show resolved Hide resolved
// Validate dry-run flag value with a set of allowed value
allowedDryRunValues := []string{"false", "true", "none", "client", "server"}
isAllowed := false
for _, v := range allowedDryRunValues {
if dryRunOptionFlagValue == v {
isAllowed = true
break
}
}
if !isAllowed {
return errors.New("Invalid dry-run flag. Flag must one of the following: false, true, none, client, server")
}
return nil
}
3 changes: 3 additions & 0 deletions cmd/helm/template.go
Expand Up @@ -79,6 +79,9 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
}
client.SetRegistryClient(registryClient)

if client.DryRunOption == "" {
client.DryRunOption = "true"
}
client.DryRun = true
client.ReleaseName = "release-name"
client.Replace = true // Skip the name check
Expand Down
11 changes: 10 additions & 1 deletion cmd/helm/upgrade.go
Expand Up @@ -96,6 +96,9 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
}
client.SetRegistryClient(registryClient)

if client.DryRunOption == "" {
client.DryRunOption = "none"
}
// Fixes #7002 - Support reading values from STDIN for `upgrade` command
// Must load values AFTER determining if we have to call install so that values loaded from stdin are are not read twice
if client.Install {
Expand All @@ -112,6 +115,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
instClient.ChartPathOptions = client.ChartPathOptions
instClient.Force = client.Force
instClient.DryRun = client.DryRun
instClient.DryRunOption = client.DryRunOption
instClient.DisableHooks = client.DisableHooks
instClient.SkipCRDs = client.SkipCRDs
instClient.Timeout = client.Timeout
Expand Down Expand Up @@ -146,6 +150,10 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
if err != nil {
return err
}
// Validate dry-run flag value is one of the allowed values
if err := validateDryRunOptionFlag(client.DryRunOption); err != nil {
return err
}

p := getter.All(settings)
vals, err := valueOpts.MergeValues(p)
Expand Down Expand Up @@ -221,7 +229,8 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
f.BoolVar(&createNamespace, "create-namespace", false, "if --install is set, create the release namespace if not present")
f.BoolVarP(&client.Install, "install", "i", false, "if a release by this name doesn't already exist, run an install")
f.BoolVar(&client.Devel, "devel", false, "use development versions, too. Equivalent to version '>0.0.0-0'. If --version is set, this is ignored")
f.BoolVar(&client.DryRun, "dry-run", false, "simulate an upgrade")
f.StringVar(&client.DryRunOption, "dry-run", "", "simulate an install. If --dry-run is set with no option being specified or as '--dry-run=client', it will not attempt cluster connections. Setting '--dry-run=server' allows attempting cluster connections.")
f.Lookup("dry-run").NoOptDefVal = "client"
f.BoolVar(&client.Recreate, "recreate-pods", false, "performs pods restart for the resource if applicable")
f.MarkDeprecated("recreate-pods", "functionality will no longer be updated. Consult the documentation for other methods to recreate pods")
f.BoolVar(&client.Force, "force", false, "force resource updates through a replacement strategy")
Expand Down
12 changes: 5 additions & 7 deletions pkg/action/action.go
Expand Up @@ -103,7 +103,7 @@ type Configuration struct {
// TODO: As part of the refactor the duplicate code in cmd/helm/template.go should be removed
//
// This code has to do with writing files to disk.
func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, pr postrender.PostRenderer, dryRun, enableDNS bool) ([]*release.Hook, *bytes.Buffer, string, error) {
func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, pr postrender.PostRenderer, interactWithRemote, enableDNS bool) ([]*release.Hook, *bytes.Buffer, string, error) {
hs := []*release.Hook{}
b := bytes.NewBuffer(nil)

Expand All @@ -121,12 +121,10 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu
var files map[string]string
var err2 error

// A `helm template` or `helm install --dry-run` should not talk to the remote cluster.
// It will break in interesting and exotic ways because other data (e.g. discovery)
// is mocked. It is not up to the template author to decide when the user wants to
// connect to the cluster. So when the user says to dry run, respect the user's
// wishes and do not connect to the cluster.
if !dryRun && cfg.RESTClientGetter != nil {
// A `helm template` should not talk to the remote cluster. However, commands with the flag
//`--dry-run` with the value of `false`, `none`, or `server` should try to interact with the cluster.
// It may break in interesting and exotic ways because other data (e.g. discovery) is mocked.
if interactWithRemote && cfg.RESTClientGetter != nil {
restConfig, err := cfg.RESTClientGetter.ToRESTConfig()
if err != nil {
return hs, b, "", err
Expand Down
14 changes: 13 additions & 1 deletion pkg/action/install.go
Expand Up @@ -73,6 +73,7 @@ type Install struct {
Force bool
CreateNamespace bool
DryRun bool
DryRunOption string
DisableHooks bool
Replace bool
Wait bool
Expand Down Expand Up @@ -227,6 +228,16 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
return nil, err
}

// Determine dry run behavior
if i.DryRun || i.DryRunOption == "client" || i.DryRunOption == "server" || i.DryRunOption == "true" {
i.DryRun = true
}

var interactWithRemote bool
if !i.DryRun || i.DryRunOption == "server" || i.DryRunOption == "none" || i.DryRunOption == "false" {
interactWithRemote = true
}

// Pre-install anything in the crd/ directory. We do this before Helm
// contacts the upstream server and builds the capabilities object.
if crds := chrt.CRDObjects(); !i.ClientOnly && !i.SkipCRDs && len(crds) > 0 {
Expand Down Expand Up @@ -281,7 +292,7 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
rel := i.createRelease(chrt, vals)

var manifestDoc *bytes.Buffer
rel.Hooks, manifestDoc, rel.Info.Notes, err = i.cfg.renderResources(chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.PostRenderer, i.DryRun, i.EnableDNS)
rel.Hooks, manifestDoc, rel.Info.Notes, err = i.cfg.renderResources(chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.PostRenderer, interactWithRemote, i.EnableDNS)
// Even for errors, attach this if available
if manifestDoc != nil {
rel.Manifest = manifestDoc.String()
Expand Down Expand Up @@ -491,6 +502,7 @@ func (i *Install) availableName() error {
if err := chartutil.ValidateReleaseName(start); err != nil {
return errors.Wrapf(err, "release name %q", start)
}
// On dry run, bail here
if i.DryRun {
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/action/install_test.go
Expand Up @@ -254,7 +254,7 @@ func TestInstallRelease_DryRun(t *testing.T) {
is.Equal(res.Info.Description, "Dry run complete")
}

// Regression test for #7955: Lookup must not connect to Kubernetes on a dry-run.
// Regression test for #7955
func TestInstallRelease_DryRun_Lookup(t *testing.T) {
is := assert.New(t)
instAction := installAction(t)
Expand Down
19 changes: 17 additions & 2 deletions pkg/action/upgrade.go
Expand Up @@ -71,8 +71,9 @@ type Upgrade struct {
// DisableHooks disables hook processing if set to true.
DisableHooks bool
// DryRun controls whether the operation is prepared, but not executed.
// If `true`, the upgrade is prepared but not performed.
DryRun bool
// DryRunOption controls whether the operation is prepared, but not executed with options on whether or not to interact with the remote cluster.
DryRunOption string
// Force will, if set to `true`, ignore certain warnings and perform the upgrade anyway.
//
// This should be used with caution.
Expand Down Expand Up @@ -147,6 +148,12 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, chart *chart.
if err := chartutil.ValidateReleaseName(name); err != nil {
return nil, errors.Errorf("release name is invalid: %s", name)
}

// Determine dry run behavior
if u.DryRun || u.DryRunOption == "client" || u.DryRunOption == "server" || u.DryRunOption == "true" {
u.DryRun = true
}

u.cfg.Log("preparing upgrade for %s", name)
currentRelease, upgradedRelease, err := u.prepareUpgrade(name, chart, vals)
if err != nil {
Expand All @@ -161,6 +168,7 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, chart *chart.
return res, err
}

// Do not update for dry runs
if !u.DryRun {
u.cfg.Log("updating status for upgraded release for %s", name)
if err := u.cfg.Releases.Update(upgradedRelease); err != nil {
Expand Down Expand Up @@ -239,7 +247,13 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin
return nil, nil, err
}

hooks, manifestDoc, notesTxt, err := u.cfg.renderResources(chart, valuesToRender, "", "", u.SubNotes, false, false, u.PostRenderer, u.DryRun, u.EnableDNS)
// Determine whether or not to interact with remote
var interactWithRemote bool
if !u.DryRun || u.DryRunOption == "server" {
interactWithRemote = true
}

hooks, manifestDoc, notesTxt, err := u.cfg.renderResources(chart, valuesToRender, "", "", u.SubNotes, false, false, u.PostRenderer, interactWithRemote, u.EnableDNS)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -317,6 +331,7 @@ func (u *Upgrade) performUpgrade(ctx context.Context, originalRelease, upgradedR
return nil
})

// Run if it is a dry run
if u.DryRun {
u.cfg.Log("dry run for %s", upgradedRelease.Name)
if len(u.Description) > 0 {
Expand Down