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.

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

SHIP-0040: Support RuntimeClass in Builds #263

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adambkaplan
Copy link
Member

Changes

Proposal to specify the Kubernetes Runtime Class in build pods. This is a direct follow up to SHIP-0039 that refines our ability to schedule and execute build pods.

Support for Kubernetes User Namespaces is also floated as an option to include in the scope, which would fit a broader theme of improving build security thorugh multiple layers of system isolation. The SHIP is thus marked as "provisional" to foster discussion.

/kind feature

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

NONE

Proposal to specify the Kubernetes Runtime Class in build pods. This is
a direct follow up to SHIP-0039 that refines our ability to schedule
and execute build pods.

Support for Kubernetes User Namespaces is also floated as an option to
include in the scope, which would fit a broader theme of improving
build security thorugh multiple layers of system isolation. The SHIP is
thus marked as "provisional" to foster discussion.

Signed-off-by: Adam Kaplan <[email protected]>
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 25, 2025
@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 25, 2025
Copy link

openshift-ci bot commented Mar 25, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign qu1queee for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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


## Open Questions [optional]

- Should user namespaces also be in scope for this feature?
Copy link
Member

Choose a reason for hiding this comment

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

This is still a Kubernetes Beta feature which is disabled by default afaik. Anyway, I would be okay.

Comment on lines +137 to +140
The reconcile loop for `BuildRun` could in theory catch this situation ahead of time and fail the
build prior to pod creation. We could also test this scenario and see how Tekton `TaskRuns` behave;
if the `TaskRun` fails with a reasonable error message, then Shipwright `BuildRun`s can simply echo
the information in their status.
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 Mar 30, 2025

Choose a reason for hiding this comment

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

I prefer if we continue to create the TaskRun without checking this. We already don't validate other things in that area that could go wrong. We for example do not check the pod security enforcement on the namespace against the security context of the build strategy.

What we may want to validate is the existence of the runtime class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants