Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support annotations for ping services in the Helm chart #1491

Closed
TBBle opened this issue Apr 24, 2020 · 5 comments · Fixed by #1520
Closed

Support annotations for ping services in the Helm chart #1491

TBBle opened this issue Apr 24, 2020 · 5 comments · Fixed by #1520
Labels
area/operations Installation, updating, metrics etc good first issue These are great first issues. If you are looking for a place to start, start here! help wanted We would love help on these issues. Please come help us! kind/feature New features for Agones
Milestone

Comments

@TBBle
Copy link
Contributor

TBBle commented Apr 24, 2020

Is your feature request related to a problem? Please describe.

Running up on AWS, we want to put an annotation on the LoadBalancer services to use NLB instead of ELB. This is necessary for udp (#1148) although not sufficient.

This also lets us apply annotations for external-dns to get nice auto-registered DNS entries for the ping services, from the Helm config instead of later.

Describe the solution you'd like

Being able to do this in values.yaml

agones:
  ping:
    udp:
      annotations:
        external-dns.alpha.kubernetes.io/hostname: agones-udp-ping.example.com
        service.beta.kubernetes.io/aws-load-balancer-type: nlb
    http:
      annotations:
        external-dns.alpha.kubernetes.io/hostname: agones-http-ping.example.com
        service.beta.kubernetes.io/aws-load-balancer-type: nlb

This should be a reasonably simple change in the Helm chart, and is pretty common in other charts. We just need to copy the blob of YAML in the annotations value into the Service.

Describe alternatives you've considered

The workaround, after installation, is to use kubectl annotate. Although it seems from #1148 that this won't work for service.beta.kubernetes.io/aws-load-balancer-type, and I haven't yet tested it with external-dns.alpha.kubernetes.io/hostname

Additional context

I haven't looked, but it's probably generally-useful to be able to do this for Services in a Helm chart. Allocator would definitely benefit too (also being a LoadBalancer by default), although I have turned that off in my deployment here.

@TBBle TBBle added the kind/feature New features for Agones label Apr 24, 2020
@aLekSer
Copy link
Collaborator

aLekSer commented Apr 24, 2020

Hello, nice to hear that there is a workaround for UDP, as this was the main issue left on EKS.

@markmandel markmandel added area/operations Installation, updating, metrics etc good first issue These are great first issues. If you are looking for a place to start, start here! help wanted We would love help on these issues. Please come help us! labels Apr 24, 2020
@aLekSer
Copy link
Collaborator

aLekSer commented Apr 30, 2020

Which Kubernetes version are you using? Can you please also post your Instance type?
I have started adding this to helm chart. Testing this with Terraform. Currently got the same issue:

Annotations:              external-dns.alpha.kubernetes.io/hostname: agones-udp-ping.example.com
                          service.beta.kubernetes.io/aws-load-balancer-type: nlb
Selector:                 agones.dev/role=ping
Type:                     LoadBalancer
IP:                       172.20.248.98
Port:                     udp  50000/UDP
TargetPort:               8080/UDP
NodePort:                 udp  31112/UDP
Endpoints:                10.0.4.201:8080,10.0.5.101:8080
Session Affinity:         None
External Traffic Policy:  Cluster
Events:
  Type     Reason                  Age                From                Message
  ----     ------                  ----               ----                -------
  Normal   EnsuringLoadBalancer    15s (x4 over 50s)  service-controller  Ensuring load balancer
  Warning  SyncLoadBalancerFailed  15s (x4 over 50s)  service-controller  Error syncing load balancer: failed to ensure load balancer: Only TCP LoadBalancer is supported for AWS ELB     

Ideas why it still wanted to create ELB is that I used T2 instance that might not support NLB. Now I am using Kubernetes 1.15 cluster.

@aLekSer
Copy link
Collaborator

aLekSer commented Apr 30, 2020

There is a PR and an issue to track exact that annotation:
kubernetes/kubernetes#80394
kubernetes/kubernetes#79523

@TBBle
Copy link
Contributor Author

TBBle commented May 7, 2020

Sorry, I probably shouldn't have included the NLB annotation in the udp-ping block example, since that's still blocked by #1148, which may end up being documented away, and upstream kubernetes/kubernetes#79523.

Although NLB is nice-to-have, the more-important use-case for me is service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags as we use tags to assign AWS costs to projects, and my Agones load-balancer currently falls into the "Work it out by hand" bucket.

@aLekSer
Copy link
Collaborator

aLekSer commented May 7, 2020

I have created initial bool version to support initial annotations here (enable/disable):
https://github.com/aLekSer/agones/tree/helm-service-annotations
But I will rewrite it to support general annotations (not only two listed in the description) for Ping services.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operations Installation, updating, metrics etc good first issue These are great first issues. If you are looking for a place to start, start here! help wanted We would love help on these issues. Please come help us! kind/feature New features for Agones
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants