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 13 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 == "unchanged" {
client.DryRunOption = "none"
}
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", "unchanged", "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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The output in the generated help documentation reads...

--dry-run string[="client"]                  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. (default "unchanged")

It includes a ="client" and the default value being unchanged. To an end user trying to read this it may be confusing.

What if the default value was "" instead of "unchanged"? In install, upgrade, and template Helm could check for an empty string to make it's changes. Then the default of unchanged would not be presented to the end user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great call out. I'll change it to "" so end users are not exposed to this internal default value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tapaskapadia I think this is my one nit I would like to see changed before approving.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattfarina I just pushed up the changes to make the default value "" which does make the help documentation much clearer. Here's how the help documentation reads now:

--dry-run string[="client"]                  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.

Here's the expected behavior table (nothing has changed) with my matching test results including the latest changes: test_04-30-23.txt

command dryRun Flag Value expected behavior
install --dry-run dry run w/ no server interaction
install --dry-run=true dry run w/ no server interaction
install --dry-run=client dry run w/ no server interaction
install --dry-run=server dry run with server interaction
install --dry-run=badinput error for invalid option
install --dry-run=none not a dry run
install --dry-run=false not a dry run
install [no flag present] not a dry run
upgrade --install --dry-run dry run w/ no server interaction
upgrade --install --dry-run=true dry run w/ no server interaction
upgrade --install --dry-run=client dry run w/ no server interaction
upgrade --install --dry-run=server dry run with server interaction
upgrade --install --dry-run=badinput error for invalid option
upgrade --install --dry-run=none not a dry run
upgrade --install --dry-run=false not a dry run
upgrade --install [no flag present] not a dry run
template --dry-run dry run w/ no server interaction
template --dry-run=client dry run w/ no server interaction
template --dry-run=server dry run with server interaction
template --dry-run=true dry run w/ no server interaction
template --dry-run=false dry run with server interaction
template --dry-run=badinput error for invalid option
template --dry-run=none dry run with server interaction
template [no flag present] dry run w/ no server interaction

Choose a reason for hiding this comment

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

Is there any further action needed here? I'm happy to help in any way possible.

Choose a reason for hiding this comment

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

@mattfarina @tapaskapadia Are there any additional changes required before this can be merged? It seems like all of the requested changes have been addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philomory I believe I have implemented all the requested changes. If there’s anything else needed, I am happy to make changes.

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 == "unchanged" {
client.DryRunOption = "true"
}
mattfarina marked this conversation as resolved.
Show resolved Hide resolved
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 == "unchanged" {
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", "unchanged", "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