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

savitaashture · 2024-07-05T11:39:31Z

Changes

As part of 48f257d and add787c

when default-run-as-user and default-run-as-group is empty ("") then don't set container SCC for runAsUser and runAsGroup
because users on platform like OpenShift generally don't set these values and it will be set with default
So added check to handle the same

As part of 848ccb0

Added new field default-run-as-non-root to config-defaults-triggers configmap so that RunAsNonRoot can be now configured through CM

Reason for configuring runAsNonRoot through cm

when runAsUser and runAsGroup set to 0 which is valid value but because of runAsNonRoot: true
getting below error

waiting:
     message: 'container''s runAsUser breaks non-root policy (pod: "el-github-listener-5878566fb4-hqt5r_default(086e7204-f323-499c-90a2-4b7e0e277e2d)",
       container: event-listener)'
     reason: CreateContainerConfigError

Allowing runAsNonRoot to be configured through CM will ease users to set runAsUser and runAsGroup to 0
basically this can be the combination

  securityContext:
   allowPrivilegeEscalation: false
   capabilities:
     drop:
     - ALL
   runAsGroup: 0
   runAsNonRoot: false
   runAsUser: 0
   seccompProfile:
     type: RuntimeDefault

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

* Added new field default-run-as-non-root to config-defaults-triggers configmap so that RunAsNonRoot can be now configured through CM

tekton-robot · 2024-07-05T11:43:00Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	88.9%	94.1%	5.2

tekton-robot · 2024-07-05T12:03:27Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	88.9%	94.1%	5.2
pkg/reconciler/eventlistener/resources/container.go	100.0%	93.1%	-6.9
pkg/reconciler/eventlistener/resources/custom.go	94.1%	93.3%	-0.8
pkg/reconciler/eventlistener/resources/deployment.go	100.0%	98.4%	-1.6

pkg/apis/config/default.go

khrm

I think "0" shouldn't be allowed in both the vanilla k8s and OpenShift.

savitaashture · 2024-07-05T13:52:54Z

I think "0" shouldn't be allowed in both the vanilla k8s and OpenShift.

@khrm actually in doc and all it sayd 0 is valid and its a root
but i tried it on kind and see a below error

      waiting:
        message: 'container''s runAsUser breaks non-root policy (pod: "el-github-listener-5878566fb4-hqt5r_default(086e7204-f323-499c-90a2-4b7e0e277e2d)",
          container: event-listener)'
        reason: CreateContainerConfigError

It might be because we have

    securityContext:
      allowPrivilegeEscalation: false
      capabilities:
        drop:
        - ALL
      runAsGroup: 0
      runAsNonRoot: true
      runAsUser: 0
      seccompProfile:
        type: RuntimeDefault

runAsNonRoot: true and setting 0 🤔

Alright i did verify
runAsUser: 0 is failing because runAsNonRoot: true but 0 is valid and we cannot stop users to provide those value

khrm · 2024-07-05T14:50:13Z

Do we allow setting runAsNonRoot as false?
If yes, then it might be a valid value.

savitaashture · 2024-07-05T16:05:09Z

Do we allow setting runAsNonRoot as false? If yes, then it might be a valid value.

Yes make sense
handled here 848ccb0

tekton-robot · 2024-07-05T16:08:21Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	88.9%	91.7%	2.8
pkg/reconciler/eventlistener/resources/container.go	100.0%	93.1%	-6.9
pkg/reconciler/eventlistener/resources/custom.go	94.1%	93.3%	-0.8
pkg/reconciler/eventlistener/resources/deployment.go	100.0%	98.4%	-1.6

savitaashture · 2024-07-05T17:34:46Z

/test

savitaashture · 2024-07-05T17:48:23Z

@dibyom PR is ready to review PTAL thank you

tekton-robot · 2024-07-05T17:49:28Z

@savitaashture: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-tekton-triggers-build-tests
/test pull-tekton-triggers-integration-tests
/test tekton-triggers-unit-tests

The following commands are available to trigger optional jobs:

/test pull-tekton-triggers-go-coverage

Use /test all to run all jobs.

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2024-07-05T17:50:41Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	88.9%	88.2%	-0.7

tekton-robot · 2024-07-05T17:57:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	88.9%	88.2%	-0.7

piyush-garg · 2024-07-05T19:24:35Z

config/config-defaults-triggers.yaml

@@ -42,3 +42,4 @@ data:
    default-service-account: "default"
    default-run-as-user: "65532"


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

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: |

piyush-garg · 2024-07-05T19:26:31Z

pkg/apis/config/default.go

@@ -39,6 +41,11 @@ type Defaults struct {
 	DefaultServiceAccount string
 	DefaultRunAsUser      int64


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

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 ""

khrm

Let's rebase these. @savitaashture We can merge the PR then.

tekton-robot · 2024-07-08T04:31:09Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	88.9%	88.2%	-0.7

savitaashture · 2024-07-08T04:43:55Z

/test tekton-triggers-unit-tests

savitaashture · 2024-07-08T05:24:48Z

/test tekton-triggers-unit-tests

savitaashture · 2024-07-08T05:27:19Z

Let's rebase these. @savitaashture We can merge the PR then.

@khrm Done
Its ready now

khrm

/approve

tekton-robot · 2024-07-08T05:32:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khrm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [khrm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

piyush-garg · 2024-07-08T06:49:53Z

pkg/reconciler/eventlistener/resources/container.go

 			SeccompProfile: &corev1.SeccompProfile{
 				Type: corev1.SeccompProfileTypeRuntimeDefault,
 			},
 		}
 	}

-	containerSecurityContext.RunAsUser = ptr.Int64(cfg.Defaults.DefaultRunAsUser)
-	containerSecurityContext.RunAsGroup = ptr.Int64(cfg.Defaults.DefaultRunAsGroup)
+	if !cfg.Defaults.IsDefaultRunAsUserEmpty {


can we have this also behind c.SetSecurityContext

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

ya make sense

we can do that

vdemeester · 2024-07-08T06:59:24Z

/lgtm

savitaashture requested review from khrm and dibyom July 5, 2024 11:39

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Jul 5, 2024

tekton-robot requested a review from iancoffey July 5, 2024 11:39

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 5, 2024

savitaashture changed the title ~~Handle validation when value for runAsGroup and runAsUser is empty~~ [wip] Handle validation when value for runAsGroup and runAsUser is empty Jul 5, 2024

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2024

Handle validation when value for runAsGroup and runAsUser is empty

48f257d

savitaashture force-pushed the fix_check branch from aeebe93 to 48f257d Compare July 5, 2024 11:59

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 5, 2024

savitaashture changed the title ~~[wip] Handle validation when value for runAsGroup and runAsUser is empty~~ Handle validation when value for runAsGroup and runAsUser is empty Jul 5, 2024

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2024

khrm reviewed Jul 5, 2024

View reviewed changes

pkg/apis/config/default.go Show resolved Hide resolved

khrm reviewed Jul 5, 2024

View reviewed changes

Make RunAsNonRoot configurable through default configmap

848ccb0

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Jul 5, 2024

savitaashture force-pushed the fix_check branch from adb1001 to add787c Compare July 5, 2024 16:57

savitaashture closed this Jul 5, 2024

savitaashture reopened this Jul 5, 2024

Refactor to handle conversion at config level

b9b8e10

savitaashture force-pushed the fix_check branch from add787c to b9b8e10 Compare July 5, 2024 17:40

savitaashture added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 5, 2024

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 5, 2024

piyush-garg reviewed Jul 5, 2024

View reviewed changes

khrm reviewed Jul 8, 2024

View reviewed changes

savitaashture closed this Jul 8, 2024

savitaashture reopened this Jul 8, 2024

khrm reviewed Jul 8, 2024

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2024

savitaashture requested review from vdemeester and removed request for iancoffey July 8, 2024 05:56

piyush-garg reviewed Jul 8, 2024

View reviewed changes

tekton-robot assigned vdemeester Jul 8, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2024

tekton-robot merged commit 87e123e into tektoncd:main Jul 8, 2024
6 checks passed

savitaashture mentioned this pull request Aug 5, 2024

Add runAsUser and runAsGroup under setSecurityContext flag #1756

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

savitaashture commented Jul 5, 2024 •

edited

Loading

tekton-robot commented Jul 5, 2024

tekton-robot commented Jul 5, 2024

khrm left a comment

savitaashture commented Jul 5, 2024 •

edited

Loading

khrm commented Jul 5, 2024

savitaashture commented Jul 5, 2024 •

edited

Loading

tekton-robot commented Jul 5, 2024

savitaashture commented Jul 5, 2024

savitaashture commented Jul 5, 2024 •

edited

Loading

tekton-robot commented Jul 5, 2024

tekton-robot commented Jul 5, 2024

tekton-robot commented Jul 5, 2024

piyush-garg Jul 5, 2024

savitaashture Jul 6, 2024

piyush-garg Jul 5, 2024

savitaashture Jul 6, 2024

khrm left a comment

tekton-robot commented Jul 8, 2024

savitaashture commented Jul 8, 2024

savitaashture commented Jul 8, 2024

savitaashture commented Jul 8, 2024

khrm left a comment

tekton-robot commented Jul 8, 2024

piyush-garg Jul 8, 2024

piyush-garg Jul 8, 2024

savitaashture Jul 8, 2024

vdemeester commented Jul 8, 2024

		@@ -42,3 +42,4 @@ data:
		default-service-account: "default"
		default-run-as-user: "65532"

		@@ -39,6 +41,11 @@ type Defaults struct {
		DefaultServiceAccount string
		DefaultRunAsUser int64

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

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

Conversation

savitaashture commented Jul 5, 2024 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 5, 2024

tekton-robot commented Jul 5, 2024

khrm left a comment

Choose a reason for hiding this comment

savitaashture commented Jul 5, 2024 • edited Loading

khrm commented Jul 5, 2024

savitaashture commented Jul 5, 2024 • edited Loading

tekton-robot commented Jul 5, 2024

savitaashture commented Jul 5, 2024

savitaashture commented Jul 5, 2024 • edited Loading

tekton-robot commented Jul 5, 2024

tekton-robot commented Jul 5, 2024

tekton-robot commented Jul 5, 2024

piyush-garg Jul 5, 2024

Choose a reason for hiding this comment

savitaashture Jul 6, 2024

Choose a reason for hiding this comment

piyush-garg Jul 5, 2024

Choose a reason for hiding this comment

savitaashture Jul 6, 2024

Choose a reason for hiding this comment

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 8, 2024

savitaashture commented Jul 8, 2024

savitaashture commented Jul 8, 2024

savitaashture commented Jul 8, 2024

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 8, 2024

piyush-garg Jul 8, 2024

Choose a reason for hiding this comment

piyush-garg Jul 8, 2024

Choose a reason for hiding this comment

savitaashture Jul 8, 2024

Choose a reason for hiding this comment

vdemeester commented Jul 8, 2024

savitaashture commented Jul 5, 2024 •

edited

Loading

savitaashture commented Jul 5, 2024 •

edited

Loading

savitaashture commented Jul 5, 2024 •

edited

Loading

savitaashture commented Jul 5, 2024 •

edited

Loading