make service annotations optional when in chartMode aws to avoid deprecated in-tree service annotation

[efossas]

What would you like Teleport to do?

Make service annotations optional when in "aws" chartMode. This could be implemented by using a simple conditional in the values file, such as: aws.defaultServiceAnnotations: true. If this is acceptable, I can write the PR very quickly.

What problem does this solve?

The service annotations currently set when in "aws" chartMode use this annotation:

service.beta.kubernetes.io/aws-load-balancer-type: nlb

https://github.com/gravitational/teleport/blob/master/examples/chart/teleport-cluster/templates/service.yaml#L14

However, that is for the in-tree aws provider which is deprecated. For those of us using the out-of-tree plugin, aws-load-balancer-controller, we need to use this annotation:

service.beta.kubernetes.io/aws-load-balancer-type: external

https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.3/guide/service/annotations/#lb-type

If a workaround exists, please include it.

It's painful. Set the needed service annotations using the annotations.service values field. Set chartMode: custom. Create the ConfigMap for teleport.yaml manually and ensure it is named correctly, then deploy the Helm chart.

[hugoShaka]

This suggestion makes sense. Ideally we would like to support out of the box major cloud providers like aws, gcp and azure but it seems there is no stable way to do it in the kube ecosystem, even within a single cloud provider.

I think we can add this flag and keep the legacy in-tree annotations on by default (at least for a couple of kube versions) because:

• it's a breaking change, should be only be done on a major version
• Our "getting-started on EKS" guide is already terribly complex and it would have to cover the installation of the aws-lb-controller (which is officially an add-on like coredns and the cni, but somehow users have to manually install and maintain it with Helm 🤷 )

@webvictim wdyt ?

[webvictim]

Yep, I agree just adding the flag to disable the default annotations in AWS mode is probably the best way to approach this.

One thing I thought of re: workarounds - given that the user annotations are added after the default ones (https://github.com/gravitational/teleport/blob/master/examples/chart/teleport-cluster/templates/service.yaml#L11-L19) I would have thought that you could just override the annotation and Kubernetes would use the one that was defined last?

annotations:
  service:
    service.beta.kubernetes.io/aws-load-balancer-type: external

[efossas]

Just an fyi, now that I know how teleport works and having needed to create a custom teleport.yaml for agentless, I'm no longer interested in this.

I would suggest creating a directory with examples of provider specific teleport.yaml config in the future and avoid the provider specific "chartMode" all together. From what I've seen in the k8s community, chart maintainers usually regret trying to add cloud specific values in their charts as their tool becomes more popular.

[webvictim]

I would suggest creating a directory with examples of provider specific teleport.yaml config in the future and avoid the provider specific "chartMode" all together.

@efossas This is more or less what we've done in the latest updates to the Helm chart which will be released at the same time as Teleport 12 (which should be in a week or two). There is no longer any need to use custom mode to override parameters - instead you can set chartMode: aws for some sane defaults, then override any Teleport-specific values that you want to change. This should give the best of both worlds.

If you're curious, please see the RFD which has full details of the changes: https://github.com/gravitational/teleport/blob/8a1febc43414894dc587fad6b52fe2f3b1f1e6f2/rfd/0096-helm-chart-revamp.md

[hugoShaka]

Btw I think this issue is solved in v12. If you set service.beta.kubernetes.io/aws-load-balancer-type: external, the chart will honour the annotation and not try to add service.beta.kubernetes.io/aws-load-balancer-type: nlb.