Upstream CA Cert filtering fires for unrelated Secrets

[davecheney]

The logic added in beta.1 to filter out anything except kubernetes.io/tls and items with data["ca.crt"] is failing because kubernetes.io/service-account-token's also have a key called ca.crt.

% k get -n kube-system secret endpoint-controller-token-r9rgv -o yaml
apiVersion: v1
data:
  ca.crt: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURERENDQWZTZ0F3SUJBZ0lSQUlKeDFoaFo3REVIMmdXaWp4SU1FZ0F3RFFZSktvWklodmNOQVFFTEJRQXcKTHpFdE1Dc0dBMVVFQXhNa05HTm1PREUyWldVdE5UVm1PQzAwWVdKa0xXSTRaVEl0TmpJMU1EY3hNREV4TmpjMApNQjRYRFRFNE1ERXhOekl6TXpNME1Wb1hEVEl6TURFeE56QXdNek0wTVZvd0x6RXRNQ3NHQTFVRUF4TWtOR05tCk9ERTJaV1V0TlRWbU9DMDBZV0prTFdJNFpUSXROakkxTURjeE1ERXhOamMwTUlJQklqQU5CZ2txaGtpRzl3MEIKQVFFRkFBT0NBUThBTUlJQkNnS0NBUUVBbCtpQTNzQjIrbE9LMTZYM24yR1o1ODQ3a1o3Nzh5c04wTkZkdU44cgp5SkNLUE8vQ0RxSTRoOFhTdTdGL0t6TUZoUDFIRzlxQTczaklIaER6S1ZHZHhBa2MwYi96eHFNNWRxa3BYK1JPClNQVVEyUENnbDRCcWlxRU9yQkp6RDA2RmFuTnNya0cvTzRkdlVTaVpVOGdqeE1id0hibHd1SVhiL0dMbEtqL1kKdk9DME5Vcmxva1FmcG10WVUxZHdCY2RVaFhEQjRWTGw0YlJuL3V1ekhZSWUvMG8wclUwMXVVY09TR0d2TE9YZgpQSWFMTmN0QXdVZXppaEpla0QxZlF3UHJHWTFvQ3JBY3J3N3RDblNBQ3dOTHZrQlNBcXVsUVNTbmN5L1BPS2N5Ci81RlFQbUs0SEo4WjZGeWtFUnQwdWdsR29GTUpvL1o4Mm13djFRK0I5Ykh3N1FJREFRQUJveU13SVRBT0JnTlYKSFE4QkFmOEVCQU1DQWdRd0R3WURWUjBUQVFIL0JBVXdBd0VCL3pBTkJna3Foa2lHOXcwQkFRc0ZBQU9DQVFFQQppQmdiMVpCaFNUTXdpckVVQlNVanI5ZzZuWm9qMGE3SGJsSFlldzYrZDhNNDIzak1PR0wxM0lEM1o4SW5zekQ3ClNMcXdLazgvT1NsR2htYTVPNnNsM0tqY1ZnOTdQcm0vRXNRT3FaZzZwN1lwWFZDQ2V5MXRtcjMrdjBjV1V5dWoKVlhoREtWUWJKeEhxNGorNVZmTnZNTnpyUFJEandhZGVSRCtsakYxV2x2UGswZWZqdXRlM2JjQ01UR2ovaEtieQo1MXA1dHlCRFM5OSs4TkNsd1FncHpBODdhUVVxL0s3RUFUb2phWDZITXlnc3NHV3RoNDEvNmdXMjJEcHQxUEM3Ck9JK0RSK3pjaVUveVNpSjZaRkIvQVlsN2YwREE0alVjajZMWnlRSFhJcUJMWWtPTHQwaGN1a1BxaTZrMXBScW4KdHVRK3F4Qm1VSlBOam5xT09NeWl5QT09Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K
  namespace: a3ViZS1zeXN0ZW0=
  token: ZXlKaGJHY2lPaUpTVXpJMU5pSXNJblI1Y0NJNklrcFhWQ0o5LmV5SnBjM01pT2lKcmRXSmxjbTVsZEdWekwzTmxjblpwWTJWaFkyTnZkVzUwSWl3aWEzVmlaWEp1WlhSbGN5NXBieTl6WlhKMmFXTmxZV05qYjNWdWRDOXVZVzFsYzNCaFkyVWlPaUpyZFdKbExYTjVjM1JsYlNJc0ltdDFZbVZ5Ym1WMFpYTXVhVzh2YzJWeWRtbGpaV0ZqWTI5MWJuUXZjMlZqY21WMExtNWhiV1VpT2lKbGJtUndiMmx1ZEMxamIyNTBjbTlzYkdWeUxYUnZhMlZ1TFhJNWNtZDJJaXdpYTNWaVpYSnVaWFJsY3k1cGJ5OXpaWEoyYVdObFlXTmpiM1Z1ZEM5elpYSjJhV05sTFdGalkyOTFiblF1Ym1GdFpTSTZJbVZ1WkhCdmFXNTBMV052Ym5SeWIyeHNaWElpTENKcmRXSmxjbTVsZEdWekxtbHZMM05sY25acFkyVmhZMk52ZFc1MEwzTmxjblpwWTJVdFlXTmpiM1Z1ZEM1MWFXUWlPaUkzTW1VME5UTTFaaTFtWW1VM0xURXhaVGN0T1RSbVpTMDBNakF4TUdFNU9EQXdNR1FpTENKemRXSWlPaUp6ZVhOMFpXMDZjMlZ5ZG1salpXRmpZMjkxYm5RNmEzVmlaUzF6ZVhOMFpXMDZaVzVrY0c5cGJuUXRZMjl1ZEhKdmJHeGxjaUo5LmZlSkV0M20zVkM0UUtacDd5eWloX082MjVnTG5DZGFraFAxam9aTnNGUktXRnMyV3ZiWjg2ZThpU0xTY3doLW5yTC1wcHNtSk5rRlNnZVpuWVctZ0lkTjBlRDFSN0hJRldzX0RNRXVPM1BST1N6MU01Rl9QUVhmMElOZEs3U3RMM3VTMTBfM2V3cWdsbE5MLUVkNTRSTHN2a0hkeU1BSU11aS01cXlJSkJCYmgySWFQcmdrdW9MUUowWi0wWHJwbGdLWFIzWi04OHFRTktqZ3NjVUhIMFZjclBpcWVXZ0VodXg3ckZwOHBuREVTaWRNVFZXbkV6aHdjaHdsbTdtU2ZQU1hzYm1MNEkxblFZYkFaUnd2SHFKUmdXbmxUay1VVzF5eFRUTlJoMHRxNWw4VUdjTjNTaUJlMEJOdjZVaGJrNWNaMVVkM090emo3N3VjbmdzakJuZw==
kind: Secret
metadata:
  annotations:
    kubernetes.io/service-account.name: endpoint-controller
    kubernetes.io/service-account.uid: 72e4535f-fbe7-11e7-94fe-42010a98000d
  creationTimestamp: "2018-01-18T00:35:14Z"
  name: endpoint-controller-token-r9rgv
  namespace: kube-system
  resourceVersion: "149"
  selfLink: /api/v1/namespaces/kube-system/secrets/endpoint-controller-token-r9rgv
  uid: 72e58c89-fbe7-11e7-94fe-42010a98000d
type: kubernetes.io/service-account-token

A short term fix for this would be to exclude type: kubernetes.io/service-account-token's, but what we probably should do is change the type of the CA cert we use for TLSCertificateDelegation to have our own type, say type: projectcontour.io/ca-certificate.

We should do this for IngressRoute and HTTPLoadbalancer objects.

@stevesloka @youngnick thoughts?

[davecheney]

@jpeach are you working on this as part of #1714 ?

[jpeach]

I wasn't planning on it, but after #1714 we will only take CA bundles from secrets with no type, or of type "opaque" or "kubernetes.io/tls". I think we need to find a place to document this.

[jpeach]

Document this per #1415

[davecheney]

@jpeach thanks for working on #1415. Do you think there is anything more to do on this ticket for rc.2? If not, would you mind closing it or moving to the backlog if there is nothing pressing.

[jpeach]

@davecheney We chatted about this on slack. If we want to require that HTTPProxy validation uses only CA bundles from projectcontour.io/ca-certificate secrets, then we should do that now (preferably for rc2). I'm happy to backlog this, but if we do I don't think we can make the typed secret a requirement.

[davecheney]

Thanks for clarifying. Do you think you will have the bandwidth to tackle this for rc.2?

[jpeach]

Here's my proposal:

1. Retain existing semantics for IngressRoute
2. Add support for a new Secret of type projectcontour.io/ca-certificate. The semantics of this secret are that you can have any Data keys you like, but we will require all the values to contain one or more X509 certificates in PEM format (the PEM type must be "CERTIFICATE").
3. Add type tracking to the cache so that we can enforce the rule that HTTPProxy validation must only consume CA secrets of type projectcontour.io/ca-certificate.
4. Document the above.

I'll ping the plan to the slack channel too.

/cc @mattalberts

[bgagnon]

My only concern is that custom Secret types are not really an established practice in the K8S ecosystem. In fact, even the tls/non-tls dichotomy is mostly convention since ultimately, only the data keys matter.

Have you considered an approach using a label selector instead of a custom type?

• specify a CA secret label selector to Contour
• label secrets accordingly

This can also be extended to specify a namespace (or better, a namespace selector) in which Secrets should be considered. This model is used extensively in Prometheus Operator, for instance. Labels and selectors is also a central design in all things Kubernetes.

This could solve the immediate problems without closing the door on introducing a custom secret type later on.

[stevesloka]

I agree with @bgagnon that changing the type may not be the best solution.

I think the root issue is that changes to a secret cause a DAG rebuild right? Can we apply the same logic as we do with services but to secrets (regardless of the type).

If a secret is updated but not referenced in the DAG then skip rebuilding? This way we don't have to be too restrictive on the types the folks choose to implement.

[jpeach]

Discussed with @davecheney, and clarified that the spurious DAG rebuild is the primary problem that we are trying to address here. I think that we can improve that without introducing new incompatible requirements around the secrets type, so we should drop that proposal.

Moving this to the backlog.