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

[FEATURE] spec.buildSteps[4].securityContext.runAsUser: Invalid value: "string" #1354

Open
1 task done
cmoulliard opened this issue Aug 9, 2023 · 11 comments
Open
1 task done
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature.
Milestone

Comments

@cmoulliard
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Kubernetes Version

1.27

Shipwright Version

Shipwright: v0.11.0
Tekton: v0.48.0

Current Behavior

When we would like to use $(params.USER_ID) part of the following YAML syntax of the BuildStrategy

buildSteps:
    - name: build-and-push
      image: $(params.CNB_BUILDER_IMAGE)
      imagePullPolicy: Always
      securityContext:
        runAsUser: $(params.USER_ID)
        runAsGroup: $(params.GROUP_ID)
      command: ["/cnb/lifecycle/builder"]
      args:
        - "-app=$(workspaces.source.path)/$(params.SOURCE_SUBPATH)"
        - "-layers=/layers"
        - "-group=/layers/group.toml"
        - "-plan=/layers/plan.toml"

we got this error: * spec.buildSteps[4].securityContext.runAsUser: Invalid value: "string": spec.buildSteps[4].securityContext.runAsUser in body must be of type integer: "string"

Ideally we should be able to pass a parameter which is nextconverted to an int

        runAsUser: 1001
        runAsGroup: 1000

Expected Behavior

No response

Steps To Reproduce

No response

Anything else?

No response

@cmoulliard cmoulliard added the kind/bug Categorizes issue or PR as related to a bug. label Aug 9, 2023
@SaschaSchwarze0
Copy link
Member

cmoulliard added a commit to redhat-buildpacks/testing that referenced this issue Aug 9, 2023
@qu1queee qu1queee added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Aug 23, 2023
@qu1queee
Copy link
Contributor

From refinement, adding a priority to this issue and help wanted label. We also discussed on the current K8S version and the need for an update (not related to this issue)

@qu1queee
Copy link
Contributor

@SaschaSchwarze0 does this issue qualifies for a bug (without reading the slack conversation) ?

@SaschaSchwarze0
Copy link
Member

@SaschaSchwarze0 does this issue qualifies for a bug (without reading the slack conversation) ?

Imo not, I consider this a feature request. We probably have not documented today in all possible details where a param can be used and where not, which is also something we can do. So far, we've only documented the limitation where config and secret references can be used (in https://github.com/shipwright-io/build/blob/main/docs/build.md#defining-paramvalues).

@qu1queee qu1queee added kind/feature Categorizes issue or PR as related to a new feature. kind/documentation Categorizes issue or PR as related to documentation. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jan 11, 2024
@qu1queee qu1queee changed the title [BUG] spec.buildSteps[4].securityContext.runAsUser: Invalid value: "string" [Feature] spec.buildSteps[4].securityContext.runAsUser: Invalid value: "string" Jan 11, 2024
@qu1queee qu1queee changed the title [Feature] spec.buildSteps[4].securityContext.runAsUser: Invalid value: "string" [FEATURE] spec.buildSteps[4].securityContext.runAsUser: Invalid value: "string" Jan 11, 2024
@cmoulliard
Copy link
Author

Imo not, I consider this a feature request.

Ideally such a parameter should be set from a step executed previously aka what we can using Tekton task results

@SaschaSchwarze0
Copy link
Member

Imo not, I consider this a feature request.

Ideally such a parameter should be set from a step executed previously aka what we can using Tekton task results

This would not be possible with Shipwright's current design to use a TaskRun to run a BuildRun. We basically have only one Pod and container run-as values are immutable. Or basically, at the time a step would write that result, the containers of all other steps are already started (and are handing in Tekton's entrypoint).

@cmoulliard
Copy link
Author

This would not be possible with Shipwright's current design to use a TaskRun to run a BuildRun. We basically have only one Pod and container run-as values are immutable. Or basically, at the time a step would write that result, the containers of all other steps are already started (and are handing in Tekton's entrypoint).

Can a temporary solution be that a build A store the result using a volume or configMap and that an ENV var or param is set from the volume or ConfigMap within the build B ?

@SaschaSchwarze0
Copy link
Member

This would not be possible with Shipwright's current design to use a TaskRun to run a BuildRun. We basically have only one Pod and container run-as values are immutable. Or basically, at the time a step would write that result, the containers of all other steps are already started (and are handing in Tekton's entrypoint).

Can a temporary solution be that a build A store the result using a volume or configMap and that an ENV var or param is set from the volume or ConfigMap within the build B ?

I don't think so. The purpose of a Build is to produce a container image, and not results for another build. What is the use case behind a dynamic runAs configuration of a step that is determined by a previous step?

What would actually work is the emptyDir combined with su: Step 1 can write a file to that emptyDir with the user and group. Step 2 would run as root and would run the desired command through su, would certainly need more privileges then.

@cmoulliard
Copy link
Author

I don't think so. The purpose of a Build is to produce a container image, and not results for another build. What is the use case behind a dynamic runAs configuration of a step that is determined by a previous step?

To build a container image using buildpack and extensions, it is needed to know in advance what the UID/GUID are - see: https://github.com/redhat-buildpacks/catalog/blob/main/tekton/task/buildpacks-extension-check/01/buildpacks-extension-check.yaml as such UID/GUID could be different between the UBI builder images (red hat vs paketo vs tanzu etc) and will allow kanilo to write layers created within the mounted volume

@SaschaSchwarze0
Copy link
Member

I don't think so. The purpose of a Build is to produce a container image, and not results for another build. What is the use case behind a dynamic runAs configuration of a step that is determined by a previous step?

To build a container image using buildpack and extensions, it is needed to know in advance what the UID/GUID are - see: https://github.com/redhat-buildpacks/catalog/blob/main/tekton/task/buildpacks-extension-check/01/buildpacks-extension-check.yaml as such UID/GUID could be different between the UBI builder images (red hat vs paketo vs tanzu etc) and will allow kanilo to write layers created within the mounted volume

So, you create a build strategy where the image itself is dynamic coming from a parameter. And depending on which image is used, the runAsUser/runAsGroup would be different. But, you do not need to define that on the steps that use the image itself (because they would simply use the correct user and group as defined in the image itself), but you need to define those on a step that runs a different image (kaniko). All correct ?

Beside the su option, another option would be to use different build strategies.

@qu1queee qu1queee removed the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label May 23, 2024
@adambkaplan adambkaplan added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Jul 11, 2024
@adambkaplan
Copy link
Member

I think we need to revisit the requirement. We're a bit hesitant to support dynamic parameters for the securityContext field, given that can have impacts on other parts of the pod spec (ex: added capabilities) and required pod security standard.

That said, in theory we can implement a version of the BuildStep API that lets these be set as an IntOrString type, which won't break our YAML/OpenAPI schema. Dependent golang code would need to be updated.

@adambkaplan adambkaplan added this to the Backlog milestone Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature.
Projects
Status: Todo
Development

No branches or pull requests

4 participants