Skip to content

Proposal to remove Tekton resources #727

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

Conversation

SaschaSchwarze0
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 commented Apr 10, 2021

Changes

Related to Remove usage of Tekton resources #696.

This is still provisional, but looking for some feedback on some aspects.

It was honestly complicated impossible at some places to keep this EP isolated. I know this makes it complex, but the design implications of getting rid of the resources at many places brought up topics that are in other EPs/issues/pull requests (listed in the risks section). I decided to touch those topics then. Otherwise, it would have imo been incomplete.

For those interested to see code, here are two PoCs related to this work:

  1. remove_image_resource from @zhangtbj removes the image output resource. It does not follow the naming defined in this EP as this came later, but it outlines very well how the image
  2. sascha-git-clone from me eliminates the source input resource. Workspace, parameter and arguments naming matches the EP.

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

NONE

@openshift-ci-robot openshift-ci-robot added the release-note-none Label for when a PR does not need a release note label Apr 10, 2021
@SaschaSchwarze0 SaschaSchwarze0 added the kind/design Categorizes issue or PR as related to design. label Apr 10, 2021
@SaschaSchwarze0 SaschaSchwarze0 force-pushed the sascha-ep-remove-tekton-resources branch from 357d285 to 74ee01c Compare April 12, 2021 07:38
@SaschaSchwarze0 SaschaSchwarze0 force-pushed the sascha-ep-remove-tekton-resources branch 3 times, most recently from 5b00da2 to f6291fc Compare April 19, 2021 11:44
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.

This was quite the read. Thanks for this proposal. It makes absolutely sense to me. We should tackle the TODO with regards to the prefix and then move on with this one.

@qu1queee qu1queee self-requested a review April 19, 2021 14:26
Copy link
Contributor

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks for this thorough EP! It's quite a lot to tackle but I think it'll set us up for success in the long term. 👍

In our sample build strategies, we should support these two results as much as possible:

- `ko` supports to write an OCI image manifest file using the `--oci-layout-path` argument. The strategy needs to write that manifest to a temporary location, use a tool to extract the digest and size from the index.json file, and write this data to the result files.
- Kaniko supports to write an OCI image manifest file using the `--oci-layout-path` argument. Kaniko is based on scratch, as such, we cannot extend the existing build-and-push step to use some tool and run some extraction logic. We need to add an extra step. For this, we need to add volumeMounts to write the temporary file in the `build-and-push` step, and consume it in the following one to extract and store the results.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, that might not work. Kaniko works by running all steps of the Dockerfile in its own container. When loading the FROM image, this overwrites the root file system of the Kaniko image which normally is not there as it is based on scratch. So, after Kaniko finished, you have no guarantee on what is still there (I might have missed it, but I think /busybox is not excluded when Kaniko for example cleans up the whole file system, beside /kaniko, between two stages). I think the main purpose of the debug image is to start the container with a shell to be able to debug things and manually start Kaniko inside the container. I think we would skate on thin ice to use debug to be able to run something after Kaniko finished.

7. Improve the implementation for HTTP sources to make it consistent with this design.
8. Define and implement a well-defined set of exit codes per resource kind and use them in the BuildRun controller for an enhanced error reporting.

The goal to run as non-root is not covered in great detail in this proposal as some items are not 100 % clear to me yet, for example:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather remove this an an explicit goal of this EP, and just mention it as a nice-to-have that this enables later on. There are a number of open questions just for this item, and I don't want to block the rest of the improvement on answering these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, maybe, I was trying this out today. Current state: PSP enforces MustRunAsNonRoot, git image is built with ko and base image ubi-minimal, I have no security context on the container nor on the pod. It works. Based on k8s docs this imo must mean that ko is building images that have a USER directive. Need to check as which user the container actually runs.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2021
@SaschaSchwarze0
Copy link
Member Author

Updating proposal, changes:

  • Added shp- prefix to parameters and results.
  • Removed the image-tag parameter from implementation details, moved to alternatives
  • Added crane digest details to get the image digest for BuildKit
  • Moved single-container-fo-all-sources approach to alternatives

@SaschaSchwarze0 SaschaSchwarze0 force-pushed the sascha-ep-remove-tekton-resources branch from f6291fc to 853028a Compare April 20, 2021 09:54
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2021
@SaschaSchwarze0 SaschaSchwarze0 force-pushed the sascha-ep-remove-tekton-resources branch from 853028a to 13c1e84 Compare April 21, 2021 07:19
@SaschaSchwarze0
Copy link
Member Author

Updated proposal, changes:

  • Made the sentence about potential limitations in using go-git more specific by mentioning the lfs support that I think is not there.

Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

/lgtm

EDIT: Almost forgot to say, great work! nice one. I dont have any request for change, I think we discussed this one internally before, which make it easier for me to digest it faster. I think all the right ideas are in place, looking forward to contribute to the implementation part.


## Summary

Nothing really to add here. I would repeat the above motivation and the below goals if I would write something here.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could always add something more generic, as in:

The deprecation of Tekton PipelineResources will block us from operating with input or output resources. These resources are valuable for Shipwright/Build because it allow us to input artifacts ( like source code from git ) and to surface metadata at the TaskRun Status level in an abstracted way. This EP proposes an approach on how to fullfill those future deprecated features in Tekton side.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2021
@qu1queee
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qu1queee

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 21, 2021
@openshift-merge-robot openshift-merge-robot merged commit 0db21d5 into shipwright-io:master Apr 21, 2021
@SaschaSchwarze0 SaschaSchwarze0 deleted the sascha-ep-remove-tekton-resources branch April 22, 2021 11:24
@adambkaplan adambkaplan added this to the release-v0.5.0 milestone Jun 10, 2021
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/design Categorizes issue or PR as related to design. lgtm Indicates that a PR is ready to be merged. release-note-none Label for when a PR does not need a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants