From c38a47a48f8eb67b061678215b4b34780e0bc850 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Mon, 4 Mar 2024 11:44:22 -0500 Subject: [PATCH] Update --preflight command line option Fix #893 The `--preflight` command line options now only specifies those preflight checks that are to be enables. Signed-off-by: Todd Short --- pkg/kapp/preflight/registry.go | 40 +++++++++---------- pkg/kapp/preflight/registry_test.go | 22 +++------- ...t_permission_validation_escalation_test.go | 4 +- ...ssion_validation_failed_escalation_test.go | 4 +- ...ight_permission_validation_missing_test.go | 6 +-- .../preflight_permission_validation_test.go | 6 +-- 6 files changed, 36 insertions(+), 46 deletions(-) diff --git a/pkg/kapp/preflight/registry.go b/pkg/kapp/preflight/registry.go index 607944f8d..f697dc4f9 100644 --- a/pkg/kapp/preflight/registry.go +++ b/pkg/kapp/preflight/registry.go @@ -6,7 +6,6 @@ package preflight import ( "context" "fmt" - "strconv" "strings" "github.com/spf13/pflag" @@ -31,16 +30,18 @@ func NewRegistry(checks map[string]Check) *Registry { } // String returns a string representation of the -// preflight checks. It follows the format: -// CheckName={true||false},... +// enabled preflight checks. It follows the format: +// CheckName,... // This method is needed so Registry implements // the pflag.Value interface func (c *Registry) String() string { - defaults := []string{} + enabled := []string{} for k, v := range c.known { - defaults = append(defaults, fmt.Sprintf("%s=%v", k, v.Enabled())) + if v.Enabled() { + enabled = append(enabled, k) + } } - return strings.Join(defaults, ",") + return strings.Join(enabled, ",") } // Type returns a string representing the type @@ -51,9 +52,10 @@ func (c *Registry) Type() string { } // Set takes in a string in the format of -// CheckName={true||false},... +// CheckName,... // and sets the specified preflight check -// as enabled if true, disabled if false +// as enabled if listed, otherwise, sets as +// disabled if not listed. // Returns an error if there is a problem // parsing the preflight checks func (c *Registry) Set(s string) error { @@ -61,23 +63,21 @@ func (c *Registry) Set(s string) error { return nil } + enabled := map[string]struct{}{} + // enable those specified mappings := strings.Split(s, ",") - for _, mapping := range mappings { - set := strings.SplitN(mapping, "=", 2) - if len(set) != 2 { - return fmt.Errorf("unable to parse check definition %q, too many '='. Must follow the format check={true||false}", mapping) - } - key, value := set[0], set[1] - + for _, key := range mappings { if _, ok := c.known[key]; !ok { return fmt.Errorf("unknown preflight check %q specified", key) } - - enabled, err := strconv.ParseBool(value) - if err != nil { - return fmt.Errorf("unable to parse boolean representation of %q: %w", mapping, err) + c.known[key].SetEnabled(true) + enabled[key] = struct{}{} + } + // disable unspecified validators + for key := range c.known { + if _, ok := enabled[key]; !ok { + c.known[key].SetEnabled(false) } - c.known[key].SetEnabled(enabled) } return nil } diff --git a/pkg/kapp/preflight/registry_test.go b/pkg/kapp/preflight/registry_test.go index 9dc1cf2fb..e42e11996 100644 --- a/pkg/kapp/preflight/registry_test.go +++ b/pkg/kapp/preflight/registry_test.go @@ -20,42 +20,32 @@ func TestRegistrySet(t *testing.T) { }{ { name: "no preflight checks registered, parsing skipped, any value can be provided", - preflights: "someCheck=true", + preflights: "someCheck", registry: &Registry{}, }, { name: "preflight checks registered, invalid check format in flag, error returned", - preflights: "some=check=something=true", + preflights: ",", registry: &Registry{ known: map[string]Check{ - "some": nil, + "some": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph) error { return nil }, true), }, }, shouldErr: true, }, { name: "preflight checks registered, unknown preflight check specified, error returned", - preflights: "nonexistent=true", + preflights: "nonexistent", registry: &Registry{ known: map[string]Check{ - "exists": nil, - }, - }, - shouldErr: true, - }, - { - name: "preflight checks registered, known check specified, non-boolean value provided, error returned", - preflights: "someCheck=enabled", - registry: &Registry{ - known: map[string]Check{ - "someCheck": nil, + "exists": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph) error { return nil }, true), }, }, shouldErr: true, }, { name: "preflight checks registered, valid input, no error returned", - preflights: "someCheck=true", + preflights: "someCheck", registry: &Registry{ known: map[string]Check{ "someCheck": NewCheck(func(_ context.Context, _ *diffgraph.ChangeGraph) error { return nil }, true), diff --git a/test/e2e/preflight_permission_validation_escalation_test.go b/test/e2e/preflight_permission_validation_escalation_test.go index ef34474fd..39292fe11 100644 --- a/test/e2e/preflight_permission_validation_escalation_test.go +++ b/test/e2e/preflight_permission_validation_escalation_test.go @@ -105,7 +105,7 @@ rules: roleResource = strings.ReplaceAll(roleResource, "__test-name__", testName) logger.Section("deploy app with privilege escalation Role", func() { - kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation=true", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, RunOpts{StdinReader: strings.NewReader(roleResource)}) NewPresentClusterResource("role", testName, testName, kubectl) @@ -129,7 +129,7 @@ roleRef: ` bindingResource = strings.ReplaceAll(bindingResource, "__test-name__", testName) logger.Section("deploy app with privilege escalation RoleBinding", func() { - kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation=true", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, RunOpts{StdinReader: strings.NewReader(bindingResource)}) NewPresentClusterResource("rolebinding", testName, testName, kubectl) diff --git a/test/e2e/preflight_permission_validation_failed_escalation_test.go b/test/e2e/preflight_permission_validation_failed_escalation_test.go index 5aaf8640b..3fdc3fc63 100644 --- a/test/e2e/preflight_permission_validation_failed_escalation_test.go +++ b/test/e2e/preflight_permission_validation_failed_escalation_test.go @@ -107,7 +107,7 @@ rules: roleResource = strings.ReplaceAll(roleResource, "__test-name__", testName) logger.Section("attempt to deploy app with privilege escalation Role without privilege escalation permissions", func() { - _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation=true", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, RunOpts{StdinReader: strings.NewReader(roleResource), AllowError: true}) require.Error(t, err) require.Contains(t, err.Error(), "running preflight check \"PermissionValidation\": potential privilege escalation, not permitted to \"create\" rbac.authorization.k8s.io/v1, Kind=Role") @@ -132,7 +132,7 @@ roleRef: ` bindingResource = strings.ReplaceAll(bindingResource, "__test-name__", testName) logger.Section("attempt deploy app with privilege escalation RoleBinding without privilege escalation permissions", func() { - _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation=true", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, RunOpts{StdinReader: strings.NewReader(bindingResource), AllowError: true}) require.Error(t, err) require.Contains(t, err.Error(), "running preflight check \"PermissionValidation\": potential privilege escalation, not permitted to \"create\" rbac.authorization.k8s.io/v1, Kind=RoleBinding") diff --git a/test/e2e/preflight_permission_validation_missing_test.go b/test/e2e/preflight_permission_validation_missing_test.go index bf46b3812..1b4faac1b 100644 --- a/test/e2e/preflight_permission_validation_missing_test.go +++ b/test/e2e/preflight_permission_validation_missing_test.go @@ -109,7 +109,7 @@ spec: ` basicResource = strings.ReplaceAll(basicResource, "__test-name__", testName) logger.Section("attempt to deploy app with a Pod and missing permissions to create Pods", func() { - _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation=true", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, RunOpts{StdinReader: strings.NewReader(basicResource), AllowError: true}) require.Error(t, err) @@ -132,7 +132,7 @@ rules: roleResource = strings.ReplaceAll(roleResource, "__test-name__", testName) logger.Section("attempt to deploy app with a Role and missing permissions to create Roles", func() { - _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation=true", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, RunOpts{StdinReader: strings.NewReader(roleResource), AllowError: true}) require.Error(t, err) @@ -158,7 +158,7 @@ roleRef: ` bindingResource = strings.ReplaceAll(bindingResource, "__test-name__", testName) logger.Section("attempt to deploy app with a RoleBinding and missing permissions to create RoleBindings", func() { - _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation=true", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + _, err := kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, RunOpts{StdinReader: strings.NewReader(bindingResource), AllowError: true}) require.Error(t, err) diff --git a/test/e2e/preflight_permission_validation_test.go b/test/e2e/preflight_permission_validation_test.go index 58535d861..2e06fa5b5 100644 --- a/test/e2e/preflight_permission_validation_test.go +++ b/test/e2e/preflight_permission_validation_test.go @@ -107,7 +107,7 @@ spec: ` basicResource = strings.ReplaceAll(basicResource, "__test-name__", testName) logger.Section("deploy app with Pod with permissions to create Pods", func() { - kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation=true", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, RunOpts{StdinReader: strings.NewReader(basicResource)}) NewPresentClusterResource("pod", testName, testName, kubectl) @@ -128,7 +128,7 @@ rules: roleResource = strings.ReplaceAll(roleResource, "__test-name__", testName) logger.Section("deploy app with Role with permissions to create Roles", func() { - kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation=true", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, RunOpts{StdinReader: strings.NewReader(roleResource)}) NewPresentClusterResource("role", testName, testName, kubectl) @@ -152,7 +152,7 @@ roleRef: ` bindingResource = strings.ReplaceAll(bindingResource, "__test-name__", testName) logger.Section("deploy app with Pod with permissions to create RoleBindings", func() { - kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation=true", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + kapp.RunWithOpts([]string{"deploy", "--preflight=PermissionValidation", "-a", appName, "-f", "-", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, RunOpts{StdinReader: strings.NewReader(roleResource + bindingResource)}) NewPresentClusterResource("rolebinding", testName, testName, kubectl)