add v1beta1 API version

[munnerz]

What this PR does / why we need it:

Adds the v1beta1 API version.

Special notes for your reviewer:

This is WIP as I am still implementing actual changes we've described in the google doc for the v1beta1 API.

Opening this now so I can be sure I'm keeping things green with each commit 😄

Release note:

Add `v1beta1` API version

/kind feature
/area api
/hold

[munnerz]

/test pull-cert-manager-e2e-v1-11

[munnerz]

/test pull-cert-manager-e2e-v1-11

[munnerz]

/test pull-cert-manager-e2e-v1-11

[munnerz]

/test pull-cert-manager-e2e-v1-11

[munnerz]

/test pull-cert-manager-e2e-v1-11

[meyskens]

This is funny.... is this enforced somewhere? I remember being able to set it lower than this (which caused a disaster), i don't remember it being enforced in the validating webhook.

[munnerz]

That's a good question.. I'm actually not too sure. I think it may not be, and it's also quite a difficult thing for us to enforce given the CA can override the behaviour.

All of cert-manager code needs to tolerate any duration, and can't really rely on this being true.

That said, supposing a CA returns a Certificate valid for 1s, or a user requests a certificate valid for 1s, what would we really want to happen here? That's an excessively short duration, and will lead to cert-manager performing lots of certificate requests.

As a cluster administrator, I'd like to have some guard against this. Perhaps a flag on the controller which is a universal minimum value? (could be enforced by the webhook then?).

How we do handle the case where a user doesn't explicitly set a duration, yet the CA still returns one valid for 1s? Should we go into that tight request loop despite the minimum duration not being met?

Thinking even further.. what if a CA returns a certificate that is already expired?

[wallrj]

It is used in the validation functions:

git grep MinimumCertificateDuration
pkg/apis/certmanager/v1alpha2/const.go: MinimumCertificateDuration = time.Hour
pkg/apis/certmanager/v1alpha3/const.go: MinimumCertificateDuration = time.Hour
pkg/internal/apis/certmanager/validation/certificate.go:        if duration < cmapiv1alpha2.MinimumCertificateDuration {
pkg/internal/apis/certmanager/validation/certificate.go:                el = append(el, field.Invalid(fldPath.Child("duration"), duration, fmt.Sprintf("certificate duration must be greater than %s", cmapiv1alpha2.MinimumCertificateDuration)))
pkg/internal/apis/certmanager/validation/certificate_test.go:                   errs: []*field.Error{field.Invalid(fldPath.Child("duration"), usefulDurations["half hour"].Duration, fmt.Sprintf("certificate duration must be greater than %s", cmapiv1alpha2.MinimumCertificateDuration))},

...but I also remember setting duration lower than this in the nginx workshop, so is the validation broken?

Or, perhaps we never set the duration and relied instead on the TPP policy to set a duration.

• https://github.com/jetstack/cert-manager-nginx-plus-lab/blob/master/pingpong.yaml

[munnerz]

Opened #3067 to track this

[meyskens]

@wallrj venafi doesn't support setting the duration so yes we did there, however i tested this with the CA issuer before. Was before experimental controllers so cannot tell how it behaves now.

[munnerz]

/test all

[meyskens]

i don't think we should validate these.

The source of these comes from the API not the user, the use case for validation is minimal. But that doesn't fully make the case.

"tls-alpn-01" already makes a point here that the spec allows to add more validation methods. cert-manager misbehaves is it find a challenge type that isn't in this list as the API is not able to store it as it is "invalid", this causes all old cert-manager versions to not be able to issue certificates the moment let's encrypt (or another ACME issuer) adds any new validation type

[meyskens]

i found this out the hard way in 799d253

[munnerz]

I am not so sure - our own controllers only support a limited set of types, so I was considering also modifying the controller as part of this PR to reject unknown challenge types too.

Whilst tls-alpn-01 does demonstrate new ones can be added, we don't have an extensibility story that will actually allow these to be implemented just yet. I feel like we'd be better to leave the allowed values as http-01 and dns-01 only and then throw anything that isn't recognised away? Not sure 🤷‍♂️

If, for example, a user creates a Challenge resource manually and specifies type as NOT_REAL, it'd be far more useful if that create call fails and says "only HTTP-01 and DNS-01 allowed"

WDYT?

[meyskens]

I am pro limiting these to the ones we support where our controller is fully tested for that behavior.

[meyskens]

Maybe worth expanding that these 2 are supported by cert-manager but other values exist

[munnerz]

👍 , but let's keep convo in #3038 (comment)

[meyskens]

Same on the two types here

[meyskens]

One day I am gonna blog about Netscape + cert-manager

[meyskens]

@munnerz just the acme things then lgtm

[munnerz]

/test pull-cert-manager-e2e-v1-18

[meyskens]

/lgtm

[munnerz]

/unhold