Add label selector option for Secret and ConfigMap sources

[ocampeau]

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:

• all Secret that has label fruit: banana and a key ca.crt will have the value of ca.crt included in the Bundle
• all ConfigMap that has label fruit: apple and a key ca.crt will have the value of ca.crt included in the Bundle

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:

• all ConfigMap that has label fruit: banana and a key ca.crt will have the value of ca.crt included in the Bundle
• all ConfigMap that has label fruit: apple and a key ca.crt will have the value of ca.crt included in the Bundle

fixes #256

[jetstack-bot]

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[erikgb]

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]

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

[ocampeau]

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

[SgtCoDFish]

/ok-to-test

[erikgb]

Added some review comments, but mostly nits. I am prepared to move forward with this PR when the code looks good. Well done @ocampeau! 😍

[ocampeau]

/retest-required

[inteon]

@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]

[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 [inteon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ocampeau]

I have resolved all conversation except this one: #258 (comment)

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]

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

[erikgb]

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!

[erikgb]

Some minor nits left, but this is close to a LGTM from me!

[erikgb]

Suggested change

	el = append(el, field.Invalid(path, fmt.Sprintf("name: %s, selector: {}", configMap.Name), "must validate one and only one schema (oneOf): [name, selector]. Found none valid"))
	el = append(el, field.Invalid(path, fmt.Sprintf("name: %s, selector: {}", configMap.Name), "must validate one and only one schema (oneOf): [name, selector]. Found both set"))

I am not sure about the error message here, but it should indicate that we detected both set - which is invalid.

[ocampeau]

I think Found both set is good. It makes sens to me at least.

[inteon]

/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]

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

[ocampeau]

Documentation added with this PR: cert-manager/website#1391