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

Jump to bottom

Handle validation when value for runAsGroup and runAsUser is empty #1747

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Handle validation when value for runAsGroup and runAsUser is empty #1747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48f257d
Handle validation when value for runAsGroup and runAsUser is empty
savitaashture Jul 5, 2024
848ccb0
Make RunAsNonRoot configurable through default configmap
savitaashture Jul 5, 2024
b9b8e10
Refactor to handle conversion at config level
savitaashture Jul 5, 2024
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
1 change: 1 addition & 0 deletions config/config-defaults-triggers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,4 @@ data:
default-service-account: "default"
default-run-as-user: "65532"
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should default set them empty, and if user wants they can set as their desired value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those values were set from starting and now if we remove we might break behavior
IMO its okay to set some default values when user won't set anything

We are setting default values here https://github.com/tektoncd/triggers/pull/1747/files#diff-572521168fb33345a93f07ab72e633d00cccb422144d8a05399a6fa98cb12025R33-R35

In the config-defaults-triggers.yaml is sample example to show how user can set as its under _example: |

default-run-as-group: "65532"
default-run-as-non-root: "true" # allowed values are true and false
41 changes: 33 additions & 8 deletions pkg/apis/config/default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,11 @@ const (
defaultServiceAccountKey = "default-service-account"
defaultRunAsUserKey = "default-run-as-user"
defaultRunAsGroupKey = "default-run-as-group"
defaultRunAsNonRootKey = "default-run-as-non-root"
DefaultServiceAccountValue = "default"
defaultRunAsUserValue = 65532
defaultRunAsGroupValue = 65532
defaultRunAsNonRootValue = true
)

// Defaults holds the default configurations
Expand All @@ -39,6 +41,11 @@ type Defaults struct {
DefaultServiceAccount string
DefaultRunAsUser int64
Copy link
Contributor

Choose a reason for hiding this comment

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

we dont need these default if we want to keep empty value in cm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned here #1747 (comment) imo if user don't set empty then we can set them with default values

It shouldn't be mandatory that user has to set values through CM its completely optional (which is the current behavior )

Only thing is now we are providing way to configure different values by providing those keys in CM or remove default values for those keys if they don't want by setting ""

DefaultRunAsGroup int64
DefaultRunAsNonRoot bool
// These two fields are used to decide whether to configure
// runAsUser and runAsGroup within a Security Context Constraint (SCC).
IsDefaultRunAsUserEmpty bool
IsDefaultRunAsGroupEmpty bool
}

// GetDefaultsConfigName returns the name of the configmap containing all
Expand All @@ -62,7 +69,8 @@ func (cfg *Defaults) Equals(other *Defaults) bool {

return other.DefaultServiceAccount == cfg.DefaultServiceAccount &&
other.DefaultRunAsUser == cfg.DefaultRunAsUser &&
other.DefaultRunAsGroup == cfg.DefaultRunAsGroup
other.DefaultRunAsGroup == cfg.DefaultRunAsGroup &&
other.DefaultRunAsNonRoot == cfg.DefaultRunAsNonRoot
}

// NewDefaultsFromMap returns a Config given a map corresponding to a ConfigMap
Expand All @@ -71,36 +79,53 @@ func NewDefaultsFromMap(cfgMap map[string]string) (*Defaults, error) {
DefaultServiceAccount: DefaultServiceAccountValue,
DefaultRunAsUser: defaultRunAsUserValue,
DefaultRunAsGroup: defaultRunAsGroupValue,
DefaultRunAsNonRoot: defaultRunAsNonRootValue,
}

if defaultServiceAccount, ok := cfgMap[defaultServiceAccountKey]; ok {
tc.DefaultServiceAccount = defaultServiceAccount
}

if defaultRunAsUser, ok := cfgMap[defaultRunAsUserKey]; ok {
khrm marked this conversation as resolved.
Show resolved Hide resolved
if defaultRunAsUser == "" {
tc.DefaultRunAsUser = 0
} else {
if defaultRunAsUser != "" {
runAsUser, err := strconv.ParseInt(defaultRunAsUser, 10, 0)
if err != nil {
return nil, fmt.Errorf("failed parsing runAsUser config %q", defaultRunAsUser)
}
tc.DefaultRunAsUser = runAsUser
} else {
// if runAsUser is "" don't set runAsUser in SCC
tc.IsDefaultRunAsUserEmpty = true
}
}

if defaultRunAsGroup, ok := cfgMap[defaultRunAsGroupKey]; ok {
if defaultRunAsGroup == "" {
tc.DefaultRunAsGroup = 0
} else {
if defaultRunAsGroup != "" {
runAsGroup, err := strconv.ParseInt(defaultRunAsGroup, 10, 0)
if err != nil {
return nil, fmt.Errorf("failed parsing runAsUser config %q", defaultRunAsGroup)
return nil, fmt.Errorf("failed parsing runAsGroup config %q", defaultRunAsGroup)
}
tc.DefaultRunAsGroup = runAsGroup
} else {
// if runAsGroup is "" don't set runAsGroup in SCC
tc.IsDefaultRunAsGroupEmpty = true
}
}

if defaultRunAsNonRoot, ok := cfgMap[defaultRunAsNonRootKey]; ok {
if defaultRunAsNonRoot != "" {
runAsNonRoot, err := strconv.ParseBool(defaultRunAsNonRoot)
if err != nil {
return nil, fmt.Errorf("failed parsing runAsNonRoot config %q", defaultRunAsNonRoot)
}
tc.DefaultRunAsNonRoot = runAsNonRoot
} else {
// if "" value is provided via configmap set back to default value which is true
tc.DefaultRunAsNonRoot = defaultRunAsNonRootValue
}

}

return &tc, nil
}

Expand Down
11 changes: 8 additions & 3 deletions pkg/apis/config/default_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ func TestNewDefaultsFromConfigMap(t *testing.T) {
DefaultServiceAccount: "default",
DefaultRunAsUser: 65532,
DefaultRunAsGroup: 65532,
DefaultRunAsNonRoot: true,
},
fileName: config.GetDefaultsConfigName(),
},
Expand All @@ -58,16 +59,20 @@ func TestNewDefaultsFromEmptyConfigMap(t *testing.T) {
DefaultServiceAccount: "default",
DefaultRunAsUser: 65532,
DefaultRunAsGroup: 65532,
DefaultRunAsNonRoot: true,
}
verifyConfigFileWithExpectedConfig(t, DefaultsConfigEmptyName, expectedConfig)
}

func TestNewDefaultsFromConfigMapWithEmptyVal(t *testing.T) {
DefaultsConfigEmptyVal := "config-defaults-triggers-empty-val"
expectedConfig := &config.Defaults{
DefaultServiceAccount: "default",
DefaultRunAsUser: 0,
DefaultRunAsGroup: 0,
DefaultServiceAccount: "default",
DefaultRunAsUser: 65532,
DefaultRunAsGroup: 65532,
DefaultRunAsNonRoot: true, // when empty value set from configmap we set back to default value for runAsNonRoot
IsDefaultRunAsUserEmpty: true,
IsDefaultRunAsGroupEmpty: true,
}
verifyConfigFileWithExpectedConfig(t, DefaultsConfigEmptyVal, expectedConfig)
}
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/config/testdata/config-defaults-empty.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,4 @@ data:
default-service-accounts: "default"
default-run-as-user: "65532"
default-run-as-group: "65532"
default-run-as-non-root: "false"
1 change: 1 addition & 0 deletions pkg/apis/config/testdata/config-defaults-triggers-empty-val.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ data:
default-service-account: "default"
default-run-as-user: ""
default-run-as-group: ""
default-run-as-non-root: ""
1 change: 1 addition & 0 deletions pkg/apis/config/testdata/config-defaults-triggers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ data:
default-service-account: "default"
default-run-as-user: "65532"
default-run-as-group: "65532"
default-run-as-non-root: "true"
10 changes: 7 additions & 3 deletions pkg/reconciler/eventlistener/resources/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,19 @@ func MakeContainer(el *v1beta1.EventListener, configAcc reconcilersource.ConfigA
Capabilities: &corev1.Capabilities{
Drop: []corev1.Capability{"ALL"},
},
RunAsNonRoot: ptr.Bool(true),
RunAsNonRoot: ptr.Bool(cfg.Defaults.DefaultRunAsNonRoot),
SeccompProfile: &corev1.SeccompProfile{
Type: corev1.SeccompProfileTypeRuntimeDefault,
},
}
}

containerSecurityContext.RunAsUser = ptr.Int64(cfg.Defaults.DefaultRunAsUser)
containerSecurityContext.RunAsGroup = ptr.Int64(cfg.Defaults.DefaultRunAsGroup)
if !cfg.Defaults.IsDefaultRunAsUserEmpty {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have this also behind c.SetSecurityContext

Copy link
Contributor

Choose a reason for hiding this comment

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

and other field like capability, priviledge escalation can be moved out of the flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya make sense

we can do that

containerSecurityContext.RunAsUser = ptr.Int64(cfg.Defaults.DefaultRunAsUser)
}
if !cfg.Defaults.IsDefaultRunAsGroupEmpty {
containerSecurityContext.RunAsGroup = ptr.Int64(cfg.Defaults.DefaultRunAsGroup)
}

container := corev1.Container{
Name: "event-listener",
Expand Down