cert-manager / trust-manager

trust-manager is an operator for distributing trust bundles across a Kubernetes cluster.
https://cert-manager.io/docs/projects/trust-manager/
Apache License 2.0
243 stars 65 forks source link

Add label selector option for Secret and ConfigMap sources #258

Closed ocampeau closed 7 months ago

ocampeau commented 8 months ago

This PR adds support to use label selector for Secret and ConfigMap sources.

Given a Bundle that looks like this:

apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
  name: some-bundle
spec:
  sources:
  - useDefaultCAs: true
  - configMapLabelSelector:
      key: ca.crt
      selector:
        matchLabels:
          fruit: apple
  - secretMapLabelSelector:
      key: ca.crt
      selector:
        matchLabels:
          fruit: banana
  target:
    configMap:
      key: ca.crt

this behavior will happen:

Multiple configMapLabelSelector and secretMapLabelSelector are supported.

In a case like this:

apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
  name: some-bundle
spec:
  sources:
  - useDefaultCAs: true
  - configMapLabelSelector:
      key: ca.crt
      selector:
        matchLabels:
          fruit: apple
  - configMapLabelSelector:
      key: ca.crt
      selector:
        matchLabels:
          fruit: banana
  target:
    configMap:
      key: ca.crt

then:

fixes #256

jetstack-bot commented 8 months ago

Hi @ocampeau. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
erikgb commented 8 months ago

I wonder if we need to extend the API with "new source types"; could we instead just extend the existing source configMap and secret types to allow either name or selector? Making the following bundle valid:

apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
  name: some-bundle
spec:
  sources:
  - useDefaultCAs: true
  - configMap:
      key: ca.crt
      name: my-configmap
  - configMap:
      key: ca.crt
      selector:
        matchLabels:
          fruit: apple
  - secret:
      key: ca.crt
      name: my-secret
  - secret:
      key: ca.crt
      selector:
        matchLabels:
          fruit: banana
  target:
    configMap:
      key: ca.crt
ocampeau commented 8 months ago

Yeah definitely! I will update my PR to reuse the existing source type like you mention. It's much better like that.

ocampeau commented 8 months ago

It's done. Let me know what you think.

SgtCoDFish commented 7 months ago

/ok-to-test

ocampeau commented 7 months ago

/retest-required

inteon commented 7 months ago

@ocampeau This is an awesome new feature❤️! Thanks a lot for contributing this. /approve Can be LGTM'd once the two comments above have been resolved ^^

jetstack-bot commented 7 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

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: - ~~[OWNERS](https://github.com/cert-manager/trust-manager/blob/main/OWNERS)~~ [inteon] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
ocampeau commented 7 months ago

I have resolved all conversation except this one: https://github.com/cert-manager/trust-manager/pull/258#discussion_r1445222407

Let me know if this is acceptable or not.

Thanks a lot for accepting this PR by the way, it will really help us. It was a pleasure to contribute to this project.

ocampeau commented 7 months ago

Any ETA on when this could potentially be merged? Do you have documentation on the release velocity of this project?

erikgb commented 7 months ago

Any ETA on when this could potentially be merged? Do you have documentation on the release velocity of this project?

Sorry, I have been very busy lately. Will try to review this weekend if @inteon hasn't reviewed and merged it before.

We have a couple of unreleased changes already, so I would suggest considering releasing 0.8.0 after this PR is merged. WDYT, @inteon? Sorry for the delay, @ocampeau!

inteon commented 7 months ago

/lgtm Thanks a lot for the feature. After this gets merged, we can do the v0.8.0 release. Also, @ocampeau could you create a documentation PR on the website to document this new feature?

ocampeau commented 7 months ago

Sure I can add documentation. I'll take a look into it

ocampeau commented 7 months ago

Documentation added with this PR: https://github.com/cert-manager/website/pull/1391