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

feat(CLOUDDST-22843): added signtractions python package #156

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

Conversation

midnightercz
Copy link

@midnightercz midnightercz commented Jun 7, 2024

Signtractions is wrapper for pubtools-sign project which allows signing containers with cosign.
This commit is related to the konflux-ci/release-service-catalog#442 which use project signtractions as tekton task for signing containers in release snapshots

@midnightercz midnightercz requested a review from a team as a code owner June 7, 2024 08:06
@mmalina
Copy link
Contributor

mmalina commented Jun 7, 2024

@midnightercz please fix the commit/PR title to conform to the gitlint check. Here it should probably be "feat(JIRA-ID): add signtractions python package". Also, can you provide more details? I assume this will be used in a tekton task, so can you provide a link to the related release-service-catalog? It would be nice to have that connection. Also, why is this needed for cosign to be able to sign? Can't cosign sign images on its own? I would assume so. Finally, do I really need to create a gitlab.com account to view the source of that package? If I go to https://gitlab.com/exd-sp-cloud-dist/signtractions it asks me to login.

@midnightercz
Copy link
Author

@mmalina I'll fix the PR title. I'm about to send PR to release-service-catalog with tekton task and tests. I'll fill the links in description after that.
Cosign itself can do the signing, but it doesn't process data needed for signing. Signtrations is used for that + signing itself via pubtools-sign. Signtractions is hosted at github now (https://github.com/midnightercz/signtractions/), I need to change the link in pypi in the next release

@midnightercz midnightercz changed the title Added signtractions python package feat(CLOUDDST-22843): Added signtractions python package Jun 7, 2024
@midnightercz midnightercz force-pushed the signtractions-dep branch 3 times, most recently from 4cf3e08 to 0fc8b88 Compare June 7, 2024 11:37
@johnbieren
Copy link
Collaborator

The commit title needs a lowercase a (added not Added)

@mmalina
Copy link
Contributor

mmalina commented Jun 7, 2024

The commit title needs a lowercase a (added not Added)

and also preferably "add", not "added" ;)

Signtractions is hosted at github now (https://github.com/midnightercz/signtractions/), I need to change the link in pypi in the next release

That's a personal repo. Do you plan on moving this to some Red Hat owned org?

@midnightercz
Copy link
Author

midnightercz commented Jun 7, 2024

and also preferably "add", not "added" ;)

Right, I'll fix that.

That's a personal repo. Do you plan on moving this to some Red Hat owned org?

sure, I can move it. Do you have any candidates? There's release-engineering, but I wasn't sure it's the best org for kunflux related stuff

@midnightercz midnightercz changed the title feat(CLOUDDST-22843): Added signtractions python package feat(CLOUDDST-22843): added signtractions python package Jun 7, 2024
Signtractions is wrapper for pubtools-sign project which
allows signing containers with cosign.
This commit is related to the
konflux-ci/release-service-catalog#442
which use project signtractions as tekton task for signing
containers in release snapshots

Signed-off-by: Jindrich Luza <[email protected]>
@davidmogar
Copy link
Contributor

davidmogar commented Jun 7, 2024

Hello @midnightercz!

Thanks for your contribution. Unfortunately this is not something we want to go forward with.

First reason for this and one that we are aiming to weight even more in the future is that this is a personal project. For us, relying in not well-known or Red Hat supported packages/repos/images is a problem as individuals could alter the outcome of any managed Release using those deps.

Second reason is the complexity. Although we accept external contributions and we welcome them, at the end of the day we become responsible for maintaining them. In this particular case, the usage of that package as seen in the linked PR requires advance and specific knowledge on the tooling used. This would be a good example.

I know this message can be frustrating, specially when you have kindly dedicated your time to contribute to the Release Catalog. You are also an expert in this tool you are proposing and for you this must be an obvious change. Having said that, our ultimately goal is to make the Release process as easy as it can be for the final user and I think this change moves us away from that.

@midnightercz
Copy link
Author

Hello @davidmogar
I'm not external contributor, I'm part of Red Hat SP Cloud. The project is hosted in my personal namespace so far as I wasn't sure what would be the best org where to put it as only organization where I have access in github is release-engineering which is quite overused.
Package signtractions used here is wrapper for pubtools-sign project which is currently used for signing container images in existing production pipeline. Pubtools-sign project is currently maintained by cloud distribution team within SP Cloud. The same will apply for signtractions.
To use the package as provider of tekton task there's no extra knowledge required. You need to know only what input you need to provide to achieve desired output. Similarly as you don't have internal knowledge about skopeo or podman and only need to know right parameters for the commands and rely on another team to maintain the tool. So to say cloud distribution will be responsible for maintaining the tekton task and relevant dependencies.
Signtractions also supports existing signing (RADAS) as it's supported by pubtools-sign project. Signing functionality could be later unified by this project.
Besides that it has other benefits, like strict data validation, logging, unit tests, extendibility (due to modular programming approach), scalability, autogenerated documentation (in progress)

@davidmogar
Copy link
Contributor

I know you are not an external contributor @midnightercz, but we find ourselves in the same situation: relying on a package from a personal project. This expose security concerns.

Regarding the complexity and knowledge required, as you mentioned, we need to know the input to pass to the task. This is complex enough from our point of view, and it would have to be passed in a Release Pipeline that we (Release Team) maintain or, even worse, by users in their ReleasePlanAdmissions.

For those reasons, we are rejecting the change as it is. We can schedule a meeting if you want and talk around where to go from here. We are more than happy to support you in writing a change that aligns better with our Pipelines and with the way in which we want users to use them.

@midnightercz
Copy link
Author

midnightercz commented Jun 7, 2024

@davidmogar
Can you explain to me how this https://github.com/konflux-ci/release-service-catalog/blob/development/tasks/rh-sign-image/rh-sign-image.yaml#L14 - for example
is less complex to this:
https://github.com/konflux-ci/release-service-catalog/pull/442/files#diff-4778059ceb567c12e748596fcb45d0bc5c55adb2bd30eb275fb1e5ba4f83d477R55 ?

r_signer_wrapper_cosign - will be basically constant, only config file needs to be changed when signing to different envs - which is not your case I assume as you're always signing to quay.io.
r_dst_quay_client - this will be basically Secret.
i_snapshot_str - I assume not used in your use case as you have snapshots in files.
i_snapshot_file - well you're already using this in other tasks so users should be pretty familiar with what is it
i_signing_key - yes, this one users need to know about. But it's just string
i_task_id - not used, can be constant
a_pool_size - can be constant, only need to be adjusted when there are performance issues
a_executor_type - can be constant, this will be probably set to THREAD or LOCAL if you don't want to process things in parallel.

so to pipeline params only 4 attributes bubble up: i_signing_key, i_snapshot_file, r_signer_wrapper_cosign, r_dst_quay_client.
Where for end users
r_signer_wrapper_cosign and r_dst_quay_client will be most likely something like secret-quay-client-prod, secret-cosign-signer-prod - well I assume those can be derived from some stage|prod 'env' param and other two are strings. Well actually even the signing key can be probably derived from some 'env' param if you don't want to use different keys for different products. And I see that snapshot is actually defined in params already.
That sums up to 1 new parametr - if you're not pushing to stage then no parameters.

So I would really like to know how this complicates things for end users

@davidmogar
Copy link
Contributor

Hello again @midnightercz,

I'm going to leave in here some very specific examples of how we do everything in our tasks/pipelines.

First of all this is a link to one of our Pipelines. One thing to note in here is that we don't allow arbitrary parameters into our Pipelines. The parameters in there are provided by our operator and will not change. The way in which users provide information is by adding it to the Release, ReleasePlan or ReleasePlanAdmission resources data key. This would be an example:

apiVersion: appstudio.redhat.com/v1alpha1
kind: ReleasePlanAdmission
metadata:
  labels:
    release.appstudio.openshift.io/auto-release: "true"
  name: cnv-fbc-prod-index-v4-15-plus
  namespace: rhtap-releng-tenant
spec:
  ...
  data:
    releaseNotes:
      product_name: Red Hat OpenShift Virtualization
      product_version: fbc
    fbc:
      # note that the tags below will be updated by the Release pipeline at runtime
      fromIndex: 'regis...'
      targetIndex: 'quay.io/...'
      binaryImage: 'regis...'
      publishingCredentials: '...'
      iibServiceConfigSecret: 'iib...'  # depending upon environment
      iibOverwriteFromIndexCredential: 'iib...'
      requestUpdateTimeout: 1500
      buildTimeoutSeconds: 1500
      allowedPackages: [...]
    sign:
      configMapName: ...

So yeah, our users are exposed to some complexity but, so far, they are only required to pass short value parameters.

Regarding Tasks, this is a good example of a task executing a Python script and consuming the information from the stored resources.

At a very least your contribution:

  • should come from a Red Hat repository and not from a personal one.
  • should consume data from the passed resources (check the Pipeline).
  • should use proper defaults.

In addition we are not a fan of the way the task is generated or of things like:

- description: |-
    DESCRIPTION: 'Pool size used for STMD tractions'
    TYPE: Arg[int](
        a: int
    )
  name: a_pool_size
  type: string
  --------
 - name: a_pool_size
   value: |-
     ---
     a: 5

You are exposing unnecessary logic. That could be defined in the task like:

- name: poolSize
  type: int
  default: 5

Once again, we are more than happy to help you with this. Schedule a meeting with us and we will discuss where to go from here. If not, you have a starting point in this message.

@midnightercz
Copy link
Author

First of all this is a link to one of our Pipelines. One thing to note in here is that we don't allow arbitrary parameters into our Pipelines. The parameters in there are provided by our operator and will not change. The way in which users provide information is by adding it to the Release, ReleasePlan or ReleasePlanAdmission resources data key.

It would be nice to have this maybe in contributing.md maybe?

I have updated konflux-ci/release-service-catalog#442 please check if new style of params is fitting your needs

@johnbieren
Copy link
Collaborator

@midnightercz This is an old PR with no recent activity. Is it still needed or can it be closed?

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