From 87e123e173028bd2d69547386b09ffda996a0394 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Fri, 5 Jul 2024 22:22:14 +0530 Subject: [PATCH] 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, }, }, },