Skip to content
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

Design Discussion: Where do we run the certificate reconciler? #814

Closed
ReToCode opened this issue Jun 8, 2023 · 8 comments
Closed

Design Discussion: Where do we run the certificate reconciler? #814

ReToCode opened this issue Jun 8, 2023 · 8 comments

Comments

@ReToCode
Copy link
Member

ReToCode commented Jun 8, 2023

The plan to realise knative/serving#13472 is, that we use the Knative certificate abstraction to request cluster-local domain certificates. I did a PR for net-certmanager here. As there are some concerns about having a dependency on cert-manager for internal encryption, we discussed extending the current certificate reconciler to also act on internal KnativeCertificate objects (= be an additional implementation to our abstraction) which would be shipped with Knative per default.

Eventing also would like to use the abstraction instead of using cert-manager resources directly (as they do today). From this the question arises, where that reconciler would run. Currently, we do run the certificate reconciler in serving controller which would make Serving a dependency for Eventing. Same applies vice versa and if Eventing also runs that reconciler we might have two concurring reconcilers.

In discussions, so far we though of:

  • Running the reconciler in its own deployment in a separate namespace. Serving + Eventing would depend on it (kind of like cert-manager that would be in `cert-manager).
  • Running the reconciler both in Serving and Eventing and try to do something like cross-namespace leader election.
  • Discuss the concerns again to see if we really need our own internal certificate reconciliation. Especially if we want a solution that works for Serving + Eventing (both together and also independently).

What are your thoughts?
Other ideas are welcome.

/cc @nak3, @pierDipi , @matzew , @dprotaso , @psschwei , @evankanderson , @davidhadas, @KauzClay, @skonto

@nak3
Copy link
Contributor

nak3 commented Jun 9, 2023

Running the reconciler both in Serving and Eventing and try to do something like cross-namespace leader election.

I'm a little bit confused about the reason for needing cross-namespace leader election. Currently, the reconciler in the serving controller reconciles resources that have a serving-specific label (ServingCertName prefix). Will the reconciler in eventing not have a different label to avoid conflicts?

@ReToCode
Copy link
Member Author

ReToCode commented Jun 9, 2023

The reconciler would be responsible for also providing certificates for the cluster-local-domains. Use cases would be

  • Serving resources like Knative Services
  • Eventing resources like Brokers and others

But I think it is a good idea. We could also have the label on them to distinguish between cluster-local-domain certs for Eventing and Serving. WDYT @pierDipi?

@davidhadas
Copy link
Contributor

We can have the reconciler stay in Serving in which case when you deploy Eventing you have the choice of using certmaneger or the option to serving in which case you do not need to deploy certmanager.

@KauzClay
Copy link
Contributor

KauzClay commented Jun 9, 2023

Discuss the concerns again to see if we really need our own internal certificate reconciliation. Especially if we want a solution that works for Serving + Eventing (both together and also independently).

Are the concerns about taking a dependency on cert-manager written anywhere? Just trying to catch up on context, I don't think I have the full picture.

I guess as of now, I would vote for relying on cert-manager for all of our certs. I'm kinda inclined to reduce the amount of stuff we have to maintain.

We already have the kcert resource, can we make Serving and Eventing create kcert objects for any cert they might need, internal or external, and then rely on net-certmanager to reconcile those into cert-manager resources?

I suppose the problems about where it would run would then apply to the net-certmanager controller though. In which case perhaps I'd vote for a new namespace?

Just trying to put some thoughts out there.

@ReToCode
Copy link
Member Author

ReToCode commented Jun 12, 2023

We can have the reconciler stay in Serving in which case when you deploy Eventing you have the choice of using certmaneger or the option to serving in which case you do not need to deploy certmanager.

I don't think that is a good approach. Eventing should not depend on Serving as it is also an independent product. But for Knative as a project it makes sense to rely on one mechanism for certificates (also to have only one CA to trust by user and all components). So it makes sense to also use the abstraction.

Are the concerns about taking a dependency on cert-manager written anywhere? Just trying to catch up on context, I don't think I have the full picture.

I think it was in various places the feature tracks. One is here: https://docs.google.com/document/d/1YdcdBVg_zpT4WSNRsWlihut5JuLddOTIggFvLni3A28/edit?disco=AAAArZbM3yI

We already have the kcert resource, can we make Serving and Eventing create kcert objects for any cert they might need, internal or external, and then rely on net-certmanager to reconcile those into cert-manager resources?

I think that is currently the idea. I'm working on this for internal domains in Serving and @pierDipi is looking into using it in Eventing as well. That's why we have the discussion on where to put the reconciler.

In which case perhaps I'd vote for a new namespace?
Yeah thats one solution I think. But it opens new questions, who/what would deploy these resources? A user can install Serving then Eventing, Eventing then Serving or just Serving, just Eventing. It has to work for all the variants.

Just trying to put some thoughts out there.

👍 Thanks for this. This is exactly what this issue is about.

@KauzClay
Copy link
Contributor

I think that is currently the idea. I'm working on this for internal domains in Serving and @pierDipi is looking into using it in Eventing as well. That's why we have the discussion on where to put the reconciler.

I see, sorry I missed that section of comments in your findings doc.

With that knowledge, instead of the separate namespace, I like what @nak3 and yourself mention above about using labels as well.

@ReToCode
Copy link
Member Author

The approach @nak3 mentioned should work out fine, so closing this.

/close

@knative-prow knative-prow bot closed this as completed Aug 30, 2023
@knative-prow
Copy link

knative-prow bot commented Aug 30, 2023

@ReToCode: Closing this issue.

In response to this:

The approach @nak3 mentioned should work out fine, so closing this.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants