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

Add PNC prebuild task based upon git-clone-oci-ta #1819

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rnc
Copy link

@rnc rnc commented Jan 14, 2025

As middleware (PNC) need to preprocess the source in order to prepare it for building within a container an extra step is required for the PNC custom pipeline

@rnc rnc marked this pull request as ready for review January 15, 2025 12:08
@rnc rnc requested a review from a team as a code owner January 15, 2025 12:08
task/pnc-prebuild-git-clone-oci-ta/0.1/patch.yaml Outdated Show resolved Hide resolved
Comment on lines 64 to 65
- prepare
- --build-tool-version=$(params.BUILD_TOOL_VERSION)
- --java-version=$(params.JAVA_VERSION)
- --recipe-image=$(params.RECIPE_IMAGE)
- --tooling-image=$(params.PNC_KONFLUX_TOOLING_IMAGE)
- --type=$(params.BUILD_TOOL)
- /var/workdir/source
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 a very opaque command. It isn'y clear what this actually does to the source especially since it is running with full access to the internet. I don't know how we would be able to qualify/verify our trust in the changes made here.

Copy link
Author

@rnc rnc Jan 17, 2025

Choose a reason for hiding this comment

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

Admittedly this command does not need network access and is similar to what we would need if KONFLUX-4903 was available. It runs java code to preprocess the source to ensure its possible to build it inside a container from https://github.com/project-ncl/konflux-tooling and is only meant to be used by middleware/pnc. Is one of the main concerns because the language is not in shell scripting?

Choose a reason for hiding this comment

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

If the concern is full internet access we can probably run it within "unshare" but I believe it's a matter of trusting the application, that it rally does what it should?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it doesn't need internet access, I'd restrict it.

Copy link
Author

Choose a reason for hiding this comment

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

@mmorhun Are you saying this is your only objection before approval? That tooling image does not currently have unshare installed so we would have to modify the image I suppose.

Copy link
Author

Choose a reason for hiding this comment

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

I'll look at installing unshare in the tooling image and running this command with unshare -n /opt/jboss/container/java/run/run-java.sh...

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately I get unshare: unshare failed: Operation not permitted when trying to run the above command

Copy link
Author

Choose a reason for hiding this comment

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

Rather than unshare, would it be acceptable to simply define -DsocksProxyHost=does.not.exist which according to https://docs.oracle.com/javase/7/docs/technotes/guides/net/proxies.html should force all outgoing TCP connections to go via the proxy (which will then fail) ?

Copy link
Collaborator

@mmorhun mmorhun left a comment

Choose a reason for hiding this comment

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

Please update the CODEOWNERS file.

- op: replace
path: /spec/description
value: The pnc-prebuild-git-clone-oci-ta Task will clone a repo from the provided url, apply
PNC prebuild modifications from https://github.com/project-ncl/konflux-tooling and store
Copy link
Collaborator

@mmorhun mmorhun Jan 24, 2025

Choose a reason for hiding this comment

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

konflux-tooling sounds too generic for me, maybe pnc-konflux-tooling would be better? At least for the image name

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure how we can change the image name as isn't it derived from the repository name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can release it via Konflux

task/pnc-prebuild-git-clone-oci-ta/0.1/patch.yaml Outdated Show resolved Hide resolved
- --build-tool-version=$(params.BUILD_TOOL_VERSION)
- --java-version=$(params.JAVA_VERSION)
- --recipe-image=$(params.RECIPE_IMAGE)
- --tooling-image=quay.io/redhat-user-workloads/konflux-jbs-pnc-tenant/konflux-tooling:latest
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we should reference an image from redhat-user-workloads as this can be accidentally deleted of the component is deleted. Can we release this to be an image in quay.io/konflux-ci instead?

Copy link
Author

Choose a reason for hiding this comment

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

Agreed but we'll have to discuss that on how best to release that single component to the konflux owned namespace.

Copy link
Collaborator

@mmorhun mmorhun left a comment

Choose a reason for hiding this comment

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

Approving with the only condition: task image is released via Konflux.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants