Only add cluster role permissions for enabled sources

[haines]

Following up on a previous discussion, this PR changes the cluster role permissions to only include permissions for enabled sources. This doesn't require any additional configuration by the end user, because we know which permissions need to be added based on the sources selected in .Values.sources.

We run external-dns with only one source (istio-gateway), so we don't need to grant it permission to access endpoints, ingresses, nodes, pods, or services; the existing setup in the chart grants these permissions regardless.

I think it's beneficial to follow principle of least privilege here, and to be consistent with the setup for the Istio sources.

[stevehipwell]

@haines have you tested this locally? It looks good to me but I'd like to know that if a source is set that it'll continue working.

[haines]

@stevehipwell I've reviewed the code for those sources and found that some of them have informers for multiple resource kinds, so the permissions have some degree of overlap. See what you think - I haven't attempted to deduplicate (so if you use both pod and service sources, you'll have two rules granting pod and node permissions).

I've also tested all the sources on a local cluster with the updated chart and they are working correctly in dry-run mode.

[stevehipwell]

@haines I thought that might be the case. Wouldn't it be clearer to only have each rule once but have it enabled via an or?

For example.

{{- if or (or (has "node" .Values.sources) (has "pod" .Values.sources)) (has "service" .Values.sources) }}
  - apiGroups: [""]
    resources: ["nodes"]
    verbs: ["list","watch"]
{{- end }}

[haines]

Wouldn't it be clearer to only have each rule once but have it enabled via an or?

Personally, I found it clearer to just have a separate block for each source; I find those or chains a bit hard to scan, so it's difficult to see at a glance "if I enable the X source, this grants these additional permissions to the cluster role".

What about including a comment in the generated manifest to show those blocks, so the template can be easier to read but also produce less confusing output?

So something like

{{- if has "ingress" .Values.sources }}
  # permissions for "ingress" source
  - apiGroups: ["extensions","networking.k8s.io"]
    resources: ["ingresses"]
    verbs: ["get","watch","list"]
{{- end }}

{{- if has "istio-gateway" .Values.sources }}
  # permissions for "istio-gateway" source
  - apiGroups: ["networking.istio.io"]
    resources: ["gateways"]
    verbs: ["get","watch","list"]
{{- end }}

...

[stevehipwell]

@haines the chains add a bit of complexity for chart development, but result in much cleaner manifests for the end users. I'd much prefer the pattern that results in clearer outputs for the users.

[haines]

@stevehipwell done! I think it's pretty readable in the end - I realised the or function can take many arguments, eliminating the need to juggle parentheses.

I'll submit a PR to the Helm docs which incorrectly state that or takes two arguments. (update: helm/helm-www#1232)

[stevehipwell]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haines, stevehipwell

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• charts/OWNERS [stevehipwell]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[stevehipwell]

@Raffo I'll leave you to do the LGTM.

/assign @Raffo

[Raffo]

/lgtm

[stevehipwell]

@Raffo the release process worked correctly!! 🥳

[Raffo]

That is very good to hear!