fix(service)!: align syntax with upstream

[parkerfath]

What does this PR do?

Changes the indent and order of a few values.yaml items under service.spec to clarify which items go inside the "spec" field and which items are directly under "service."

Before

service:
  type: LoadBalancer
  loadBalancerSourceRanges:
    - 192.168.0.1/32
  externalIPs:
    - 1.2.3.4
  ipFamilyPolicy: SingleStack
  ipFamilies:
    - IPv6
  spec:
    externalTrafficPolicy: Cluster

After

service:
  spec:
    type: LoadBalancer
    externalIPs:
      - 1.2.3.4
    externalTrafficPolicy: Cluster
    ipFamilyPolicy: SingleStack
    ipFamilies:
      - IPv6
    loadBalancerSourceRanges:
      - 192.168.0.1/32

Motivation

See #1680 - I tried uncommenting and setting externalTrafficPolicy but it was silently ignored.

More

• Yes, I updated the tests accordingly
• Yes, I updated the schema accordingly
• Yes, I ran make test and all the tests passed

(No test or schema change needed, but I did run the tests.)

[mloiseleur]

🤔 In fact, we should not have also type, loadBalancerSourceRanges and externalIPs. And there is the same issue for the additionalServices section.

Like all the others, there are all subparameters of Service's spec. For consistency and simplicity, they all should be set in service.spec (except selector and ports entries, that should be computed from related values)

This is not only in values, we should also remove nearly all the code (and just keep what the user set on .spec) here:

https://github.com/traefik/traefik-helm-chart/blob/master/traefik/templates/_service.tpl#L22-L50

And also, to avoid values.yaml growing too much, we've moved the examples to EXAMPLES.md

I know it's breaking but sometimes it can be better in the long term.
This brings two questions:

1. Wdyt of this approach?
2. Do you think you can try to implement it? Or do you prefer that we update your PR?

[parkerfath]

🤔 In fact, we should not have also type, loadBalancerSourceRanges and externalIPs. And there is the same issue for the additionalServices section.

Like all the others, there are all subparameters of Service's spec. For consistency and simplicity, they all should be set in service.spec (except selector and ports entries, that should be computed from related values)

This is not only in values, we should also remove nearly all the code (and just keep what the user set on .spec) here:

https://github.com/traefik/traefik-helm-chart/blob/master/traefik/templates/_service.tpl#L22-L50

And also, to avoid values.yaml growing too much, we've moved the examples to EXAMPLES.md

I know it's breaking but sometimes it can be better in the long term. This brings two questions:

1. Wdyt of this approach?
2. Do you think you can try to implement it? Or do you prefer that we update your PR?

Hmm I think I see what you're getting at, but not sure I have enough context to implement. Would it be something like this? https://github.com/parkerfath/traefik-helm-chart/commit/3a6136191934d6f737b863c423745425d72901c5

[mloiseleur]

Yes.
There is also this helper to rework. It should only set .service.spec. And there are also tests to update accordingly to this new behavior.

[parkerfath]

I am interested, but might not be able to carve out the time to do it quickly (not to mention this is my first contribution to a helm chart). So, if it's high priority you can feel free to take it off my hands, otherwise, I'll see what I can do with my free cycles.

[mloiseleur]

@parkerfath I updated the PR with the simplified syntax. It comes with less code to maintain and should avoid any future confusion this. Since it's a breaking change, I updated PR description with a before / after block.

[jnoordsij]

Maybe we could use (merge . (dict "type" "LoadBalancer")) here (if the top-of-my-head Helm skills suffice) to at least retain the previous default? Otherwise requiring people to provide this now is also not too much of a breaking change I'd say.

[mloiseleur]

All the other moves like this, we failed gracefully (with schema) and did not support it, so the user can update it.

🤔

With current schema verification, it will fail anyway before this merge is executed.
So I think we can stay like that. It's only a parent change.