Cert-manager: ClusterIssuer CRD validation problems [v0.11.0]

[whereisaaron]

Describe the bug:

1. When validating a ClusterIssuer this should be allowed:

spec:
  acme:
    solvers:
    - http01: {}

instead you are required by the validation to add ingress even though ingress has no required properties, e.g. the below will validate, but the above won't.

spec:
  acme:
    solvers:
    - http01:
        ingress: {}

1. When validating a ClusterIssuer, invalid properties are allowed for solvers. I typo'd a selector as below (dnzZones instead of dnsZones), but the CRD still validated and was created.

spec:
  acme:
    solvers:
    - selector:
        dnzZones:
        - example.com
      dns01:
        ...

Expected behaviour:

1. For ingress property to not be required.

2. For selector properties to be validated.

Steps to reproduce the bug:

Try to create resources with the examples above.

Anything else we need to know?:

v0.11 combined with IAM Server Accounts is looking pretty awesome 😄

Environment details::

• Kubernetes version (e.g. v1.10.2): 1.14.6
• Cloud-provider/provisioner (e.g. GKE, kops AWS, etc): EKS
• cert-manager version (e.g. v0.4.0): v0.11.0
• Install method (e.g. helm or static manifests): helm 3

/kind bug

[munnerz]

So we actually can't make the first example valid:

spec:
  acme:
    solvers:
    - http01: {}

The extra ingress sub-field was added because we will support additional options here in future (i.e. webhook, ingressgateway, httpproxy (for contour)) etc.

Whilst there is _currently_ only one, we can't always assume this to be true and so defaulting here would mean we'll leave a confusing API with inconsistent defaulting behaviour in future.

Because ingress is a pointer field in golang, its presence indicates that you want to use the HTTP01 ingress based solver.

---

In this example:

    - selector:
        dnzZones:
        - example.com

There are a couple of reasons this is currently accepted and not obvious what's going on without spotting the typo:

1) We currently do not set crd.spec.preserveUnknownFields: true in our CustomResourceDefinition resources. This means that kubectl does not fail validation if an unknown field name is submitted. This is why you didn't receive an error that you'd spelt the field name wrong.

2) An empty selector is _also_ a valid configuration, i.e. selector: {}. Because dnzZones is not a recognised field name, internally cert-manager sees this as an empty selector, which will therefore match everything. This is why cert-manager itself didn't, and cannot really, complain that you've provided an invalid configuration. It's simply not aware that you specified any other fields.

We can solve (1) in future by enabling this option, but we need to validate our openapi schema is complete before we can. I'll put in a PR to do this that we can try and land at the start of the v0.13 release cycle so we have plenty of time to gather feedback 😄

v0.11 combined with IAM Server Accounts is looking pretty awesome 😄

🎉 😄

[munnerz]

FWIW, this work has happened as part of v0.12! #2365