Skip to content

Commit

Permalink
Update --preflight command line option
Browse files Browse the repository at this point in the history 
Fix carvel-dev#893

The `--preflight` command line options now only specifies those preflight
checks that are to be enables.

Signed-off-by: Todd Short <todd.short@me.com>
  • Loading branch information
@tmshort
tmshort committed Mar 4, 2024
1 parent 63faf8a commit c38a47a
Show file tree
Hide file tree
Showing 6 changed files with 36 additions and 46 deletions.
40 changes: 20 additions & 20 deletions pkg/kapp/preflight/registry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package preflight
import (
"context"
"fmt"
"strconv"
"strings"

"github.com/spf13/pflag"
Expand All @@ -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
Expand All @@ -51,33 +52,32 @@ 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 {
if c.known == nil {
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
}
Expand Down
22 changes: 6 additions & 16 deletions pkg/kapp/preflight/registry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/preflight_permission_validation_escalation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/preflight_permission_validation_failed_escalation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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")
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/preflight_permission_validation_missing_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/preflight_permission_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit c38a47a

Please sign in to comment.