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

Add SecurityContext to Eventlistener containers #1494

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Add SecurityContext to Eventlistener containers #1494

merged 1 commit into from
Feb 14, 2023

Conversation

dibyom
Copy link
Member

@dibyom dibyom commented Dec 14, 2022 •
edited
Loading

Changes

The security context is the same one that is applied to other Tekton workloads such as the Triggers and Pipeline controller pods. Eventlisteners already run as non-root, non-privileged containers. Adding this setting allows them to run in environments with pod security admission set to "restricted" (such as the tekton-pipelines namespace)

Fixes #1490

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Eventlistener containers now contain the right security context to allow running with restricted pod security admission

/kind bug

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 14, 2022
@tekton-robot tekton-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 14, 2022
@dibyom
Copy link
Member Author

dibyom commented Dec 14, 2022

/assign @savitaashture @khrm

@savitaashture
Copy link
Contributor

savitaashture commented Dec 14, 2022 •
edited
Loading

shall we take input from EL spec 🤔
because with this change EL always runs as non root with some security which might fail on Openshift and also if someone wants to run EL without security context then we don't have way specially cases like Openshift

khrm
khrm reviewed Dec 14, 2022
View reviewed changes
Copy link
Contributor

@khrm khrm left a comment

Choose a reason for hiding this comment

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

Seems fine but need to check this once with OpenShift.

@dibyom
Copy link
Member Author

dibyom commented Dec 14, 2022

@savitaashture this changes ensures the container always runs as non-root. Do you have a use case for why someone might want it differently? If openshift or another platform does need it to be customized then yeah we could add it but I like the simplicity of always ensuring the EL container with the same security context

@iancoffey
Copy link
Member

Nice, LGTM

@dibyom
Copy link
Member Author

dibyom commented Jan 5, 2023

@savitaashture @khrm could you please take a look? 🙏

@dibyom
Add SecurityContext to Eventlistener containers
c8cd8b3 
The security context is the same one that is applied to other Tekton workloads
such as the Triggers and Pipeline controller pods. Eventlisteners already run
as non-root, non-privileged containers. Adding this setting allows them to run
in environments with pod security admission set to "restricted" (such as the
tekton-pipelines namespace)

Fixes tektoncd#1490

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
savitaashture
savitaashture approved these changes Jan 27, 2023
View reviewed changes
Copy link
Contributor

@savitaashture savitaashture left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2023
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: savitaashture

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2023
@savitaashture
Copy link
Contributor

/test pull-tekton-triggers-integration-tests

@tekton-robot tekton-robot merged commit 8a1267a into tektoncd:main Feb 14, 2023
khrm
khrm reviewed Mar 2, 2023
View reviewed changes
Copy link
Contributor

@khrm khrm left a comment

Choose a reason for hiding this comment

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

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 2, 2023
@savitaashture savitaashture mentioned this pull request Mar 21, 2023
Merged
4 tasks
@dibyom dibyom deleted the sc branch March 21, 2023 17:40
@savitaashture savitaashture mentioned this pull request Mar 23, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khrm khrm khrm left review comments

@savitaashture savitaashture savitaashture approved these changes

@bobcatfish bobcatfish Awaiting requested review from bobcatfish

@iancoffey iancoffey Awaiting requested review from iancoffey

Assignees

@khrm khrm

@savitaashture savitaashture

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EventListeners do not have correct SecurityContext to run in tekton-pipelines namespace
5 participants
@dibyom @savitaashture @iancoffey @tekton-robot @khrm