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-0036: Part 2: Introduce securityContext for build strategy spec #1266

Conversation

SaschaSchwarze0
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 commented Apr 14, 2023

Changes

This is part of the implementation of SHIP-0036: RunAs user and group for supporting steps.

This pull request introduces the securityContext on the BuildStrategy. It changes the BuildRun reconciler to set the runAsUser and runAsGroup of steps based on the build strategy's securityContext. /etc/group and /etc/passwd are mounted through downward API volumes as described in the ship.

In the Git step implementation I also added a verbose flag. This is false by default and not enabled anywhere. I needed it for additional logging during the implementation. As this can be helpful in future changes, I left it in, but can also remove it if desired.

The .ko.yaml is updated to use the new base images from part 1. The configuration was updated to have stricter security context's without privilege escalation and capabilities, and with the new shared home directory.

All sample build strategies have been updated to specify a strategy-level security context. The now unnecessary prepare steps have all been removed. The Paketo strategies are now using the Jammy builder and are proving that it is possible to run as an alternative non-root user successfully.

The change in gomega.go introduces an additional matcher. I am retaining it although the test code where I used it does not exist anymore after I refactored the solution based on feedback in the ship.

Documentation has been updated.

I am marking this as HOLD because a rollout must be coordinated with the PR SHIP-0036: Part 1: Update base images to allow an arbitrary user to run it #1268. Once both are approved, then

  • The HOLD will be removed from the other pull request.
  • A manual run of the base images build will be triggered.
  • The temporary commit DO NOT MERGE Use base images from other pull request will be updated to use the base images from the other PR.
  • The HOLD label on this PR will be removed.
  • After the tests finished, this PR will be merged.
  • All existing pull requests will need to be rebased when changes are made because the old code cannot run with the new base images.

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

Release Notes

You can now define a securityContext on build strategy level to control the runAs user for all steps including the shipwright-managed steps. This allows you to use any runAs user for your build strategy steps while still being able to run without any runAsRoot steps.

@openshift-ci openshift-ci bot added release-note Label for when a PR has specified a release note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 14, 2023
@SaschaSchwarze0 SaschaSchwarze0 force-pushed the sascha-0036-common-runas branch 3 times, most recently from 024c9d6 to 7b7890a Compare April 14, 2023 14:28
@SaschaSchwarze0 SaschaSchwarze0 changed the title [WIP] SHIP-0036: Part 2: Update runtime logic to use a common runAsUser and runAsGroup in the steps when possible [WIP] SHIP-0036: Option 2: Part 2: Update runtime logic to use a common runAsUser and runAsGroup in the steps when possible Apr 17, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2023
@SaschaSchwarze0 SaschaSchwarze0 changed the title [WIP] SHIP-0036: Option 2: Part 2: Update runtime logic to use a common runAsUser and runAsGroup in the steps when possible [WIP] SHIP-0036: Part 2: Update runtime logic to use a common runAsUser and runAsGroup in the steps when possible May 3, 2023
@SaschaSchwarze0 SaschaSchwarze0 force-pushed the sascha-0036-common-runas branch 3 times, most recently from 16b4eca to 86a0c55 Compare May 11, 2023 13:58
@SaschaSchwarze0 SaschaSchwarze0 changed the title [WIP] SHIP-0036: Part 2: Update runtime logic to use a common runAsUser and runAsGroup in the steps when possible [WIP] SHIP-0036: Part 2: Update runtime logic to use a runAsUser and runAsGroup from the securityContext May 11, 2023
@SaschaSchwarze0 SaschaSchwarze0 changed the title [WIP] SHIP-0036: Part 2: Update runtime logic to use a runAsUser and runAsGroup from the securityContext [WIP] SHIP-0036: Part 2: Introduce securityContext for build strategy spec May 11, 2023
@SaschaSchwarze0 SaschaSchwarze0 added kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels May 11, 2023
@SaschaSchwarze0
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2023
@SaschaSchwarze0 SaschaSchwarze0 changed the title [WIP] SHIP-0036: Part 2: Introduce securityContext for build strategy spec SHIP-0036: Part 2: Introduce securityContext for build strategy spec May 11, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2023
Comment on lines +214 to +216
if flagValues.verbose {
log.Printf("Debug: %s %s\n", path, check.versionArg)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is really just that line, I would actually say to remove the verbose flag.

cmd/git/main.go Outdated Show resolved Hide resolved
@SaschaSchwarze0
Copy link
Member Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 19, 2023
Copy link
Contributor

@HeavyWombat HeavyWombat left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 19, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HeavyWombat

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2023
@openshift-merge-robot openshift-merge-robot merged commit 2bc9bb4 into shipwright-io:main Jun 19, 2023
12 checks passed
@SaschaSchwarze0 SaschaSchwarze0 deleted the sascha-0036-common-runas branch June 19, 2023 09:01
@SaschaSchwarze0 SaschaSchwarze0 added this to the release-v0.12.0 milestone Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Label for when a PR has specified a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants