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 5 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
24 changes: 23 additions & 1 deletion cmd/helm/install.go
Expand Up @@ -154,7 +154,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.DryRun, "dry-run", "none", "simulate an install. If --dry-run is set with no option being specified or as 'client', it will not attempt cluster connections. Setting option as '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 @@ -261,6 +262,11 @@ func runInstall(args []string, client *action.Install, valueOpts *values.Options

client.Namespace = settings.Namespace()

// validate dry-run flag value is one of the allowed values
if err := validateDryRunFlag(client.DryRun); 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 @@ -301,3 +307,19 @@ func compInstall(args []string, toComplete string, client *action.Install) ([]st
}
return nil, cobra.ShellCompDirectiveNoFileComp
}

func validateDryRunFlag(dryRunFlagValue string) error {
// validate dry-run flag value with set of allowed value
allowedDryRunValues := []string{"false", "true", "none", "client", "server"}
isAllowed := false
for _, v := range allowedDryRunValues {
if dryRunFlagValue == v {
isAllowed = true
break
}
}
if !isAllowed {
return errors.New("Invalid dry-run flag. Flag must one of the following: false, true, none, client, sever")
}
return nil
}
2 changes: 1 addition & 1 deletion cmd/helm/template.go
Expand Up @@ -73,7 +73,7 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
client.KubeVersion = parsedKubeVersion
}

client.DryRun = true
client.DryRun = "client"
client.ReleaseName = "release-name"
client.Replace = true // Skip the name check
client.ClientOnly = !validate
Expand Down
7 changes: 6 additions & 1 deletion cmd/helm/upgrade.go
Expand Up @@ -139,6 +139,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 := validateDryRunFlag(client.DryRun); err != nil {
return err
}

p := getter.All(settings)
vals, err := valueOpts.MergeValues(p)
Expand Down Expand Up @@ -214,7 +218,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.DryRun, "dry-run", "none", "simulate an install. If --dry-run is set with no option being specified or as 'client', it will not attempt cluster connections. Setting option as '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
14 changes: 7 additions & 7 deletions pkg/action/action.go
Expand Up @@ -102,7 +102,7 @@ type Configuration struct {
// TODO: This function is badly in need of a refactor.
// 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 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, dryRun string) ([]*release.Hook, *bytes.Buffer, string, error) {
hs := []*release.Hook{}
b := bytes.NewBuffer(nil)

Expand All @@ -120,12 +120,12 @@ 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 `--dry-run` with the value of false, none, or sever should try to connect to the cluster.
// This enables the ability to render 'lookup' functions.
// It may break in interesting and exotic ways because other data (e.g. discovery)
// is mocked.
if (dryRun == "server" || dryRun == "none" || dryRun == "false") && cfg.RESTClientGetter != nil {
restConfig, err := cfg.RESTClientGetter.ToRESTConfig()
if err != nil {
return hs, b, "", err
Expand Down
14 changes: 9 additions & 5 deletions pkg/action/install.go
Expand Up @@ -71,7 +71,7 @@ type Install struct {
ClientOnly bool
Force bool
CreateNamespace bool
DryRun bool
DryRun string
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking api change. DryRun needs to remain a bool. Perhaps add a new member, DryRunOption. All the cli flags can work using the new member but the old member would still need to be honored if true as there are other applications out there that use this as a library.

Choose a reason for hiding this comment

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

And what do you think of that ?
#8137 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. Let me rework it with another flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shoot, not quite what I was trying to suggest. I was still suggesting to just have the one single dryrun flag for the cli. That flag would load its input into the new DryRunOption string.

The reason for the new string values is that if a downstream application is using the install action as a library, it's going to have an Install struct with a DryRun boolean already. If we change the type of DryRun, those downstream applications will break.

So my suggestion is that the new string element would be used for the dryrun flag. In places where DryRun is checked, call a function like:

func (i *Install) DryRunCheck() string {
	if i.DryRunOptions != nil {
		return i.DryRunOptions
	}
	if i.DryRun {
		return "client"
	}
	return "none"
}

you could simplify all the choices here and use/return a custom type instead of string, which might make the rest of the code easier:

type DryRunType string

var DryRunServer = DryRunType("server")
var DryRunClient = DryRunType("client")
var DryRunNone = DryRunType("none")
if DryRunCheck() == DryRunServer { ...

The benefit to all this is that the user experience at the command line doesn't have to change, yet we maintain backward compatibility for all the projects that use helm as a go library.

disclaimer: These snippets are used as illustrations. Don't feel like I'm dictating anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you love it when you reply to something before you've scrolled all the way down?

DisableHooks bool
Replace bool
Wait bool
Expand Down Expand Up @@ -128,6 +128,8 @@ type ChartPathOptions struct {
func NewInstall(cfg *Configuration) *Install {
in := &Install{
cfg: cfg,
// Set default value of DryRun for before flags are binded (tests)
DryRun: "none",
}
in.ChartPathOptions.registryClient = cfg.RegistryClient

Expand Down Expand Up @@ -207,7 +209,7 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
// contacts the upstream server and builds the capabilities object.
if crds := chrt.CRDObjects(); !i.ClientOnly && !i.SkipCRDs && len(crds) > 0 {
// On dry run, bail here
if i.DryRun {
if i.DryRun != "none" && i.DryRun != "false" {
i.cfg.Log("WARNING: This chart or one of its subcharts contains CRDs. Rendering may fail or contain inaccuracies.")
} else if err := i.installCRDs(crds); err != nil {
return nil, err
Expand Down Expand Up @@ -241,7 +243,7 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
}

// special case for helm template --is-upgrade
isUpgrade := i.IsUpgrade && i.DryRun
isUpgrade := i.IsUpgrade && (i.DryRun != "none" && i.DryRun != "false")
options := chartutil.ReleaseOptions{
Name: i.ReleaseName,
Namespace: i.Namespace,
Expand All @@ -257,6 +259,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)
// Even for errors, attach this if available
if manifestDoc != nil {
Expand Down Expand Up @@ -298,7 +301,7 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
}

// Bail out here if it is a dry run
if i.DryRun {
if i.DryRun != "none" && i.DryRun != "false" {
rel.Info.Description = "Dry run complete"
return rel, nil
}
Expand Down Expand Up @@ -467,7 +470,8 @@ func (i *Install) availableName() error {
if err := chartutil.ValidateReleaseName(start); err != nil {
return errors.Wrapf(err, "release name %q", start)
}
if i.DryRun {
// On dry run, bail here
if i.DryRun != "none" && i.DryRun != "false" {
return nil
}

Expand Down
8 changes: 4 additions & 4 deletions pkg/action/install_test.go
Expand Up @@ -234,7 +234,7 @@ func TestInstallRelease_WithChartAndDependencyAllNotes(t *testing.T) {
func TestInstallRelease_DryRun(t *testing.T) {
is := assert.New(t)
instAction := installAction(t)
instAction.DryRun = true
instAction.DryRun = "true"
vals := map[string]interface{}{}
res, err := instAction.Run(buildChart(withSampleTemplates()), vals)
if err != nil {
Expand All @@ -254,11 +254,11 @@ 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)
instAction.DryRun = true
instAction.DryRun = "true"
vals := map[string]interface{}{}

mockChart := buildChart(withSampleTemplates())
Expand All @@ -278,7 +278,7 @@ func TestInstallRelease_DryRun_Lookup(t *testing.T) {
func TestInstallReleaseIncorrectTemplate_DryRun(t *testing.T) {
is := assert.New(t)
instAction := installAction(t)
instAction.DryRun = true
instAction.DryRun = "true"
vals := map[string]interface{}{}
_, err := instAction.Run(buildChart(withSampleIncludingIncorrectTemplates()), vals)
expectedErr := "\"hello/templates/incorrect\" at <.Values.bad.doh>: nil pointer evaluating interface {}.doh"
Expand Down
12 changes: 7 additions & 5 deletions pkg/action/upgrade.go
Expand Up @@ -70,8 +70,7 @@ 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
DryRun 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 @@ -114,6 +113,8 @@ type resultMessage struct {
func NewUpgrade(cfg *Configuration) *Upgrade {
up := &Upgrade{
cfg: cfg,
// Set default value of DryRun for before flags are binded (tests)
DryRun: "none",
}
up.ChartPathOptions.registryClient = cfg.RegistryClient

Expand Down Expand Up @@ -152,8 +153,8 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, chart *chart.
if err != nil {
return res, err
}

if !u.DryRun {
// Do not update for dry runs
if u.DryRun == "none" || u.DryRun == "false" {
u.cfg.Log("updating status for upgraded release for %s", name)
if err := u.cfg.Releases.Update(upgradedRelease); err != nil {
return res, err
Expand Down Expand Up @@ -309,7 +310,8 @@ func (u *Upgrade) performUpgrade(ctx context.Context, originalRelease, upgradedR
return nil
})

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