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

Review of API schema #51

Closed
munnerz opened this issue Aug 8, 2017 · 9 comments
Closed

Review of API schema #51

munnerz opened this issue Aug 8, 2017 · 9 comments

Comments

@munnerz
Copy link
Member

munnerz commented Aug 8, 2017

Right now our API schema is relatively flexible. We only define a single external version, v1alpha1, and we give no guarantees on the stability of this API.

Now that we're at a point where the project does function, we should review our schema to make sure it's consistent and makes sense. This specifically relates to the 'Issuer' and 'Certificate' resource type.

An example for ACME can be found here: https://github.com/jetstack-experimental/cert-manager/blob/master/docs/acme-cert.yaml and here: https://github.com/jetstack-experimental/cert-manager/blob/master/docs/acme-issuer.yaml. The full type definitions can be found in types.go.

@munnerz munnerz added this to the Initial alpha milestone Aug 8, 2017
@munnerz
Copy link
Member Author

munnerz commented Aug 8, 2017

@mikebryant
Copy link
Member

Currently Certificate.spec.issuer is required.
will it be possible to have a default issuer? Or select the issuer by policy?

For example, dns providers will often be set up to only work for names under a single subdomain (e.g. the route53 provider, having the credentials to a single zone)

It's more burden on users if they have to select a specific issuer. Instead could the system figure it out? (e.g. if the Issuer is set up to know it can handle any dns challenge under *.example.com, it can act on any relevant Certificate objects that haven't made a specific choice?

Along similar lines, if Certificate.spec.acme.config is empty will it be able to issue the certificate, using a default challenge provider?

@munnerz munnerz moved this from Short-term to Medium-term in Kubernetes Incubator Sep 9, 2017
@munnerz
Copy link
Member Author

munnerz commented Sep 19, 2017

Hey @mikebryant - sorry this message slipped through my inbox.

So right now, no it's not possible. It is something that I want to support however, it's just the details of how we do it that are complex. This is a blocker for #19, unless we start exposing more API surface through Ingress annotations.

Open questions:

  • Where should the designation for 'default' exist? We could follow the storage class pattern and have an is-default annotation on Issuers? Alternatively, it could exist as an annotation on the Namespace itself.

  • If we go the annotation route, what happens if two Issuers specify themselves as default?

  • How does this inter-play with non-namespaced issuers (Non-namespaced Issuers #91)? Ideally, a user should be able to specify a namespace default as well as a global one.

  • Should this be done via an Initializer (i.e. admission controller), so that Certificate resources ultimately do have the Issuer field set, it's just set automatically set by the apiserver through introspection of cluster state? The alternative is to determine the Issuer to use each time the resource is processed, however that means that the Issuer could actually change over time, causing issues with renewal.

  • Regarding specifying Certificate.spec.acme.config - so this field is tied to the ACME implementation, and others don't have it. This means that not all issuing backends will have additional config on the Certificate, and thus means we cannot generalise the fields needed to specify a default. I'm open to ideas on how this should be approached, but have a feeling we should implement default ACME config as a separate feature (with it's own design considerations)

I'm personally not a huge fan of Issuers specifying lists of supported domains. It feels like it's over-complicated for provisioning an Issuer, and isn't applicable in all cases (think: ACME HTTP01). It's also further tied to the providers specific config. e.g. the supported domains are actually on a per-challenge provider basis in ACME, which is specific to ACME itself.

@mikebryant
Copy link
Member

  • I support following the StorageClass pattern - annotation on the Issuer itself.

  • Like in StorageClass, the initializer can refuse to process it - https://kubernetes.io/docs/admin/admission-controllers/#defaultstorageclass

  • I would image we use the default in the Namespace if available, if not fall back to the non-namespaced default

  • Yes, initializer sounds good. It also means it's easy for people to opt in/out of this behavious, by not specifying any defaults, or we can provide an option to disable the initializer processing. In particular this means we could also defer functionality relating to picking an issuer based on domain etc, if someone wants this they could potentially write their own initializer to take care of their particular policy details.

  • For ACME config, a simple solution might be to just say - Make the Certificate.spec.acme section optional. If the Issuer it gets routed to only has a single provider defined, it just uses that one. Or we add something like Issuer.spec.acme.dns-01.providers[].is-default. Happy to have a separate discussion on this.

I would be happy to leave the complicated semantics like policy based on lists of domains out etc. If we leave open the possibility to do the autoconfiguration by initializers, that would let anyone with such needs sort it out themselves, without needing to fork or reimplement all the certificate managing stuff.

My use-case is just to have a single default Issuer per cluster, with a single root domain that everything will be subdomains off. But I do want to avoid needing that configuration in the Certificate objects themselves, so I can provide a clean boundary between what users request (A certificate, not caring where it comes from), and the cluster level configuration that provides these things.

@munnerz
Copy link
Member Author

munnerz commented Sep 19, 2017

@mikebryant I've created #97 to track adding a default annotation to Issuers (and supporting Initializer for Certificate resources).

I like the idea of a simple solution regarding picking a challenge provider - I think it'd be great if we didn't require all the extra config, especially in single-Issuer, single-config scenarios. The only edge-case I can see here, is that right now, all ACME issuers are 'configured' for solving HTTP01 challenges (as there's no configuration required). This means it'd be impossible to create a default issuer that always uses DNS01.

If we require a user specify http-01: {} in their Issuer spec, we could then observe the lack of http-01: {} to mean that http01 support is disabled, but once again, this may be confusing for first-time users that only want http-01 challenge support! Any thoughts here? Is this an acceptable requirement that we put on users? The downside of this, is that anyone who doesn't specify http-01 or dns-01 fields will not be able to solve challenges using the 'vanilla' Issuer (i.e. one with no config specified)

+1 for leaving policy stuff out for now. Perhaps in future we can provide a some built in initializers for policy that can be enabled by advanced users.

@mikebryant
Copy link
Member

mikebryant commented Sep 19, 2017

I would prefer to be explicit.
In our environment, the cluster isn't generally accessible to the internet, so I wouldn't want to have http-01 enabled by default, it would just cause confusion.

So definitely +1 on requiring it to be specified to be enabled with http-01: {}

I wouldn't have thought it would be hugely confusing for users, if that's one of the default examples. My expectation would be someone starts using the project by copying and pasting an example Issuer manifest, they're unlikely to type it out from scratch and forget to put in http-01 or dns-01?

@munnerz munnerz modified the milestones: Initial alpha, 0.0.1 Oct 12, 2017
@dippynark
Copy link
Contributor

dippynark commented Oct 13, 2017

@mikebryant We could consider having http-01 as a default only if no other method is defined, but I think that'd cause more confusion if http-01 disappeared as soon as another method was added (for example, after http-01 had already been used for a while for obtaining certs).

So yeah I agree with being explicit in the methods you want to enable

@munnerz
Copy link
Member Author

munnerz commented Oct 13, 2017

I've opened #115 which adds the functionality that @mikebryant and @dippynark have described.

Once merged, an ACME Issuer will require some config on it in order to start issuing certificates. The docs/examples need updating correspondingly.

Unfortunately we don't currently have test coverage on this part of the codebase, but post-0.0.1 I'm going to work on getting Boulder running for tests so we can properly test out the ACME provider.

@munnerz
Copy link
Member Author

munnerz commented Oct 13, 2017

I'm going to close this issue off once #118 is closed as that's the final API change for this release at least.

We should spend some time getting used to the new schema, and we can re-open discussions in a new issue if we need to address anything next time around.

@munnerz munnerz closed this as completed Oct 13, 2017
@munnerz munnerz moved this from Medium-term to Done in Kubernetes Incubator Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

3 participants