Consider how to consistently configure Serving and Eventing encryption issuers

[pierDipi]

Problem

We need to think about how to consistently configure Eventing and Serving encryption issuers when users want to change the issuer used.

Serving has this configuration into a ConfigMap while Eventing ships with actual cert-manager issuers, so the usual spec.config wouldn't work for both and ideally we have a consistent API for both

Persona: Which persona is this feature for? Administrator

Exit Criteria A measurable (binary) test that would indicate that the problem has been resolved.

Align the configuration for serving (net-certmanager) and Eventing issuers

Additional context (optional)

/area API

[knative-prow[bot]]

@pierDipi: The label(s) area/api cannot be applied, because the repository doesn't have them.

In response to [this](https://github.com/knative/operator/issues/1621): > > >**Problem** > >We need to think about how to consistently configure Eventing and Serving encryption issuers when users want to change the issuer used. > >Serving has this configuration into a ConfigMap while Eventing ships with actual cert-manager issuers, so the usual `spec.config` would work for both and ideally we have a consistent API for both > >**[Persona:](https://github.com/knative/eventing/blob/main/docs/personas.md)** >Which persona is this feature for? >`Administrator` > >**Exit Criteria** >A measurable (binary) test that would indicate that the problem has been resolved. > >Align the configuration for serving (net-certmanager) and Eventing issuers > >**Additional context (optional)** > >/area API > 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.

[ReToCode]

Thank you for creating this. I'm going to bring this to the next Serving WG meeting.

[ReToCode]

This is also related to https://github.com/knative/operator/issues/950 and probably should happen in the same effort.

[pierDipi]

We have a few options:

Option 1

KnativeEventing reconciler will special case spec.config.certmanager and spec.config.config-certmanager, so that both CRs will look consistent:

kind: KnativeServing
spec:
  config:
    certmanager:
      clusterLocalIssuerRef: |
         kind: ClusterIssuer
         name: knative-internal-encryption-issuer

kind: KnativeEventing
spec:
  config:
    certmanager:
      clusterLocalIssuerRef: |
         kind: ClusterIssuer
         name: knative-internal-encryption-issuer

Pros

• Consistent APIs
• Single documentation page to describe operator encryption configuration

Cons

• We cannot ever add a config-certmanager ConfigMap to Knative Eventing
• YAML in YAML is a little weird

Option 2

Top level field (just an example, we consider a deeper structure) spec.encryption so that both CRs will look consistent but the reconciler logic would be different:

kind: KnativeServing
spec:
    encryption:
      clusterLocalIssuerRef:
         kind: ClusterIssuer
         name: knative-internal-encryption-issuer

kind: KnativeEventing
spec:
    encryption:
      clusterLocalIssuerRef:
         kind: ClusterIssuer
         name: knative-internal-encryption-issuer

Pros

• Consistent APIs
• Single documentation page to describe operator encryption configuration
• No YAML in YAML

Cons

• It breaks the pattern of spec.config configuring ConfigMaps
• KnativeServing reconciler needs to prioritize spec.encryption over spec.config.certmanager if someone configures both (but it's not clear why someone would do that)

Option 3

2 different APIs, so not achieving the original issue goal of having consistent APIs:

kind: KnativeServing
spec:
  config:
    certmanager:
      clusterLocalIssuerRef: |
         kind: ClusterIssuer
         name: knative-internal-encryption-issuer

kind: KnativeEventing
spec:
    encryption:
      clusterLocalIssuerRef:
         kind: ClusterIssuer
         name: knative-internal-encryption-issuer

Pros

• It follows the pattern of spec.config configuring ConfigMaps only

Cons

• Inconsistent APIs
• Multiple documentation pages to describe operator encryption configuration

@dprotaso @matzew @ReToCode @skonto can you provide your input please?

[pierDipi]

I would personally prefer Option 2

[pierDipi]

cc @rhuss

[skonto]

In general my pov is that probably we are doing too much anyway as most projects (see for example here and here ) just have as an interface a secret in order to consume CAs (intermediate or not)/tls certs. Operator could automate the scenario where we use cert-manager and core Serving, Eventing should just implement encryption with the assumption that all certificates are already distributed. Certs trust/distribution should be decoupled as a problem imho. That means at the Serving, Eventing CR level there should be only one flag, enable/disable native encryption. The issuer stuff belong elsewhere imho. Tbh even net-certmanager and the Serving certificate abstraction is too much as we don't have many implementations and we could even simplify our design more without it. In any case the operation aspect of the design we agreed upon here should be done either manually or by the operator. The latter means that shipping issuers can be avoided.

In detail here are my comments for the options in detail (if we have to choose one):

Option 1: There are many examples (I can list some) out there of passing small yaml data (our yaml is tiny here) in a cm with some drawbacks like readability etc, so people are familiar anyway. Not sure why Eventing cannot add a cm. Net-certmanager could just read more than one cms anyway, one for Eventing and one for Serving. There are more options when it comes to how to encode the issuer info assuming there is no pressure to use yaml within the cm. Since this is cert-manager we could simply have (no need to specify the kind) in the cm (normal attributes):

kind: KnativeServing
spec:
  config:
    certmanager:
      cluster-system-internal-issuer-ref: <name>   # cluster issuer or
      system-internal-issuer-ref: <name>          # ns issuer

This just uses ordinary cm key:val (it is a dictionary after all). It requires a change at the net-certmanager but that is not a problem as that component is not that widely used afaik (that change can be easily planned).

Option 2:

Leaking certmanager semantics at the top level spec is not that good imho. Not sure if that kind of info belongs to the spec. However, if it is added at the top level spec some parts need to be refined eg. indicate somehow that the encryption field refers to cert manager (otherwise it looks arbitrary to me).

Option 3: It could be a short-term solution but UX does not look good. Also see option 2 for the Eventing side.

[ReToCode]

Basically, I can live with all of them, here my thoughts:

Option 1:

• Is a hack in operator, but result has a nice UX.
• YAML in YAML is a problem, but we have that everywhere, so if we want to change this, do it for all cases.

Option 2:

• We add a different config pattern just for certmanager. I'm not sure if this outweighs the benefit of having a consistent config.

Option 3:

• I probably go with that, as I don't think its a problem to have a different configs. Eventing and Serving are two pretty separated things anyway.

[pierDipi]

Another option, that would work for Eventing, no operator config and instead we would require the presence of a well-known ClusterIssuer called something like knative-eventing-ca-issuer that would need to be pre-created before enabling the feature.

[skonto]

@ReToCode should we close this one? cc @pierDipi