From 48f257d9cc8539c9e05ad51536f403d43c43c96e Mon Sep 17 00:00:00 2001 From: savitaashture Date: Fri, 5 Jul 2024 16:55:23 +0530 Subject: [PATCH 1/3] Handle validation when value for runAsGroup and runAsUser is empty --- pkg/apis/config/default.go | 30 ++----- pkg/apis/config/default_test.go | 12 +-- .../eventlistener/resources/container.go | 21 ++++- .../eventlistener/resources/container_test.go | 5 +- .../eventlistener/resources/custom.go | 5 +- .../eventlistener/resources/deployment.go | 5 +- .../resources/deployment_test.go | 81 ++++++++----------- 7 files changed, 76 insertions(+), 83 deletions(-) diff --git a/pkg/apis/config/default.go b/pkg/apis/config/default.go index 7d9e52997..e4a09be72 100644 --- a/pkg/apis/config/default.go +++ b/pkg/apis/config/default.go @@ -17,9 +17,7 @@ limitations under the License. package config import ( - "fmt" "os" - "strconv" corev1 "k8s.io/api/core/v1" ) @@ -29,16 +27,16 @@ const ( defaultRunAsUserKey = "default-run-as-user" defaultRunAsGroupKey = "default-run-as-group" DefaultServiceAccountValue = "default" - defaultRunAsUserValue = 65532 - defaultRunAsGroupValue = 65532 + defaultRunAsUserValue = "65532" + defaultRunAsGroupValue = "65532" ) // Defaults holds the default configurations // +k8s:deepcopy-gen=true type Defaults struct { DefaultServiceAccount string - DefaultRunAsUser int64 - DefaultRunAsGroup int64 + DefaultRunAsUser string + DefaultRunAsGroup string } // GetDefaultsConfigName returns the name of the configmap containing all @@ -78,27 +76,11 @@ func NewDefaultsFromMap(cfgMap map[string]string) (*Defaults, error) { } if defaultRunAsUser, ok := cfgMap[defaultRunAsUserKey]; ok { - if defaultRunAsUser == "" { - tc.DefaultRunAsUser = 0 - } else { - runAsUser, err := strconv.ParseInt(defaultRunAsUser, 10, 0) - if err != nil { - return nil, fmt.Errorf("failed parsing runAsUser config %q", defaultRunAsUser) - } - tc.DefaultRunAsUser = runAsUser - } + tc.DefaultRunAsUser = defaultRunAsUser } if defaultRunAsGroup, ok := cfgMap[defaultRunAsGroupKey]; ok { - if defaultRunAsGroup == "" { - tc.DefaultRunAsGroup = 0 - } else { - runAsGroup, err := strconv.ParseInt(defaultRunAsGroup, 10, 0) - if err != nil { - return nil, fmt.Errorf("failed parsing runAsUser config %q", defaultRunAsGroup) - } - tc.DefaultRunAsGroup = runAsGroup - } + tc.DefaultRunAsGroup = defaultRunAsGroup } return &tc, nil diff --git a/pkg/apis/config/default_test.go b/pkg/apis/config/default_test.go index 8cd9e4c8d..61d189f92 100644 --- a/pkg/apis/config/default_test.go +++ b/pkg/apis/config/default_test.go @@ -36,8 +36,8 @@ func TestNewDefaultsFromConfigMap(t *testing.T) { { expectedConfig: &config.Defaults{ DefaultServiceAccount: "default", - DefaultRunAsUser: 65532, - DefaultRunAsGroup: 65532, + DefaultRunAsUser: "65532", + DefaultRunAsGroup: "65532", }, fileName: config.GetDefaultsConfigName(), }, @@ -56,8 +56,8 @@ func TestNewDefaultsFromEmptyConfigMap(t *testing.T) { DefaultsConfigEmptyName := "config-defaults-empty" expectedConfig := &config.Defaults{ DefaultServiceAccount: "default", - DefaultRunAsUser: 65532, - DefaultRunAsGroup: 65532, + DefaultRunAsUser: "65532", + DefaultRunAsGroup: "65532", } verifyConfigFileWithExpectedConfig(t, DefaultsConfigEmptyName, expectedConfig) } @@ -66,8 +66,8 @@ func TestNewDefaultsFromConfigMapWithEmptyVal(t *testing.T) { DefaultsConfigEmptyVal := "config-defaults-triggers-empty-val" expectedConfig := &config.Defaults{ DefaultServiceAccount: "default", - DefaultRunAsUser: 0, - DefaultRunAsGroup: 0, + DefaultRunAsUser: "", + DefaultRunAsGroup: "", } verifyConfigFileWithExpectedConfig(t, DefaultsConfigEmptyVal, expectedConfig) } diff --git a/pkg/reconciler/eventlistener/resources/container.go b/pkg/reconciler/eventlistener/resources/container.go index 72ce5bdef..ea35151cb 100644 --- a/pkg/reconciler/eventlistener/resources/container.go +++ b/pkg/reconciler/eventlistener/resources/container.go @@ -17,6 +17,7 @@ limitations under the License. package resources import ( + "errors" "strconv" "github.com/tektoncd/triggers/pkg/apis/config" @@ -29,7 +30,7 @@ import ( type ContainerOption func(*corev1.Container) -func MakeContainer(el *v1beta1.EventListener, configAcc reconcilersource.ConfigAccessor, c Config, cfg *config.Config, opts ...ContainerOption) corev1.Container { +func MakeContainer(el *v1beta1.EventListener, configAcc reconcilersource.ConfigAccessor, c Config, cfg *config.Config, opts ...ContainerOption) (corev1.Container, error) { isMultiNS := false if len(el.Spec.NamespaceSelector.MatchNames) != 0 { isMultiNS = true @@ -64,8 +65,20 @@ func MakeContainer(el *v1beta1.EventListener, configAcc reconcilersource.ConfigA } } - containerSecurityContext.RunAsUser = ptr.Int64(cfg.Defaults.DefaultRunAsUser) - containerSecurityContext.RunAsGroup = ptr.Int64(cfg.Defaults.DefaultRunAsGroup) + if cfg.Defaults.DefaultRunAsUser != "" { + runAsUser, err := strconv.ParseInt(cfg.Defaults.DefaultRunAsUser, 10, 0) + if err != nil { + return corev1.Container{}, errors.New("failed parsing runAsUser config default-run-as-user") + } + containerSecurityContext.RunAsUser = ptr.Int64(runAsUser) + } + if cfg.Defaults.DefaultRunAsGroup != "" { + runAsGroup, err := strconv.ParseInt(cfg.Defaults.DefaultRunAsGroup, 10, 0) + if err != nil { + return corev1.Container{}, errors.New("failed parsing runAsGroup config default-run-as-group") + } + containerSecurityContext.RunAsGroup = ptr.Int64(runAsGroup) + } container := corev1.Container{ Name: "event-listener", @@ -111,5 +124,5 @@ func MakeContainer(el *v1beta1.EventListener, configAcc reconcilersource.ConfigA opt(&container) } - return container + return container, nil } diff --git a/pkg/reconciler/eventlistener/resources/container_test.go b/pkg/reconciler/eventlistener/resources/container_test.go index 3eb0ea1a8..c1e4eaea6 100644 --- a/pkg/reconciler/eventlistener/resources/container_test.go +++ b/pkg/reconciler/eventlistener/resources/container_test.go @@ -483,7 +483,10 @@ func TestContainer(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := MakeContainer(tt.el, &reconcilersource.EmptyVarsGenerator{}, config, cfg.FromContextOrDefaults(context.Background()), tt.opts...) + got, err := MakeContainer(tt.el, &reconcilersource.EmptyVarsGenerator{}, config, cfg.FromContextOrDefaults(context.Background()), tt.opts...) + if err != nil { + t.Error(err) + } if diff := cmp.Diff(tt.want, got); diff != "" { t.Errorf("MakeContainer() did not return expected. -want, +got: %s", diff) } diff --git a/pkg/reconciler/eventlistener/resources/custom.go b/pkg/reconciler/eventlistener/resources/custom.go index d7053f034..8cc6d9239 100644 --- a/pkg/reconciler/eventlistener/resources/custom.go +++ b/pkg/reconciler/eventlistener/resources/custom.go @@ -48,7 +48,7 @@ func MakeCustomObject(ctx context.Context, el *v1beta1.EventListener, configAcc namespace = el.GetNamespace() } - container := MakeContainer(el, configAcc, c, cfg, func(c *corev1.Container) { + container, err := MakeContainer(el, configAcc, c, cfg, func(c *corev1.Container) { // handle env and resources for custom object if len(original.Spec.Template.Spec.Containers) == 1 { c.Env = append(c.Env, original.Spec.Template.Spec.Containers[0].Env...) @@ -77,6 +77,9 @@ func MakeCustomObject(ctx context.Context, el *v1beta1.EventListener, configAcc SuccessThreshold: 1, } }) + if err != nil { + return nil, err + } podlabels := kmeta.UnionMaps(FilterLabels(ctx, el.Labels), GenerateLabels(el.Name, c.StaticResourceLabels)) diff --git a/pkg/reconciler/eventlistener/resources/deployment.go b/pkg/reconciler/eventlistener/resources/deployment.go index dbe89b8db..3b2c983c6 100644 --- a/pkg/reconciler/eventlistener/resources/deployment.go +++ b/pkg/reconciler/eventlistener/resources/deployment.go @@ -47,7 +47,10 @@ func MakeDeployment(ctx context.Context, el *v1beta1.EventListener, configAcc re if err != nil { return nil, err } - container := MakeContainer(el, configAcc, c, cfg, opt, addCertsForSecureConnection(c)) + container, err := MakeContainer(el, configAcc, c, cfg, opt, addCertsForSecureConnection(c)) + if err != nil { + return nil, err + } filteredLabels := FilterLabels(ctx, el.Labels) diff --git a/pkg/reconciler/eventlistener/resources/deployment_test.go b/pkg/reconciler/eventlistener/resources/deployment_test.go index 88760a38c..8c10f8af6 100644 --- a/pkg/reconciler/eventlistener/resources/deployment_test.go +++ b/pkg/reconciler/eventlistener/resources/deployment_test.go @@ -43,6 +43,27 @@ func TestDeployment(t *testing.T) { "eventlistener": eventListenerName, } + cData, err := MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), + addCertsForSecureConnection(config)) + if err != nil { + t.Error(err) + } + + cDataWithTLS, err := MakeContainer(makeEL(withTLSEnvFrom("Bill")), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(withTLSEnvFrom("Bill")), config), + addCertsForSecureConnection(config)) + if err != nil { + t.Error(err) + } + + cDataWithProbes, err := MakeContainer(makeEL(setProbes()), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(setProbes()), config), + addCertsForSecureConnection(config)) + if err != nil { + t.Error(err) + } + tests := []struct { name string el *v1beta1.EventListener @@ -67,12 +88,8 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{ - MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), - addCertsForSecureConnection(config)), - }, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{cData}, + SecurityContext: &strongerSecurityPolicy, }, }, }, @@ -102,12 +119,8 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{ - MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), - addCertsForSecureConnection(config)), - }, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{cData}, + SecurityContext: &strongerSecurityPolicy, }, }, }, @@ -145,12 +158,8 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{ - MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), - addCertsForSecureConnection(config)), - }, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{cData}, + SecurityContext: &strongerSecurityPolicy, Tolerations: []corev1.Toleration{{ Key: "foo", Value: "bar", @@ -191,12 +200,8 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{ - MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), - addCertsForSecureConnection(config)), - }, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{cData}, + SecurityContext: &strongerSecurityPolicy, NodeSelector: map[string]string{ "foo": "bar", }, @@ -234,12 +239,8 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "bob", - Containers: []corev1.Container{ - MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), - addCertsForSecureConnection(config)), - }, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{cData}, + SecurityContext: &strongerSecurityPolicy, }, }, }, @@ -264,11 +265,7 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{ - MakeContainer(makeEL(withTLSEnvFrom("Bill")), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(withTLSEnvFrom("Bill")), config), - addCertsForSecureConnection(config)), - }, + Containers: []corev1.Container{cDataWithTLS}, Volumes: []corev1.Volume{{ Name: "https-connection", VolumeSource: corev1.VolumeSource{ @@ -318,11 +315,7 @@ func TestDeployment(t *testing.T) { TopologySpreadConstraints: []corev1.TopologySpreadConstraint{{ MaxSkew: 1, }}, - Containers: []corev1.Container{ - MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), - addCertsForSecureConnection(config)), - }, + Containers: []corev1.Container{cData}, SecurityContext: &strongerSecurityPolicy, }, }, @@ -348,12 +341,8 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{ - MakeContainer(makeEL(setProbes()), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(setProbes()), config), - addCertsForSecureConnection(config)), - }, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{cDataWithProbes}, + SecurityContext: &strongerSecurityPolicy, }, }, }, From 848ccb06b94069ef7eb181737ebc67568c1f8ceb Mon Sep 17 00:00:00 2001 From: savitaashture Date: Fri, 5 Jul 2024 21:34:04 +0530 Subject: [PATCH 2/3] Make RunAsNonRoot configurable through default configmap --- config/config-defaults-triggers.yaml | 1 + pkg/apis/config/default.go | 23 ++++++++++++++++++- pkg/apis/config/default_test.go | 3 +++ .../testdata/config-defaults-empty.yaml | 1 + .../config-defaults-triggers-empty-val.yaml | 1 + .../testdata/config-defaults-triggers.yaml | 1 + .../eventlistener/resources/container.go | 2 +- 7 files changed, 30 insertions(+), 2 deletions(-) diff --git a/config/config-defaults-triggers.yaml b/config/config-defaults-triggers.yaml index a438cf81f..83ffa17a2 100644 --- a/config/config-defaults-triggers.yaml +++ b/config/config-defaults-triggers.yaml @@ -42,3 +42,4 @@ data: default-service-account: "default" default-run-as-user: "65532" default-run-as-group: "65532" + default-run-as-non-root: "true" # allowed values are true and false diff --git a/pkg/apis/config/default.go b/pkg/apis/config/default.go index e4a09be72..a02484605 100644 --- a/pkg/apis/config/default.go +++ b/pkg/apis/config/default.go @@ -17,7 +17,9 @@ limitations under the License. package config import ( + "fmt" "os" + "strconv" corev1 "k8s.io/api/core/v1" ) @@ -26,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 @@ -37,6 +41,7 @@ type Defaults struct { DefaultServiceAccount string DefaultRunAsUser string DefaultRunAsGroup string + DefaultRunAsNonRoot bool } // GetDefaultsConfigName returns the name of the configmap containing all @@ -60,7 +65,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 @@ -69,6 +75,7 @@ func NewDefaultsFromMap(cfgMap map[string]string) (*Defaults, error) { DefaultServiceAccount: DefaultServiceAccountValue, DefaultRunAsUser: defaultRunAsUserValue, DefaultRunAsGroup: defaultRunAsGroupValue, + DefaultRunAsNonRoot: defaultRunAsNonRootValue, } if defaultServiceAccount, ok := cfgMap[defaultServiceAccountKey]; ok { @@ -83,6 +90,20 @@ func NewDefaultsFromMap(cfgMap map[string]string) (*Defaults, error) { tc.DefaultRunAsGroup = defaultRunAsGroup } + if defaultRunAsNonRoot, ok := cfgMap[defaultRunAsNonRootKey]; ok { + if defaultRunAsNonRoot != "" { + runAsNonRoot, err := strconv.ParseBool(defaultRunAsNonRoot) + if err != nil { + return nil, fmt.Errorf("failed parsing runAsGroup config %v", 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 } diff --git a/pkg/apis/config/default_test.go b/pkg/apis/config/default_test.go index 61d189f92..c31dc3fbc 100644 --- a/pkg/apis/config/default_test.go +++ b/pkg/apis/config/default_test.go @@ -38,6 +38,7 @@ func TestNewDefaultsFromConfigMap(t *testing.T) { DefaultServiceAccount: "default", DefaultRunAsUser: "65532", DefaultRunAsGroup: "65532", + DefaultRunAsNonRoot: true, }, fileName: config.GetDefaultsConfigName(), }, @@ -58,6 +59,7 @@ func TestNewDefaultsFromEmptyConfigMap(t *testing.T) { DefaultServiceAccount: "default", DefaultRunAsUser: "65532", DefaultRunAsGroup: "65532", + DefaultRunAsNonRoot: true, } verifyConfigFileWithExpectedConfig(t, DefaultsConfigEmptyName, expectedConfig) } @@ -68,6 +70,7 @@ func TestNewDefaultsFromConfigMapWithEmptyVal(t *testing.T) { DefaultServiceAccount: "default", DefaultRunAsUser: "", DefaultRunAsGroup: "", + DefaultRunAsNonRoot: true, // when empty value set from configmap we set back to default value for runAsNonRoot } verifyConfigFileWithExpectedConfig(t, DefaultsConfigEmptyVal, expectedConfig) } diff --git a/pkg/apis/config/testdata/config-defaults-empty.yaml b/pkg/apis/config/testdata/config-defaults-empty.yaml index bf7faf39f..ff877c388 100644 --- a/pkg/apis/config/testdata/config-defaults-empty.yaml +++ b/pkg/apis/config/testdata/config-defaults-empty.yaml @@ -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" diff --git a/pkg/apis/config/testdata/config-defaults-triggers-empty-val.yaml b/pkg/apis/config/testdata/config-defaults-triggers-empty-val.yaml index 185e5e0c0..e5aa67a9f 100644 --- a/pkg/apis/config/testdata/config-defaults-triggers-empty-val.yaml +++ b/pkg/apis/config/testdata/config-defaults-triggers-empty-val.yaml @@ -21,3 +21,4 @@ data: default-service-account: "default" default-run-as-user: "" default-run-as-group: "" + default-run-as-non-root: "" diff --git a/pkg/apis/config/testdata/config-defaults-triggers.yaml b/pkg/apis/config/testdata/config-defaults-triggers.yaml index fbe85cd59..1c6334171 100644 --- a/pkg/apis/config/testdata/config-defaults-triggers.yaml +++ b/pkg/apis/config/testdata/config-defaults-triggers.yaml @@ -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" diff --git a/pkg/reconciler/eventlistener/resources/container.go b/pkg/reconciler/eventlistener/resources/container.go index ea35151cb..c82c83874 100644 --- a/pkg/reconciler/eventlistener/resources/container.go +++ b/pkg/reconciler/eventlistener/resources/container.go @@ -58,7 +58,7 @@ 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, }, From b9b8e10e05a0467cbd848d59bacdaff511bf250f Mon Sep 17 00:00:00 2001 From: savitaashture Date: Fri, 5 Jul 2024 22:22:14 +0530 Subject: [PATCH 3/3] Refactor to handle conversion at config level --- pkg/apis/config/default.go | 36 +++++++-- pkg/apis/config/default_test.go | 18 +++-- .../eventlistener/resources/container.go | 21 ++--- .../eventlistener/resources/container_test.go | 5 +- .../eventlistener/resources/custom.go | 5 +- .../eventlistener/resources/deployment.go | 5 +- .../resources/deployment_test.go | 81 +++++++++++-------- 7 files changed, 94 insertions(+), 77 deletions(-) diff --git a/pkg/apis/config/default.go b/pkg/apis/config/default.go index a02484605..21e7c3691 100644 --- a/pkg/apis/config/default.go +++ b/pkg/apis/config/default.go @@ -30,8 +30,8 @@ const ( defaultRunAsGroupKey = "default-run-as-group" defaultRunAsNonRootKey = "default-run-as-non-root" DefaultServiceAccountValue = "default" - defaultRunAsUserValue = "65532" - defaultRunAsGroupValue = "65532" + defaultRunAsUserValue = 65532 + defaultRunAsGroupValue = 65532 defaultRunAsNonRootValue = true ) @@ -39,9 +39,13 @@ const ( // +k8s:deepcopy-gen=true type Defaults struct { DefaultServiceAccount string - DefaultRunAsUser string - DefaultRunAsGroup string + DefaultRunAsUser int64 + 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 @@ -83,18 +87,36 @@ func NewDefaultsFromMap(cfgMap map[string]string) (*Defaults, error) { } if defaultRunAsUser, ok := cfgMap[defaultRunAsUserKey]; ok { - tc.DefaultRunAsUser = defaultRunAsUser + 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 { - tc.DefaultRunAsGroup = defaultRunAsGroup + if defaultRunAsGroup != "" { + runAsGroup, err := strconv.ParseInt(defaultRunAsGroup, 10, 0) + if err != nil { + 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 runAsGroup config %v", defaultRunAsNonRoot) + return nil, fmt.Errorf("failed parsing runAsNonRoot config %q", defaultRunAsNonRoot) } tc.DefaultRunAsNonRoot = runAsNonRoot } else { diff --git a/pkg/apis/config/default_test.go b/pkg/apis/config/default_test.go index c31dc3fbc..ef23dd0d5 100644 --- a/pkg/apis/config/default_test.go +++ b/pkg/apis/config/default_test.go @@ -36,8 +36,8 @@ func TestNewDefaultsFromConfigMap(t *testing.T) { { expectedConfig: &config.Defaults{ DefaultServiceAccount: "default", - DefaultRunAsUser: "65532", - DefaultRunAsGroup: "65532", + DefaultRunAsUser: 65532, + DefaultRunAsGroup: 65532, DefaultRunAsNonRoot: true, }, fileName: config.GetDefaultsConfigName(), @@ -57,8 +57,8 @@ func TestNewDefaultsFromEmptyConfigMap(t *testing.T) { DefaultsConfigEmptyName := "config-defaults-empty" expectedConfig := &config.Defaults{ DefaultServiceAccount: "default", - DefaultRunAsUser: "65532", - DefaultRunAsGroup: "65532", + DefaultRunAsUser: 65532, + DefaultRunAsGroup: 65532, DefaultRunAsNonRoot: true, } verifyConfigFileWithExpectedConfig(t, DefaultsConfigEmptyName, expectedConfig) @@ -67,10 +67,12 @@ func TestNewDefaultsFromEmptyConfigMap(t *testing.T) { func TestNewDefaultsFromConfigMapWithEmptyVal(t *testing.T) { DefaultsConfigEmptyVal := "config-defaults-triggers-empty-val" expectedConfig := &config.Defaults{ - DefaultServiceAccount: "default", - DefaultRunAsUser: "", - DefaultRunAsGroup: "", - DefaultRunAsNonRoot: true, // when empty value set from configmap we set back to default value for runAsNonRoot + 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) } diff --git a/pkg/reconciler/eventlistener/resources/container.go b/pkg/reconciler/eventlistener/resources/container.go index c82c83874..bac047223 100644 --- a/pkg/reconciler/eventlistener/resources/container.go +++ b/pkg/reconciler/eventlistener/resources/container.go @@ -17,7 +17,6 @@ limitations under the License. package resources import ( - "errors" "strconv" "github.com/tektoncd/triggers/pkg/apis/config" @@ -30,7 +29,7 @@ import ( type ContainerOption func(*corev1.Container) -func MakeContainer(el *v1beta1.EventListener, configAcc reconcilersource.ConfigAccessor, c Config, cfg *config.Config, opts ...ContainerOption) (corev1.Container, error) { +func MakeContainer(el *v1beta1.EventListener, configAcc reconcilersource.ConfigAccessor, c Config, cfg *config.Config, opts ...ContainerOption) corev1.Container { isMultiNS := false if len(el.Spec.NamespaceSelector.MatchNames) != 0 { isMultiNS = true @@ -65,19 +64,11 @@ func MakeContainer(el *v1beta1.EventListener, configAcc reconcilersource.ConfigA } } - if cfg.Defaults.DefaultRunAsUser != "" { - runAsUser, err := strconv.ParseInt(cfg.Defaults.DefaultRunAsUser, 10, 0) - if err != nil { - return corev1.Container{}, errors.New("failed parsing runAsUser config default-run-as-user") - } - containerSecurityContext.RunAsUser = ptr.Int64(runAsUser) + if !cfg.Defaults.IsDefaultRunAsUserEmpty { + containerSecurityContext.RunAsUser = ptr.Int64(cfg.Defaults.DefaultRunAsUser) } - if cfg.Defaults.DefaultRunAsGroup != "" { - runAsGroup, err := strconv.ParseInt(cfg.Defaults.DefaultRunAsGroup, 10, 0) - if err != nil { - return corev1.Container{}, errors.New("failed parsing runAsGroup config default-run-as-group") - } - containerSecurityContext.RunAsGroup = ptr.Int64(runAsGroup) + if !cfg.Defaults.IsDefaultRunAsGroupEmpty { + containerSecurityContext.RunAsGroup = ptr.Int64(cfg.Defaults.DefaultRunAsGroup) } container := corev1.Container{ @@ -124,5 +115,5 @@ func MakeContainer(el *v1beta1.EventListener, configAcc reconcilersource.ConfigA opt(&container) } - return container, nil + return container } diff --git a/pkg/reconciler/eventlistener/resources/container_test.go b/pkg/reconciler/eventlistener/resources/container_test.go index c1e4eaea6..3eb0ea1a8 100644 --- a/pkg/reconciler/eventlistener/resources/container_test.go +++ b/pkg/reconciler/eventlistener/resources/container_test.go @@ -483,10 +483,7 @@ func TestContainer(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := MakeContainer(tt.el, &reconcilersource.EmptyVarsGenerator{}, config, cfg.FromContextOrDefaults(context.Background()), tt.opts...) - if err != nil { - t.Error(err) - } + got := MakeContainer(tt.el, &reconcilersource.EmptyVarsGenerator{}, config, cfg.FromContextOrDefaults(context.Background()), tt.opts...) if diff := cmp.Diff(tt.want, got); diff != "" { t.Errorf("MakeContainer() did not return expected. -want, +got: %s", diff) } diff --git a/pkg/reconciler/eventlistener/resources/custom.go b/pkg/reconciler/eventlistener/resources/custom.go index 8cc6d9239..d7053f034 100644 --- a/pkg/reconciler/eventlistener/resources/custom.go +++ b/pkg/reconciler/eventlistener/resources/custom.go @@ -48,7 +48,7 @@ func MakeCustomObject(ctx context.Context, el *v1beta1.EventListener, configAcc namespace = el.GetNamespace() } - container, err := MakeContainer(el, configAcc, c, cfg, func(c *corev1.Container) { + container := MakeContainer(el, configAcc, c, cfg, func(c *corev1.Container) { // handle env and resources for custom object if len(original.Spec.Template.Spec.Containers) == 1 { c.Env = append(c.Env, original.Spec.Template.Spec.Containers[0].Env...) @@ -77,9 +77,6 @@ func MakeCustomObject(ctx context.Context, el *v1beta1.EventListener, configAcc SuccessThreshold: 1, } }) - if err != nil { - return nil, err - } podlabels := kmeta.UnionMaps(FilterLabels(ctx, el.Labels), GenerateLabels(el.Name, c.StaticResourceLabels)) diff --git a/pkg/reconciler/eventlistener/resources/deployment.go b/pkg/reconciler/eventlistener/resources/deployment.go index 3b2c983c6..dbe89b8db 100644 --- a/pkg/reconciler/eventlistener/resources/deployment.go +++ b/pkg/reconciler/eventlistener/resources/deployment.go @@ -47,10 +47,7 @@ func MakeDeployment(ctx context.Context, el *v1beta1.EventListener, configAcc re if err != nil { return nil, err } - container, err := MakeContainer(el, configAcc, c, cfg, opt, addCertsForSecureConnection(c)) - if err != nil { - return nil, err - } + container := MakeContainer(el, configAcc, c, cfg, opt, addCertsForSecureConnection(c)) filteredLabels := FilterLabels(ctx, el.Labels) diff --git a/pkg/reconciler/eventlistener/resources/deployment_test.go b/pkg/reconciler/eventlistener/resources/deployment_test.go index 8c10f8af6..88760a38c 100644 --- a/pkg/reconciler/eventlistener/resources/deployment_test.go +++ b/pkg/reconciler/eventlistener/resources/deployment_test.go @@ -43,27 +43,6 @@ func TestDeployment(t *testing.T) { "eventlistener": eventListenerName, } - cData, err := MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), - addCertsForSecureConnection(config)) - if err != nil { - t.Error(err) - } - - cDataWithTLS, err := MakeContainer(makeEL(withTLSEnvFrom("Bill")), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(withTLSEnvFrom("Bill")), config), - addCertsForSecureConnection(config)) - if err != nil { - t.Error(err) - } - - cDataWithProbes, err := MakeContainer(makeEL(setProbes()), &reconcilersource.EmptyVarsGenerator{}, config, - cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(setProbes()), config), - addCertsForSecureConnection(config)) - if err != nil { - t.Error(err) - } - tests := []struct { name string el *v1beta1.EventListener @@ -88,8 +67,12 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{cData}, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{ + MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), + addCertsForSecureConnection(config)), + }, + SecurityContext: &strongerSecurityPolicy, }, }, }, @@ -119,8 +102,12 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{cData}, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{ + MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), + addCertsForSecureConnection(config)), + }, + SecurityContext: &strongerSecurityPolicy, }, }, }, @@ -158,8 +145,12 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{cData}, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{ + MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), + addCertsForSecureConnection(config)), + }, + SecurityContext: &strongerSecurityPolicy, Tolerations: []corev1.Toleration{{ Key: "foo", Value: "bar", @@ -200,8 +191,12 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{cData}, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{ + MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), + addCertsForSecureConnection(config)), + }, + SecurityContext: &strongerSecurityPolicy, NodeSelector: map[string]string{ "foo": "bar", }, @@ -239,8 +234,12 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "bob", - Containers: []corev1.Container{cData}, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{ + MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), + addCertsForSecureConnection(config)), + }, + SecurityContext: &strongerSecurityPolicy, }, }, }, @@ -265,7 +264,11 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{cDataWithTLS}, + Containers: []corev1.Container{ + MakeContainer(makeEL(withTLSEnvFrom("Bill")), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(withTLSEnvFrom("Bill")), config), + addCertsForSecureConnection(config)), + }, Volumes: []corev1.Volume{{ Name: "https-connection", VolumeSource: corev1.VolumeSource{ @@ -315,7 +318,11 @@ func TestDeployment(t *testing.T) { TopologySpreadConstraints: []corev1.TopologySpreadConstraint{{ MaxSkew: 1, }}, - Containers: []corev1.Container{cData}, + Containers: []corev1.Container{ + MakeContainer(makeEL(), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(), config), + addCertsForSecureConnection(config)), + }, SecurityContext: &strongerSecurityPolicy, }, }, @@ -341,8 +348,12 @@ func TestDeployment(t *testing.T) { }, Spec: corev1.PodSpec{ ServiceAccountName: "sa", - Containers: []corev1.Container{cDataWithProbes}, - SecurityContext: &strongerSecurityPolicy, + Containers: []corev1.Container{ + MakeContainer(makeEL(setProbes()), &reconcilersource.EmptyVarsGenerator{}, config, + cfg.FromContextOrDefaults(context.Background()), mustAddDeployBits(t, makeEL(setProbes()), config), + addCertsForSecureConnection(config)), + }, + SecurityContext: &strongerSecurityPolicy, }, }, },