Don't sync targets to all namespaces by default about trust-manager HOT 7 OPEN

@hawksight I think #58 would solve the multi-tenant use cases much better. It is a feature I would love to see implemented. We already use the referenced Openshift feature a lot to inject the cluster CA into configmaps. It would represent a more pull-based workflow, as opposed to the push-based approach supported by trust-manager.

I don't think we should support wildcards in the namespace selector as it would increase the complexity of controller code. To select all namespaces, a user should use the empty selector IMO:

spec:
  target:
    namespaceSelector: {}

Similar to how you configure networkpolicies, ref. https://kubernetes.io/docs/concepts/services-networking/network-policies/#allow-all-ingress-traffic

Also Kubernetes injects a label on all namespaces with the namespace name. So if we are not going for wildcard support, the matchNames is not really required.

kind: Namespace
apiVersion: v1
metadata:
  name: egb-test-cert
  labels:
    kubernetes.io/metadata.name: egb-test-cert

from trust-manager.

So this change basically only means, a user would need to specify the spec.target.namespaceSelector field with {}, rather than omitting the key which can be done at the moment?

Yes, I consider this change to be just making the namespaceSelector a required field. So a user who wants to sync to all namespaces has to configure this explicitly.

from trust-manager.

Yeah I guess that'd make sense! This is something I assume we'd change in v1alpha2 or some other new API version. I'll add to the milestone!

I have a couple of other suggestions for a new API version, so if you are ready for it, I can start on the job. I just finished introducing a new API version in https://github.com/cybozu-go/accurate, so my mind is full of experiences.

from trust-manager.

@SgtCoDFish I think it could make sense to add this to the v1 milestone.

from trust-manager.

Yeah I guess that'd make sense! This is something I assume we'd change in v1alpha2 or some other new API version. I'll add to the milestone!

from trust-manager.

The use case certainly swayed me here, but I would like to think about the usability element. Thinking of cert-manager Certificate spec, there are "default" values that often people want to change but cannot (easily). It might well be easier in the long run to have a configurable default that allows for easier roll out in many use cases.

So in the example above, what if you are not a multi tenant cluster?
What if you just want the current default behaviour to sync everywhere?
Currently that would involve:

spec:
  target:
    namespaceSelector:
      matchLabels:
        key: value

Plus labelling all the applicable namespaces. (Which could be many) k label ns test key=value.

That seems like a potential chore for people, so I would like to try and make it easier for non multi-tenant environments.
Perhaps a good start would be to have a namespace list option along with labels? Similar to CertificateRequestPolicies

FIELDS:
  matchLabels	<map[string]string>
    MatchLabels is the set of Namespace labels that select on
    CertificateRequests which have been created in a Namespace matching the
    selector.

  matchNames	<[]string>
    MatchNames are the set of Namespace names that select on CertificateRequests
    that have been created in a matching Namespace. Accepts wildcards "*".

Something like:

spec:
  target:
    namespaceSelector:
      matchNames: [*]

This negates the need for namespace labelling so it is less work to implement at least.
A follow on might be that a default matchLabels or matchNames value could be supplied at installation of trust-manager? (not sure how this looks) So the idea being you can choose a default value to apply when no field is supplied on each resource. (Yes this sounds like Kyverno or OPA equivalent thing). Also unsure how that plays with the required part.

I'm also wondering if the mutli-tenant scenario actually plays better to the future Bundle (namespaced) resource as opposed to the ClusterBundle (cluster wide) resource we currently have?

• Bundle -> has to be scoped upwards to go beyond namespace?
• ClusterBundle -> Has to be scoped downwards to be applied successfully.

I do agree that with such an important role to play we do need to think of the implications of CA's being presented across a cluster.

from trust-manager.

This is probably a case I should have tried before I commented. @erikgb {} works just as well, good example.

So currently this works:

apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
  name: debian-ca-bundle
spec:
  sources:
  - useDefaultCAs: true
  target:
    configMap:
      key: "ca-certificates.crt"
    namespaceSelector: {}

Example output to all namesapces:

> k get configmap -A -l trust.cert-manager.io/bundle=debian-ca-bundle
NAMESPACE           NAME               DATA   AGE
app-appset-dev-15   debian-ca-bundle   1      99s
app-team-1          debian-ca-bundle   1      100s
app-team-2          debian-ca-bundle   1      100s
app-team-3          debian-ca-bundle   1      99s
default             debian-ca-bundle   1      100s
demos               debian-ca-bundle   1      100s
development         debian-ca-bundle   1      99s
httpbin             debian-ca-bundle   1      99s
ingress-nginx       debian-ca-bundle   1      100s
istio-system        debian-ca-bundle   1      100s
jetstack-secure     debian-ca-bundle   1      100s
kube-node-lease     debian-ca-bundle   1      100s
kube-public         debian-ca-bundle   1      100s
kube-system         debian-ca-bundle   1      100s
staging             debian-ca-bundle   1      100s
test                debian-ca-bundle   1      100s
venafi              debian-ca-bundle   1      100s

Which works just as well as not specifying the key currently:

apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
  name: debian-ca-bundle
spec:
  sources:
  - useDefaultCAs: true
  target:
    configMap:
      key: "ca-certificates.crt"

So this change basically only means, a user would need to specify the spec.target.namespaceSelector field with {}, rather than omitting the key which can be done at the moment? if it remains this way then I can probably retract all my previous words.

I guess the objective here is that the trust-manager user of the Bundle / ClusterBundle resource is forced to make this choice at creation time that it will in fact target all namespaces (when using {}).

from trust-manager.