Skip to content

Add design for trust source plugins #654

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

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

Conversation

SgtCoDFish
Copy link
Member

Follows discussion in standup on 2025-07-16

@cert-manager-prow
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@cert-manager-prow cert-manager-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jul 16, 2025
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign maelvls for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@cert-manager-prow cert-manager-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 16, 2025
@cert-manager-prow cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jul 16, 2025
image: docker.io/library/debian:bookworm
path: /etc/ssl/certs/ca-certificates.crt
status:
bundleData: |

Choose a reason for hiding this comment

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

To avoid object size limits I would have the status expose an array of secrets that contain the actual bundle, most implementations would probably only ever need a single secret, but cant hurt to future proof it in the design.

Something like:

status:
  bundleSecrets:
  - name: example-bundle-01
  - name: example-bundle-02

For namespaced sources the secrets would exist in the same namespace as the resource, for cluster sources the secrets would exist in the trust-manager namespace

Copy link
Member

Choose a reason for hiding this comment

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

If any source exceeds the size limit, the targets will also exceed the size limit. I don't think we need to account for this additional complexity.

Copy link

@ThatsMrTalbot ThatsMrTalbot Jul 17, 2025

Choose a reason for hiding this comment

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

IIRC we have had issues raised in the past about peoples bundles exceeding the size, I would argue that is where a CSI driver could help 🙂

I also wonder if post-quantum will increase CA cert size enough to make hitting the 1mb limit more likely

## Open Questions

- If the source plugins need a separate controller, why should that controller not simply write its data into a ConfigMap or Secret, which trust-manager can then consume natively?
- Concretely: What do we gain from having CRDs here?

Choose a reason for hiding this comment

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

  1. If a source produces a bundle larger than 1mb then it will need to shard it across multiple secrets, you would need to know that to include all the shards in your trust bundle. By referencing the CRD you get a single object you can reference
  2. It gives us the option of having cluster scope sources, if you just reference secrets you can never have a namespaced TrustBundle reference anything outside its own namespace. I think cluster scope sources would be a great way for us to ship our "default" CAs with a ClusterImageTrustSource
  3. I think its better UX. If you used secrets then a ImageTrustSource would ether need to produce a deterministic secret name or have a configurable secret name, and IMO the secret is just an implementation detail.


- If the source plugins need a separate controller, why should that controller not simply write its data into a ConfigMap or Secret, which trust-manager can then consume natively?
- Concretely: What do we gain from having CRDs here?
- Does this make setup difficult? Plugin CRDs should be able to be installed before trust-manager.

Choose a reason for hiding this comment

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

The controller-runtime uses a dynamic discovery implementation, so CRDs do not need to be installed first.

- If the source plugins need a separate controller, why should that controller not simply write its data into a ConfigMap or Secret, which trust-manager can then consume natively?
- Concretely: What do we gain from having CRDs here?
- Does this make setup difficult? Plugin CRDs should be able to be installed before trust-manager.
- Does this require every plugin to bind a role with CRD read permissions to the trust-manager service account?

Choose a reason for hiding this comment

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

This is a good point, probably yes

- Concretely: What do we gain from having CRDs here?
- Does this make setup difficult? Plugin CRDs should be able to be installed before trust-manager.
- Does this require every plugin to bind a role with CRD read permissions to the trust-manager service account?
- Does the key from the status field need to be configurable? Most likely it does - how do we configure it?

Choose a reason for hiding this comment

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

I would not have it configurable, I would have a "contract" we have with plugins that says you must have this field to function. That is how ClusterAPI works for example, the providers have their own CRDs that work with ClusterAPI if they fulfil the API contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants