From f249ba34a3b1625f956f052ded2d05e8263cef3c Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 27 Mar 2024 12:01:22 -0400 Subject: [PATCH] Add support for preflight check configuration (#900) This adds configuration file support (`preflightRules`) for preflight checks. Each preflight check must add a `SetConfig()` method to accept configuration from the Preflight registry. Signed-off-by: Todd Short --- pkg/kapp/cmd/app/deploy.go | 4 + pkg/kapp/config/conf.go | 8 + pkg/kapp/config/config.go | 6 + pkg/kapp/permissions/preflight.go | 4 + pkg/kapp/preflight/check.go | 25 ++- pkg/kapp/preflight/registry.go | 66 ++++++-- pkg/kapp/preflight/registry_test.go | 243 +++++++++++++++++++++++++++- 7 files changed, 334 insertions(+), 22 deletions(-) diff --git a/pkg/kapp/cmd/app/deploy.go b/pkg/kapp/cmd/app/deploy.go index db1a430fb..31dd1bd49 100644 --- a/pkg/kapp/cmd/app/deploy.go +++ b/pkg/kapp/cmd/app/deploy.go @@ -200,6 +200,10 @@ func (o *DeployOptions) Run() error { } if o.PreflightChecks != nil { + err = o.PreflightChecks.SetConfig(conf.PreflightRules()) + if err != nil { + return fmt.Errorf("preflight configuration settings failed: %w", err) + } err = o.PreflightChecks.Run(context.Background(), clusterChangesGraph) if err != nil { return fmt.Errorf("preflight checks failed: %w", err) diff --git a/pkg/kapp/config/conf.go b/pkg/kapp/config/conf.go index 2f49969f1..47ba5b21e 100644 --- a/pkg/kapp/config/conf.go +++ b/pkg/kapp/config/conf.go @@ -155,6 +155,14 @@ func (c Conf) TemplateRules() []TemplateRule { return result } +func (c Conf) PreflightRules() []PreflightRule { + var result []PreflightRule + for _, config := range c.configs { + result = append(result, config.PreflightRules...) + } + return result +} + func (c Conf) DiffMaskRules() []DiffMaskRule { var result []DiffMaskRule for _, config := range c.configs { diff --git a/pkg/kapp/config/config.go b/pkg/kapp/config/config.go index 908dc3d78..9762f3da9 100644 --- a/pkg/kapp/config/config.go +++ b/pkg/kapp/config/config.go @@ -30,6 +30,7 @@ type Config struct { LabelScopingRules []LabelScopingRule TemplateRules []TemplateRule DiffMaskRules []DiffMaskRule + PreflightRules []PreflightRule AdditionalLabels map[string]string DiffAgainstLastAppliedFieldExclusionRules []DiffAgainstLastAppliedFieldExclusionRule @@ -145,6 +146,11 @@ type ChangeRuleBinding struct { ResourceMatchers []ResourceMatcher } +type PreflightRule struct { + Name string + Config map[string]any +} + func NewConfigFromResource(res ctlres.Resource) (Config, error) { if res.APIVersion() != configAPIVersion { return Config{}, fmt.Errorf( diff --git a/pkg/kapp/permissions/preflight.go b/pkg/kapp/permissions/preflight.go index c65e33417..d6a790224 100644 --- a/pkg/kapp/permissions/preflight.go +++ b/pkg/kapp/permissions/preflight.go @@ -37,6 +37,10 @@ func (p *Preflight) SetEnabled(enabled bool) { p.enabled = enabled } +func (p *Preflight) SetConfig(_ preflight.CheckConfig) error { + return nil +} + func (p *Preflight) Run(ctx context.Context, changeGraph *ctldgraph.ChangeGraph) error { client, err := p.depsFactory.CoreClient() if err != nil { diff --git a/pkg/kapp/preflight/check.go b/pkg/kapp/preflight/check.go index 353d028db..afe7eb47f 100644 --- a/pkg/kapp/preflight/check.go +++ b/pkg/kapp/preflight/check.go @@ -9,23 +9,30 @@ import ( ctldgraph "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/diffgraph" ) -type CheckFunc func(context.Context, *ctldgraph.ChangeGraph) error +type CheckFunc func(context.Context, *ctldgraph.ChangeGraph, CheckConfig) error +type CheckConfig map[string]any +type ConfigFunc func(CheckConfig) error type Check interface { Enabled() bool SetEnabled(bool) + SetConfig(CheckConfig) error Run(context.Context, *ctldgraph.ChangeGraph) error } type checkImpl struct { enabled bool checkFunc CheckFunc + + config CheckConfig + configFunc ConfigFunc } -func NewCheck(cf CheckFunc, enabled bool) Check { +func NewCheck(cf CheckFunc, sf ConfigFunc, enabled bool) Check { return &checkImpl{ - enabled: enabled, - checkFunc: cf, + enabled: enabled, + checkFunc: cf, + configFunc: sf, } } @@ -37,6 +44,14 @@ func (cf *checkImpl) SetEnabled(enabled bool) { cf.enabled = enabled } +func (cf *checkImpl) SetConfig(config CheckConfig) error { + cf.config = config + if cf.configFunc != nil { + return cf.configFunc(config) + } + return nil +} + func (cf *checkImpl) Run(ctx context.Context, changeGraph *ctldgraph.ChangeGraph) error { - return cf.checkFunc(ctx, changeGraph) + return cf.checkFunc(ctx, changeGraph, cf.config) } diff --git a/pkg/kapp/preflight/registry.go b/pkg/kapp/preflight/registry.go index 714406335..3e38757a5 100644 --- a/pkg/kapp/preflight/registry.go +++ b/pkg/kapp/preflight/registry.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/spf13/pflag" + "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/config" ctldgraph "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/diffgraph" ) @@ -17,6 +18,8 @@ const preflightFlag = "preflight" // Registry is a collection of preflight checks type Registry struct { known map[string]Check + // Stores the enabled values from the command line + enabledFlag map[string]bool } // NewRegistry will return a new *Registry with the @@ -59,25 +62,23 @@ func (c *Registry) Type() string { // Returns an error if there is a problem // parsing the preflight checks func (c *Registry) Set(s string) error { - if c.known == nil { + if c.known == nil || c.enabledFlag == nil { return nil } - enabled := map[string]struct{}{} - // enable those specified + // Using enabledFlag allows multiple --preflight check flags to be specified mappings := strings.Split(s, ",") for _, key := range mappings { if _, ok := c.known[key]; !ok { return fmt.Errorf("unknown preflight check %q specified", key) } - c.known[key].SetEnabled(true) - enabled[key] = struct{}{} + c.enabledFlag[key] = true } - // disable unspecified validators + + // enable/disabled based on validators specified for key := range c.known { - if _, ok := enabled[key]; !ok { - c.known[key].SetEnabled(false) - } + enabled, ok := c.enabledFlag[key] + c.known[key].SetEnabled(ok && enabled) } return nil } @@ -101,9 +102,56 @@ func (c *Registry) AddCheck(name string, check Check) { if c.known == nil { c.known = make(map[string]Check) } + if c.enabledFlag == nil { + c.enabledFlag = make(map[string]bool) + } c.known[name] = check } +// Validate the configuration provided; the rules are: +// 1. Unknown validator = error +// 2. Duplicate validator = error +func (c *Registry) validateConfig(conf []config.PreflightRule) error { + haveConfig := map[string]bool{} + for _, rule := range conf { + if _, ok := c.known[rule.Name]; !ok { + return fmt.Errorf("unknown preflight check in configuration: %q", rule.Name) + } + if _, ok := haveConfig[rule.Name]; ok { + return fmt.Errorf("duplicate preflight check in configuration: %q", rule.Name) + } + haveConfig[rule.Name] = true + } + return nil +} + +func (c *Registry) SetConfig(conf []config.PreflightRule) error { + // We get the --preflight cmdline flag _before_ the configuration from the file. + // So, we need to evaluate the config that we've gotten in light of the enabledFlag + if err := c.validateConfig(conf); err != nil { + return err + } + // map the configuration by name + config := map[string]map[string]any{} + for _, rule := range conf { + config[rule.Name] = rule.Config + } + if len(c.enabledFlag) == 0 { + // no --preflight flag, so enable validators according to their presence in the config + for name, check := range c.known { + _, ok := config[name] + check.SetEnabled(ok) + } + } + for name, check := range c.known { + err := check.SetConfig(config[name]) + if err != nil { + return fmt.Errorf("setting preflight config %q: %w", name, err) + } + } + return nil +} + // Run will execute any enabled preflight checks. The provided // Context and ChangeGraph will be passed to the preflight checks // that are being executed. diff --git a/pkg/kapp/preflight/registry_test.go b/pkg/kapp/preflight/registry_test.go index 2fceb4e5f..8a83cd45b 100644 --- a/pkg/kapp/preflight/registry_test.go +++ b/pkg/kapp/preflight/registry_test.go @@ -8,7 +8,9 @@ import ( "testing" "github.com/stretchr/testify/require" + ctlconf "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/config" "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/diffgraph" + ctlres "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/resources" ) func TestRegistrySet(t *testing.T) { @@ -28,8 +30,11 @@ func TestRegistrySet(t *testing.T) { preflights: ",", registry: &Registry{ known: map[string]Check{ - "some": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph) error { return nil }, true), + "some": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph, _ CheckConfig) error { + return nil + }, nil, true), }, + enabledFlag: map[string]bool{}, }, shouldErr: true, }, @@ -38,8 +43,11 @@ func TestRegistrySet(t *testing.T) { preflights: "nonexistent", registry: &Registry{ known: map[string]Check{ - "exists": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph) error { return nil }, true), + "exists": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph, _ CheckConfig) error { + return nil + }, nil, true), }, + enabledFlag: map[string]bool{}, }, shouldErr: true, }, @@ -48,8 +56,11 @@ func TestRegistrySet(t *testing.T) { preflights: "someCheck", registry: &Registry{ known: map[string]Check{ - "someCheck": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph) error { return nil }, true), + "someCheck": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph, _ CheckConfig) error { + return nil + }, nil, true), }, + enabledFlag: map[string]bool{}, }, }, } @@ -57,7 +68,7 @@ func TestRegistrySet(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { err := tc.registry.Set(tc.preflights) - require.Equal(t, tc.shouldErr, err != nil) + require.Equalf(t, tc.shouldErr, err != nil, "Unexpected error: %v", err) }) } } @@ -76,7 +87,9 @@ func TestRegistryRun(t *testing.T) { name: "preflight checks registered, disabled checks don't run", registry: &Registry{ known: map[string]Check{ - "disabledCheck": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph) error { return errors.New("should be disabled") }, false), + "disabledCheck": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph, _ CheckConfig) error { + return errors.New("should be disabled") + }, nil, false), }, }, }, @@ -84,7 +97,9 @@ func TestRegistryRun(t *testing.T) { name: "preflight checks registered, enabled check returns an error, error returned", registry: &Registry{ known: map[string]Check{ - "errorCheck": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph) error { return errors.New("error") }, true), + "errorCheck": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph, _ CheckConfig) error { + return errors.New("error") + }, nil, true), }, }, shouldErr: true, @@ -93,7 +108,9 @@ func TestRegistryRun(t *testing.T) { name: "preflight checks registered, enabled checks successful, no error returned", registry: &Registry{ known: map[string]Check{ - "someCheck": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph) error { return nil }, true), + "someCheck": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph, _ CheckConfig) error { + return nil + }, nil, true), }, }, }, @@ -102,7 +119,217 @@ func TestRegistryRun(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { err := tc.registry.Run(nil, nil) - require.Equal(t, tc.shouldErr, err != nil) + require.Equalf(t, tc.shouldErr, err != nil, "Unexpected error: %v", err) + }) + } +} + +func TestRegistryConfig(t *testing.T) { + testCases := []struct { + name string + registry *Registry + configYaml string + shouldErr bool + }{ + { + name: "preflight checks registered, config set, no error", + registry: &Registry{ + known: map[string]Check{ + "someCheck": NewCheck( + nil, + func(cfg CheckConfig) error { + if cfg == nil { + return errors.New("config should be present") + } + v, ok := cfg["foo"] + if !ok { + return errors.New("foo config not present") + } + if v != "bar" { + return errors.New("foo should equal 'bar'") + } + _, ok = cfg["foobar"] + if ok { + return errors.New("foobar config should not present") + } + return nil + }, + true, + ), + }, + }, + configYaml: `--- +apiVersion: kapp.k14s.io/v1alpha1 +kind: Config +preflightRules: +- name: someCheck + config: + foo: bar +`, + }, + { + name: "preflight checks registered, unexpected preflight check set, error", + registry: &Registry{ + known: map[string]Check{ + "someCheck": NewCheck( + nil, + func(cfg CheckConfig) error { + if cfg != nil { + return errors.New("config should not be present") + } + return nil + }, + true, + ), + }, + }, + configYaml: `--- +apiVersion: kapp.k14s.io/v1alpha1 +kind: Config +preflightRules: +- name: otherCheck + config: + foo: bar +`, + shouldErr: true, + }, + { + name: "preflight checks registered, duplicate config entry, error", + registry: &Registry{ + known: map[string]Check{ + "someCheck": NewCheck(nil, nil, true), + }, + }, + configYaml: `--- +apiVersion: kapp.k14s.io/v1alpha1 +kind: Config +preflightRules: +- name: someCheck + config: + foo: bar +- name: someCheck + config: + bar: foo +`, + shouldErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + configRs, err := ctlres.NewFileResource(ctlres.NewBytesSource([]byte(tc.configYaml))).Resources() + require.NoErrorf(t, err, "Parsing resources") + + _, conf, err := ctlconf.NewConfFromResources(configRs) + require.NoErrorf(t, err, "Parsing config") + + err = tc.registry.SetConfig(conf.PreflightRules()) + require.Equalf(t, tc.shouldErr, err != nil, "Unexpected error: %v", err) + }) + } +} + +func TestRegistryEnable(t *testing.T) { + testCases := []struct { + name string + registry *Registry + results map[string]bool + cmdEnable string + configYaml string + }{ + { + name: "preflight enabled via config and command line", + registry: &Registry{ + known: map[string]Check{ + "someCheck": NewCheck(nil, nil, false), + }, + enabledFlag: map[string]bool{}, + }, + results: map[string]bool{ + "someCheck": true, + }, + cmdEnable: "someCheck", + configYaml: `--- +apiVersion: kapp.k14s.io/v1alpha1 +kind: Config +preflightRules: +- name: someCheck +`, + }, + { + name: "preflight enabled via config, no command line", + registry: &Registry{ + known: map[string]Check{ + "someCheck": NewCheck(nil, nil, false), + }, + enabledFlag: map[string]bool{}, + }, + results: map[string]bool{ + "someCheck": true, + }, + configYaml: `--- +apiVersion: kapp.k14s.io/v1alpha1 +kind: Config +preflightRules: +- name: someCheck +`, + }, + { + name: "preflight enabled via command line, no config", + registry: &Registry{ + known: map[string]Check{ + "someCheck": NewCheck(nil, nil, false), + }, + enabledFlag: map[string]bool{}, + }, + results: map[string]bool{ + "someCheck": true, + }, + cmdEnable: "someCheck", + }, + { + name: "preflight enabled via config, disabled via command line", + registry: &Registry{ + known: map[string]Check{ + "someCheck": NewCheck(nil, nil, false), + "otherCheck": NewCheck(nil, nil, false), + }, + enabledFlag: map[string]bool{}, + }, + results: map[string]bool{ + "someCheck": false, + "otherCheck": true, + }, + cmdEnable: "otherCheck", // someCheck not listed, so it's disabled + configYaml: `--- +apiVersion: kapp.k14s.io/v1alpha1 +kind: Config +preflightRules: +- name: someCheck +`, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Enable from commandline first + if tc.cmdEnable != "" { + err := tc.registry.Set(tc.cmdEnable) + require.NoErrorf(t, err, "Unexpected error from Set(): %v", err) + } + // Enable from config file second + if tc.configYaml != "" { + configRs, err := ctlres.NewFileResource(ctlres.NewBytesSource([]byte(tc.configYaml))).Resources() + require.NoErrorf(t, err, "Parsing resources") + _, conf, err := ctlconf.NewConfFromResources(configRs) + require.NoErrorf(t, err, "Parsing config") + err = tc.registry.SetConfig(conf.PreflightRules()) + require.NoErrorf(t, err, "Unexpected error from SetConfig(): %v", err) + } + // Check enable results + for k, v := range tc.results { + require.Equalf(t, v, tc.registry.known[k].Enabled(), "Unexpected enable value") + } }) } }