From 13ee5e565dfc09af7cfd0596a85fa0046ef9801b Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 30 Apr 2024 16:20:58 -0400 Subject: [PATCH 1/3] add ability to configure and run preflight checks during install, upgrade, and uninstall operations Signed-off-by: everettraven --- pkg/client/actionclient.go | 118 ++++++++++++++++++++++++-- pkg/client/actionclient_test.go | 141 +++++++++++++++++++++++++++----- 2 files changed, 233 insertions(+), 26 deletions(-) diff --git a/pkg/client/actionclient.go b/pkg/client/actionclient.go index 4b6c6de0..9f1fe4a0 100644 --- a/pkg/client/actionclient.go +++ b/pkg/client/actionclient.go @@ -58,11 +58,13 @@ type ActionInterface interface { Reconcile(rel *release.Release) error } -type GetOption func(*action.Get) error -type InstallOption func(*action.Install) error -type UpgradeOption func(*action.Upgrade) error -type UninstallOption func(*action.Uninstall) error -type RollbackOption func(*action.Rollback) error +type ( + GetOption func(*action.Get) error + InstallOption func(*action.Install) error + UpgradeOption func(*action.Upgrade) error + UninstallOption func(*action.Uninstall) error + RollbackOption func(*action.Rollback) error +) type ActionClientGetterOption func(*actionClientGetter) error @@ -115,6 +117,13 @@ func AppendPostRenderers(postRendererFns ...PostRendererProvider) ActionClientGe } } +func AppendPreflight(preflights ...Preflight) ActionClientGetterOption { + return func(getter *actionClientGetter) error { + getter.preflights = append(getter.preflights, preflights...) + return nil + } +} + func NewActionClientGetter(acg ActionConfigGetter, opts ...ActionClientGetterOption) (ActionClientGetter, error) { actionClientGetter := &actionClientGetter{acg: acg} for _, opt := range opts { @@ -125,6 +134,40 @@ func NewActionClientGetter(acg ActionConfigGetter, opts ...ActionClientGetterOpt return actionClientGetter, nil } +type Preflight interface { + Run(string, *release.Release) error + Name() string +} + +const ( + PreflightOperationInstall = "install" + PreflightOperationUpgrade = "upgrade" + PreflightOperationUninstall = "uninstall" +) + +type PreflightFunc func(string, *release.Release) error + +func NewPreflightFunc(name string, pf PreflightFunc) Preflight { + return &preflightFuncImpl{ + preflightFunc: pf, + name: name, + } +} + +type preflightFuncImpl struct { + preflightFunc PreflightFunc + name string +} + +func (pfi *preflightFuncImpl) Run(operation string, rel *release.Release) error { + return pfi.preflightFunc(operation, rel) +} + +func (pfi *preflightFuncImpl) Name() string { + return pfi.name +} + + type actionClientGetter struct { acg ActionConfigGetter @@ -136,6 +179,8 @@ type actionClientGetter struct { installFailureUninstallOpts []UninstallOption upgradeFailureRollbackOpts []RollbackOption + preflights []Preflight + postRendererProviders []PostRendererProvider } @@ -150,7 +195,7 @@ func (hcg *actionClientGetter) ActionClientFor(ctx context.Context, obj client.O if err != nil { return nil, err } - var cpr = chainedPostRenderer{} + cpr := chainedPostRenderer{} for _, provider := range hcg.postRendererProviders { cpr = append(cpr, provider(rm, actionConfig.KubeClient, obj)) } @@ -167,6 +212,8 @@ func (hcg *actionClientGetter) ActionClientFor(ctx context.Context, obj client.O defaultUpgradeOpts: append([]UpgradeOption{WithUpgradePostRenderer(cpr)}, hcg.defaultUpgradeOpts...), defaultUninstallOpts: hcg.defaultUninstallOpts, + preflights: hcg.preflights, + installFailureUninstallOpts: hcg.installFailureUninstallOpts, upgradeFailureRollbackOpts: hcg.upgradeFailureRollbackOpts, }, nil @@ -180,6 +227,8 @@ type actionClient struct { defaultUpgradeOpts []UpgradeOption defaultUninstallOpts []UninstallOption + preflights []Preflight + installFailureUninstallOpts []UninstallOption upgradeFailureRollbackOpts []RollbackOption } @@ -205,6 +254,25 @@ func (c *actionClient) Install(name, namespace string, chrt *chart.Chart, vals m } install.ReleaseName = name install.Namespace = namespace + + // TODO: Open Question - should we always run preflight checks if they exist? + if len(c.preflights) > 0 && !install.DryRun { + install.DryRun = true + c.conf.Log("Running preflight checks") + c.conf.Log("Performing install dry run") + rel, err := install.Run(chrt, vals) + if err != nil { + c.conf.Log("Install dry run failed") + return nil, fmt.Errorf("preflight checks were specified, install dry run failed: %w", err) + } + for _, preflight := range c.preflights { + if err := preflight.Run(PreflightOperationInstall, rel); err != nil { + return nil, fmt.Errorf("preflight check %q failed: %w", preflight.Name(), err) + } + } + install.DryRun = false + } + c.conf.Log("Starting install") rel, err := install.Run(chrt, vals) if err != nil { @@ -241,6 +309,25 @@ func (c *actionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m } } upgrade.Namespace = namespace + + // TODO: Open Question - should we always run preflight checks if they exist? + if len(c.preflights) > 0 && !upgrade.DryRun { + upgrade.DryRun = true + c.conf.Log("Running preflight checks") + c.conf.Log("Performing upgrade dry run") + rel, err := upgrade.Run(name, chrt, vals) + if err != nil { + c.conf.Log("Upgrade dry run failed") + return nil, fmt.Errorf("preflight checks were specified, upgrade dry run failed: %w", err) + } + for _, preflight := range c.preflights { + if err := preflight.Run(PreflightOperationUpgrade, rel); err != nil { + return nil, fmt.Errorf("preflight check %q failed: %w", preflight.Name(), err) + } + } + upgrade.DryRun = false + } + rel, err := upgrade.Run(name, chrt, vals) if err != nil { if rel != nil { @@ -286,6 +373,25 @@ func (c *actionClient) uninstall(name string, opts ...UninstallOption) (*release return nil, err } } + + // TODO: Open Question - should we always run preflight checks if they exist? + if len(c.preflights) > 0 && !uninstall.DryRun { + uninstall.DryRun = true + c.conf.Log("Running preflight checks") + c.conf.Log("Performing uninstall dry run") + rel, err := uninstall.Run(name) + if err != nil { + c.conf.Log("Uninstall dry run failed") + return nil, fmt.Errorf("preflight checks were specified, uninstall dry run failed: %w", err) + } + for _, preflight := range c.preflights { + if err := preflight.Run(PreflightOperationUninstall, rel.Release); err != nil { + return nil, fmt.Errorf("preflight check %q failed: %w", preflight.Name(), err) + } + } + uninstall.DryRun = false + } + return uninstall.Run(name) } diff --git a/pkg/client/actionclient_test.go b/pkg/client/actionclient_test.go index f1dd3a2f..c47d1aa2 100644 --- a/pkg/client/actionclient_test.go +++ b/pkg/client/actionclient_test.go @@ -58,9 +58,7 @@ import ( const mockTestDesc = "Test Description" var _ = Describe("ActionClient", func() { - var ( - rm meta.RESTMapper - ) + var rm meta.RESTMapper BeforeEach(func() { var err error @@ -70,7 +68,7 @@ var _ = Describe("ActionClient", func() { rm, err = apiutil.NewDynamicRESTMapper(cfg, httpClient) Expect(err).ToNot(HaveOccurred()) }) - var _ = Describe("NewActionClientGetter", func() { + _ = Describe("NewActionClientGetter", func() { It("should return a valid ActionConfigGetter", func() { actionConfigGetter, err := NewActionConfigGetter(cfg, rm) Expect(err).ShouldNot(HaveOccurred()) @@ -253,7 +251,6 @@ var _ = Describe("ActionClient", func() { Expect(err).ToNot(HaveOccurred()) }) It("should get clients with postrenderers", func() { - acg, err := NewActionClientGetter(actionConfigGetter, AppendPostRenderers(newMockPostRenderer("foo", "bar"))) Expect(err).ToNot(HaveOccurred()) Expect(acg).NotTo(BeNil()) @@ -289,7 +286,7 @@ var _ = Describe("ActionClient", func() { }) }) - var _ = Describe("ActionClientGetterFunc", func() { + _ = Describe("ActionClientGetterFunc", func() { It("implements the ActionClientGetter interface", func() { gvk := schema.GroupVersionKind{Group: "test", Version: "v1alpha1", Kind: "Test"} expectedObj := &unstructured.Unstructured{} @@ -304,7 +301,7 @@ var _ = Describe("ActionClient", func() { }) }) - var _ = Describe("ActionClientFor", func() { + _ = Describe("ActionClientFor", func() { var obj client.Object BeforeEach(func() { obj = testutil.BuildTestCR(gvk) @@ -320,7 +317,7 @@ var _ = Describe("ActionClient", func() { }) }) - var _ = Describe("ActionClient methods", func() { + _ = Describe("ActionClient methods", func() { var ( obj client.Object cl client.Client @@ -357,7 +354,7 @@ var _ = Describe("ActionClient", func() { panic(err) } }) - var _ = Describe("Install", func() { + _ = Describe("Install", func() { It("should succeed", func() { var ( rel *release.Release @@ -389,15 +386,51 @@ var _ = Describe("ActionClient", func() { Expect(r).To(BeNil()) }) }) + When("preflight checks are configured for install operation", func() { + AfterEach(func() { + // Reset preflights + ac.(*actionClient).preflights = []Preflight{} + }) + It("should fail if preflight checks fail during install", func() { + ac.(*actionClient).preflights = []Preflight{ + NewPreflightFunc("installFail", func(_ string, _ *release.Release) error { + return errors.New("expect this error") + }), + } + + r, err := ac.Install(obj.GetName(), obj.GetNamespace(), &chrt, vals) + Expect(err).To(MatchError("preflight check \"installFail\" failed: expect this error")) + Expect(r).To(BeNil()) + }) + It("should succeed if preflight checks pass during install", func() { + var ( + rel *release.Release + err error + ) + ac.(*actionClient).preflights = []Preflight{ + NewPreflightFunc("installPass", func(_ string, _ *release.Release) error { + return nil + }), + } + + By("installing the release with preflights configured", func() { + opt := func(i *action.Install) error { i.Description = mockTestDesc; return nil } + rel, err = ac.Install(obj.GetName(), obj.GetNamespace(), &chrt, vals, opt) + Expect(err).ToNot(HaveOccurred()) + Expect(rel).NotTo(BeNil()) + }) + verifyRelease(cl, obj, rel) + }) + }) }) - var _ = Describe("Upgrade", func() { + _ = Describe("Upgrade", func() { It("should fail", func() { r, err := ac.Upgrade(obj.GetName(), obj.GetNamespace(), &chrt, vals) Expect(err).To(HaveOccurred()) Expect(r).To(BeNil()) }) }) - var _ = Describe("Uninstall", func() { + _ = Describe("Uninstall", func() { It("should fail", func() { resp, err := ac.Uninstall(obj.GetName()) Expect(err).To(HaveOccurred()) @@ -407,9 +440,7 @@ var _ = Describe("ActionClient", func() { }) When("release is installed", func() { - var ( - installedRelease *release.Release - ) + var installedRelease *release.Release BeforeEach(func() { var err error opt := func(i *action.Install) error { i.Description = mockTestDesc; return nil } @@ -426,7 +457,7 @@ var _ = Describe("ActionClient", func() { panic(err) } }) - var _ = Describe("Get", func() { + _ = Describe("Get", func() { var ( rel *release.Release err error @@ -462,14 +493,14 @@ var _ = Describe("ActionClient", func() { }) }) }) - var _ = Describe("Install", func() { + _ = Describe("Install", func() { It("should fail", func() { r, err := ac.Install(obj.GetName(), obj.GetNamespace(), &chrt, vals) Expect(err).To(HaveOccurred()) Expect(r).To(BeNil()) }) }) - var _ = Describe("Upgrade", func() { + _ = Describe("Upgrade", func() { It("should succeed", func() { var ( rel *release.Release @@ -503,8 +534,43 @@ var _ = Describe("ActionClient", func() { Expect(r).To(BeNil()) }) }) + When("preflight checks are configured for upgrade operation", func() { + AfterEach(func() { + ac.(*actionClient).preflights = []Preflight{} + }) + It("should fail if preflight checks fail during upgrade", func() { + ac.(*actionClient).preflights = []Preflight{ + NewPreflightFunc("upgradeFail", func(_ string, _ *release.Release) error { + return errors.New("expect this error") + }), + } + + r, err := ac.Upgrade(obj.GetName(), obj.GetNamespace(), &chrt, vals) + Expect(err).To(MatchError("preflight check \"upgradeFail\" failed: expect this error")) + Expect(r).To(BeNil()) + }) + It("should succeed if preflight checks pass during upgrade", func() { + ac.(*actionClient).preflights = []Preflight{ + NewPreflightFunc("upgradePass", func(_ string, _ *release.Release) error { + return nil + }), + } + + var ( + rel *release.Release + err error + ) + By("upgrading the release with preflights configured", func() { + opt := func(u *action.Upgrade) error { u.Description = mockTestDesc; return nil } + rel, err = ac.Upgrade(obj.GetName(), obj.GetNamespace(), &chrt, vals, opt) + Expect(err).ToNot(HaveOccurred()) + Expect(rel).NotTo(BeNil()) + }) + verifyRelease(cl, obj, rel) + }) + }) }) - var _ = Describe("Uninstall", func() { + _ = Describe("Uninstall", func() { It("should succeed", func() { var ( resp *release.UninstallReleaseResponse @@ -526,8 +592,43 @@ var _ = Describe("ActionClient", func() { Expect(r).To(BeNil()) }) }) + When("preflight checks are configured for uninstall operation", func() { + AfterEach(func() { + ac.(*actionClient).preflights = []Preflight{} + }) + It("should fail if preflight checks fail during uninstall", func() { + ac.(*actionClient).preflights = []Preflight{ + NewPreflightFunc("uninstallFail", func(_ string, _ *release.Release) error { + return errors.New("expect this error") + }), + } + + r, err := ac.Uninstall(obj.GetName()) + Expect(err).To(MatchError("preflight check \"uninstallFail\" failed: expect this error")) + Expect(r).To(BeNil()) + }) + It("should succeed if preflight checks pass during uninstall", func() { + ac.(*actionClient).preflights = []Preflight{ + NewPreflightFunc("uninstallPass", func(_ string, _ *release.Release) error { + return nil + }), + } + + var ( + resp *release.UninstallReleaseResponse + err error + ) + By("uninstalling the release with preflights configured", func() { + opt := func(i *action.Uninstall) error { i.Description = mockTestDesc; return nil } + resp, err = ac.Uninstall(obj.GetName(), opt) + Expect(err).ToNot(HaveOccurred()) + Expect(resp).NotTo(BeNil()) + }) + verifyNoRelease(cl, obj.GetNamespace(), obj.GetName(), resp.Release) + }) + }) }) - var _ = Describe("Reconcile", func() { + _ = Describe("Reconcile", func() { It("should succeed", func() { By("reconciling the release", func() { err := ac.Reconcile(installedRelease) @@ -578,7 +679,7 @@ var _ = Describe("ActionClient", func() { }) }) - var _ = Describe("createPatch", func() { + _ = Describe("createPatch", func() { It("ignores extra fields in custom resource types", func() { o1 := newTestUnstructured([]interface{}{ map[string]interface{}{ From 7bacbdffd099dc55672667e3b7bfc89a12068cc2 Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 30 Apr 2024 16:25:43 -0400 Subject: [PATCH 2/3] formatting Signed-off-by: everettraven --- pkg/client/actionclient_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/client/actionclient_test.go b/pkg/client/actionclient_test.go index c47d1aa2..d9b91a1f 100644 --- a/pkg/client/actionclient_test.go +++ b/pkg/client/actionclient_test.go @@ -387,10 +387,10 @@ var _ = Describe("ActionClient", func() { }) }) When("preflight checks are configured for install operation", func() { - AfterEach(func() { - // Reset preflights - ac.(*actionClient).preflights = []Preflight{} - }) + AfterEach(func() { + // Reset preflights + ac.(*actionClient).preflights = []Preflight{} + }) It("should fail if preflight checks fail during install", func() { ac.(*actionClient).preflights = []Preflight{ NewPreflightFunc("installFail", func(_ string, _ *release.Release) error { @@ -535,9 +535,9 @@ var _ = Describe("ActionClient", func() { }) }) When("preflight checks are configured for upgrade operation", func() { - AfterEach(func() { - ac.(*actionClient).preflights = []Preflight{} - }) + AfterEach(func() { + ac.(*actionClient).preflights = []Preflight{} + }) It("should fail if preflight checks fail during upgrade", func() { ac.(*actionClient).preflights = []Preflight{ NewPreflightFunc("upgradeFail", func(_ string, _ *release.Release) error { @@ -593,9 +593,9 @@ var _ = Describe("ActionClient", func() { }) }) When("preflight checks are configured for uninstall operation", func() { - AfterEach(func() { - ac.(*actionClient).preflights = []Preflight{} - }) + AfterEach(func() { + ac.(*actionClient).preflights = []Preflight{} + }) It("should fail if preflight checks fail during uninstall", func() { ac.(*actionClient).preflights = []Preflight{ NewPreflightFunc("uninstallFail", func(_ string, _ *release.Release) error { From bd9d7f33ff298778f9f55154f283c9960964da80 Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 30 Apr 2024 16:39:11 -0400 Subject: [PATCH 3/3] gofmt Signed-off-by: everettraven --- pkg/client/actionclient.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/client/actionclient.go b/pkg/client/actionclient.go index 9f1fe4a0..df1e77b2 100644 --- a/pkg/client/actionclient.go +++ b/pkg/client/actionclient.go @@ -148,26 +148,25 @@ const ( type PreflightFunc func(string, *release.Release) error func NewPreflightFunc(name string, pf PreflightFunc) Preflight { - return &preflightFuncImpl{ - preflightFunc: pf, - name: name, - } + return &preflightFuncImpl{ + preflightFunc: pf, + name: name, + } } type preflightFuncImpl struct { - preflightFunc PreflightFunc - name string + preflightFunc PreflightFunc + name string } func (pfi *preflightFuncImpl) Run(operation string, rel *release.Release) error { - return pfi.preflightFunc(operation, rel) + return pfi.preflightFunc(operation, rel) } func (pfi *preflightFuncImpl) Name() string { - return pfi.name + return pfi.name } - type actionClientGetter struct { acg ActionConfigGetter