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

Return all advertised Gameserver Addresses #3089

Closed
rcreasey opened this issue Apr 12, 2023 · 22 comments · Fixed by #3307
Closed

Return all advertised Gameserver Addresses #3089

rcreasey opened this issue Apr 12, 2023 · 22 comments · Fixed by #3307
Assignees
Labels
kind/design Proposal discussing new features / fixes and how they should be implemented kind/feature New features for Agones

Comments

@rcreasey
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
The change suggested in #1921 and addressed in #1928 introduced the following order of preference when selecting an address for advertisement on a gameserver resource:

func address(node *corev1.Node) (string, error) {
for _, a := range node.Status.Addresses {
if a.Type == corev1.NodeExternalDNS {
return a.Address, nil
}
}
for _, a := range node.Status.Addresses {
if a.Type == corev1.NodeExternalIP && net.ParseIP(a.Address) != nil {
return a.Address, nil
}
}
// There might not be a public DNS/IP, so fall back to the private DNS/IP
for _, a := range node.Status.Addresses {
if a.Type == corev1.NodeInternalDNS {
return a.Address, nil
}
}
for _, a := range node.Status.Addresses {
if a.Type == corev1.NodeInternalIP && net.ParseIP(a.Address) != nil {
return a.Address, nil
}
}

However, there are use cases where NodeInternalIP might be a more preferable selection for a gameserver's advertised address.

Describe the solution you'd like
Instead of a hard coded order for address selection, I would rather this logic be configurable to fit the specific situation. This doesn't have to be something that is handled on a pod-by-pod basis, but could just be something that's configured for all gameservers on a cluster.

Describe alternatives you've considered
It seems overkill, but it might be cool to have this configuration option be part of the GameserverSpec, or PodSpec.

@rcreasey rcreasey added the kind/feature New features for Agones label Apr 12, 2023
@markmandel markmandel added the kind/design Proposal discussing new features / fixes and how they should be implemented label Apr 12, 2023
@markmandel
Copy link
Member

Would it be okay if it was part of the Agones helm configuration? I figure it's not going to change per GameServer/Fleet? It's probably more tied to your infrastructure and Agones installation?

I was thinking something like:

agones.controller.addressResolution = ["ExternalDNS", "ExternalIP", "InernalDNS", "InternalIP"]

As a helm configuration option (the above would be the default as what we have now), and then you can basically choose whatever keys a Node.Adresses has in it.

Howzat?

@rcreasey
Copy link
Collaborator Author

Yeah, that's totally fine. Having it configured per fleet isn't my use case, but only mentioned it to be flexible.

Just having a toggle to pick how it's going to prioritize who can to address is all I was looking for.

@markmandel
Copy link
Member

I really like this idea - we've had different people want different options since day 1 -- so this makes a lot of sense.

@jeremylvln
Copy link
Contributor

Hi, I have a usecase which could challenge this issue a bit. I would love to have your feedbacks on that.

My game servers are managed by a Fleet, however my players are first connected to an intermediate proxy itself managed by another Fleet. This means that while my proxies can be publicly exposed to my nodes public IPs, my game servers should absolutely not.

Being able to restrict the external/internal exposure will be a necessity for me to move into production. However, you are talking here about choosing the IP to put in a GameServer status. IMO the issue is more: how my GameServer should be exposed to the network?

The safest configuration scenario for me would be to configure Agones with Helm to use the internal network only by default, and manually configure a Fleet to use the external one if needed. For now, I declare an empty ports field to manage the networking myself, but I would love having Agones to do it for me.

@markmandel
Copy link
Member

@IamBlueSlime I'm not sure how this ticket is related to your issue, since it doesn't affect access control to the GameServer - purely what information is returned via GameServer resource records in Kubernetes.

@jeremylvln
Copy link
Contributor

@IamBlueSlime I'm not sure how this ticket is related to your issue, since it doesn't affect access control to the GameServer - purely what information is returned via GameServer resource records in Kubernetes.

This issue is about the ability to select the node IP we want to store in the GameServer status. My point is that it is part of a larger problem I have described in my previous message. I agree with you that it should be a dedicated issue, however as this is still at a design stage, I wanted to raise the point here too.

I will take the time to elaborate more my issue and will open a dedicated one here.

@markmandel
Copy link
Member

I will take the time to elaborate more my issue and will open a dedicated one here.

Perfect - sounds like a plan 👍🏻

@zmerlynn
Copy link
Collaborator

For those watching this issue - if we were to instead implement #957, would that meet your usecase? In other words, if the allocator API and the GameServer object had a full list of addresses, would that work for you?

@rcreasey
Copy link
Collaborator Author

rcreasey commented Jun 13, 2023

It would serve our purposes, yes.

Bonus points if we're able to make an informed decision about the addresses in the list easily (meaning it would be a list of objects that had an identifier or label that would let us know which was the internal, external, ipv6 etc without needing to parse the address before determining type).

@markmandel
Copy link
Member

markmandel commented Jun 13, 2023

https://kubernetes.io/docs/concepts/architecture/nodes/#addresses provides you with a subset of the keys that a Node address usually contains.

It can be more, depending on the cloud provider.

@zmerlynn
Copy link
Collaborator

[...] depending on the cloud provider.

Hmm, that could be a detriment. Right now the GameServer address field is cloud agnostic, but if we return the bag of Node addresses unedited, we risk making that less portable. In practice I'm not sure how different they are, though.

@zmerlynn
Copy link
Collaborator

Just leaving a breadcrumb for the API definition (vs the describe): NodeStatus and NodeAddress. (Was just verifying the API didn't say anything particularly opinionated about what was returned.)

@rcreasey
Copy link
Collaborator Author

https://kubernetes.io/docs/concepts/architecture/nodes/#addresses provides you with a subset of the keys that a Node address usually contains.

It can be more, depending on the cloud provider.

Yeah, I was hoping it'd be a reference to this kind of key as opposed to just a list of strings

@markmandel
Copy link
Member

Yeah, I was hoping it'd be a reference to this kind of key as opposed to just a list of strings

It's a list of {type, address} structs - so it's a list of addresses with keys that explain what they are.

But yeah, it means every time you want something other than the default GameServer address, you have to loop through 'em.

@zmerlynn
Copy link
Collaborator

Yeah, I was hoping it'd be a reference to this kind of key as opposed to just a list of strings

Right. I was thinking it was just a bag of NodeAddress, just copying NodeStatus.addresses, just as #957 discussed.

But yeah, it means every time you want something other than the default GameServer address, you have to loop through 'em.

Someone's got to do it, though. It's either:

  1. Agones, where we would need configuration to express preference, which isn't the end of the world but at the end of the day someone will probably need something different than what we provide.
  2. The client, which knows what it's looking for, generally - but yes, you'd have to write a "find the address I want" function.

@rcreasey
Copy link
Collaborator Author

rcreasey commented Jun 13, 2023

I don't mind looking through them, or writing a "find the address I want" lookup. I just didn't want to have to check the string against a regex to see if it was ipv4/6, then check the subnet to see if it was internal/external, etc.

Having the key available that says "this is the external address value" is what I was wanting. List of structs vs string is perfect.

@markmandel
Copy link
Member

What's tough is - the "ExternalIP" - that could be ipv4 or ipv6 - the Node information doesn't tell us that.

@rcreasey
Copy link
Collaborator Author

What's tough is - the "ExternalIP" - that could be ipv4 or ipv6 - the Node information doesn't tell us that.

Lol, ok. Bad example. Internal/External/Node is sufficient 😜

@zmerlynn
Copy link
Collaborator

What's tough is - the "ExternalIP" - that could be ipv4 or ipv6 - the Node information doesn't tell us that.

It's true. In my head, if we were going to support configuration around the priority, we would need a separate enum for the address family.

That said, in most languages you can do the equivalent of ParseAddr then .Is4() rather than a regexp match - but regexp is fairly straightforward as well.

@markmandel
Copy link
Member

Maybe another interesting question. Can we think of a scenario in which you would want to have access to more than one address type of a GameServer?

@zmerlynn
Copy link
Collaborator

In DNS-land it's a common approach to ask for A and AAAA records at the same time, then the client can do Happy Eyeballs to connect over IPv4 or IPv6, whichever is easiest. I imagine for some scenarios (mobile flipping between IPv6 on cell and IPv4 on wifi?) this might be relevant.

PS yes, this comment is an excuse to say Happy Eyeballs.

@markmandel
Copy link
Member

Just chatting about this in the community meeting - the thing that shifted my perspective on this is that when IPv6 is enabled on GKE, it reuses Address labels (so you will end up with multiple ExternalIP addresses on a node).

So rather than coming up with a DSL to determine which address label and what type of IP it is, let's return all the addresses from a node and let people do that in code where it's much easier.

@zmerlynn zmerlynn self-assigned this Jul 31, 2023
@zmerlynn zmerlynn changed the title Add configuration around advertised Gameserver Addresses. Return all advertised Gameserver Addresses Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/design Proposal discussing new features / fixes and how they should be implemented kind/feature New features for Agones
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants