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

More flexible and better organized target specification in API #243

Open erikgb opened 9 months ago

erikgb commented 9 months ago

I will start with a "max" target specification using the existing API (also including #235 - which I think will be merged before this issue is eventually addressed):

target:
  configMap:
    key: root-certs.pem
  secret:
    key: root-certs.pem
  additionalFormats:
    jks:
      key: bundle.jks
      password: jks-password
    pkcs12:
      key: bundle.p12
      password: pkcs12-password

I can identify several issues with this specification structure:

To solve most of these issues, I suggest a more flexible (and less "discriminating") structure. It might appear more complex, but remember that most users will probably not use a "max" specification, and only specify relevant fields for their setup.

target:
  configMap:
    pem:
      key: root-certs.pem
    jks:
      key: bundle.jks
      password: jks-password
    pkcs12:
      key: bundle.p12
      password: pkcs12-password
  secret:
    pem:
      key: root-certs.pem
    jks:
      key: bundle.jks
      password: jks-password
    pkcs12:
      key: bundle.p12
      password: pkcs12-password
hazmat345 commented 9 months ago

Maybe this could also be a chance to bring the target side more in line with the sources side? What would you think about something like this:

targets:
  - configMap:
      format: pem
      key: root-certs.pem
  - configMap:
      format: jks
      key: bundle.jks
      password: jks-password
  - configMap:
      format: pkcs12
      key: bundle.p12
      password: pkcs12-password
  - secret:
      format: pem
      key: root-certs.pem
  - secret:
      format: jks
      key: bundle.jks
      password: jks-password
  - secret:
      format: pkcs12
      key: bundle.p12
      password: pkcs12-password
erikgb commented 9 months ago

@hazmat345 Thanks for your input. I like your suggestion. It's probably better than what I suggested initially in this issue! 😄 We use kustomize a lot, and patching arrays could be troublesome. What if we separated configMap from secret to have two arrays, but used your suggestion with a format field? That would allow us to use key as mapKey when giving the arrays map semantics.

I agree that sources and target should be aligned in the API, but we could also adjust the sources part when breaking the API.

erikgb commented 9 months ago

Inspired by the suggestion from @hazmat345, I think the following might work well:

target:
  configMap:
    - format: pem
      key: root-certs.pem
    - format: jks
      key: bundle.jks
      password: jks-password
    - format: pkcs12
      key: bundle.p12
      password: pkcs12-password
  secret:
    - format: pem
      key: root-certs.pem
    - format: jks
      key: bundle.jks
      password: jks-password
    - format: pkcs12
      key: bundle.p12
      password: pkcs12-password

That will allow us to set map semantics on the arrays in configMap and secret fields, with key as the map key. With this structure, the Bundle OpenAPI spec will validate the fact that key must be unique in a single target (configMap or secret).

SgtCoDFish commented 9 months ago

I have a few things in mind writing this comment:

  1. Most languages and libraries will want a PEM bundle, so by extension most users will want a PEM bundle.
  2. I'm not sure what the value would be of outputting multiple configmaps or secrets. By definition they'll include the same data, just maybe in different formats. I'm not saying "definitely not", but I'd like to understand why because this would complicate our controller logic and working with YAML arrays is, as Erik says, much more difficult than the map we have today.

With that in mind, I'd tend towards something like this:

target:
  configMap:
    formats:
      jks:
        key: "bundle.jks"
        password: "whatever"
  secret:
    formats: {}

In this example, the configMap gets just JKS while the secret will just default to a PEM bundle with some const key that we can define (let's say bundle.pem).

This keeps it easy to write resources (since I assume people will want a PEM bundle) while allowing more flexibility for those who want it.

This could be expanded to support multiple configmaps / secrets per bundle if we wanted (but as above, I'm not feeling convinced on that).

Just throwing the idea out there!

erikgb commented 8 months ago

I agree that we should not support multiple configmaps/secrets. The increased controller complexity is just not worth it. Users that want/need this, can simply create additional bundle resources using the same sources.

But when it comes to specifying the target resource content, I don't think we should have a "magic" default target key for PEM bundles. I think this will bite us at some point in the future. 😁 I agree that PEM is probably the most common format, so it should be easy to configure it. Wouldn't the following be easy enough to represent your example above?

target:
  configMap:
    - format: jks
      key: bundle.jks
      password: whatever
  secret:
    # PEM is the default format
    - key: bundle.pem
dhess commented 8 months ago

We'd welcome multiple keys per target for the same encoding, in any case — many operators and controllers want to read self-signed CA certificates from a secret, almost always PEM-encoded, but often with different key names (cert, crt.pem, etc.).