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.

Sign up for GitHub

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

Update --preflight command line option #894

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Update --preflight command line option #894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions pkg/kapp/preflight/registry.go
View file
Open in desktop
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{}{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Non blocking, but using the sets package would make it so this can be a new sets.Set instead of doing this map[string]struct{}{} setup. Then you can do things like sets.Insert(enabled, "key") and enabled.Has("key")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it was a temporary storage, avoid disabling everything, then enabling only things we cared about. We don't actually care about the the value, just which one's have been enabled.
I suppose, if a Set were used, we could set the Set to disabled, then run through the passed in commands, and go through the Set and enable/disable...

There are so many ways to do this!

Copy link
Contributor

Choose a reason for hiding this comment

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

A sets.Set is used exactly for the purpose of only caring about a key without the value. It's a generic map to an empty struct:

type Set[T comparable] map[T]Empty

Again, non-blocking because like you said there are many ways to do this but this is a nice helper for this that is in the kube apimachinery package :)

Copy link
Contributor

Choose a reason for hiding this comment

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

[thought] Would have been our first use of generics 😆 , but nothing to add on here. Sounds good to me either way!

// 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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading