Move to multiple dynamic range ports

[WeetA34]

Hi Mark,

Our game server uses 4 passthrough ports:

• 1 game port
• 3 tools ports I would like to have the game port opened to the whole universe but not the 3 tools ports for which i want to allow only some sources

Proposal: I imagine we can move port range (gameservers.minPort and gameservers.maxPort) to a list of port ranges and choose the port range to use in the game server ports definition. So, we can define a different firewall rule for each range.

ie. install values:

...
gameservers:
  namespaces: ["default"]
  ranges:
  - name: game
    minPort: 7000
    maxPort: 7099
  - name: tools
    minPort: 7100
    maxPort: 7199

and gamerserver definition:

apiVersion: "agones.dev/v1"
kind: GameServer
...
spec:
  # if there is more than one container, specify which one is the game server
  container: example-server
  # Array of ports that can be exposed as direct connections to the game server container
  ports:
  - name: default
    range: game
    portPolicy: Passthrough
    protocol: UDP
  - name: tool1
    range: tools
    portPolicy: Passthrough
    protocol: TCP
  - name: tool2
    range: tools
    portPolicy: Passthrough
    protocol: TCP
  - name: tool3
    range: tools
    portPolicy: Passthrough
    protocol: TCP
...

Workaround: If the game port is UDP and Tools ports are TCP, you can only filter sources for TCP destinations. But it's not possible if you want to apply different firewall rules for the same protocol

Thank you Regards Stéphane

[markmandel]

Interesting!

Curious - I assume the systems that are connecting on the "tooling" ports are running outside the cluster that your game servers are running on?

So some kind of scraping/pulling system for each game server?

Is that correct?

[WeetA34]

Hi Mark,

For now, these ports are mainly used for debugging purpose and must be filtered to only get access from our network.

Regards

[markmandel]

For now, these ports are mainly used for debugging purpose and must be filtered to only get access from our network.

Understood - I'm just wondering what the connection pattern is. If we can pull the tooling into the same cluster as the game servers (or maybe run some kind of kubectl based tunnelling, if it's at development time only), then the problem might be solveable another way.

[WeetA34]

In fact, developers don't have access to the cluster. Right now, game servers are in Gamelift. I'm not sure they can easily work with kubectl port forward for remote imgui and other tool like their inter process connect tool and debug draw infos which are pulled directly from the client. If we don't want to change their work flow if we move to Agones, we should be able to access these ports directly without any tunneling tricks.

[markmandel]

Right - you would want to setup some kind of VPC to an internal corporate network type situation so you could lock down access to your specific network.

I'm guessing this would also be for both for development, but then also for production debugging?

[WeetA34]

We perform filtering with security group. Connection is from our internal network to these gameserver tools ports. Yep, it's for development for sure and for production if the issue can't be reproduced in a development deployment.

[roberthbailey]

@markmandel - assuming that we did want to do this, is there a way to do it that is backwards compatible?

[markmandel]

@markmandel - assuming that we did want to do this, is there a way to do it that is backwards compatible?

Probably have the helm configured range be the default range, and then have an ability like above to be able to configure extra ranges for each port. The things would be backward compatible.

It might be good to have a set of port ranges configured thought from the beginning (new CRD? Or new helm config?), rather than allowing any game server to specify any range they want -- it would be tricky to track an arbitrary range per GameServer.

[WeetA34]

It's probably more complicated (or at least longer) than the one hour video you did for moving gameservers from one port to multi-ports @markmandel 😄

I guess you can keep helm gameservers.minPort and gameservers.maxPort values for backward compatibility and assign them to a "default" range in the range list. This "default" range would be used in gameserver definition if range is not set.

[WeetA34]

It might be good to have a set of port ranges configured thought from the beginning (new CRD? Or new helm config?), rather than allowing any game server to specify any range they want -- it would be tricky to track an arbitrary range per GameServer.

Yep, that's my proposal: having a set of port ranges

[markmandel]

Not sure I like helm for this, because the config layer is hard to define/validate - but I don't think there's a better option?

I would probably have something like:

gameservers:
  minPort: 7000
  maxPort: 7000
  alternatePortRanges:
    - name: debug
      min: 1000
      max: 2000
    - name: health
      min: 4000
      max: 5000

Not quite sold on "alternatePortRanges" as a name, but you get the drift. "namedPortRanges" ?

Which is similar to what you have above - but gameservers.minPort and gameServers.maxPort remain the default (and default would be a keyword name for non-assigned ports in the GameServer.

So we could validate them, and ensure there are no overlapping port ranges, as well as maintaining backward compatibility.

If there are no overlapping ranges, this also means you could have one PortAllocator per range name, and it becomes quite easy to manage.

Then we could do the same as you have above, where you assign the port range by name.

Howzat?

Update:

WHAAAT - Helm 3 has can have a VALIDATION SCHEMA??? https://austindewey.com/2020/06/13/helm-tricks-input-validation-with-values-schema-json/ :exploding_head:

Ignore my desire to not use helm.

[WeetA34]

Hi Mark, i don't know if you were waiting something else from me. You're proposal sounds good. Thank you

[markmandel]

@WeetA34 not waiting on anything, just prioritising other work.

Since this is of import to you, would you be interested in working on it?

[WeetA34]

No problem Mark. It doesn't seem to be a feature tons of person are interested on :) We're not in a hurry at the moment. To be honest, i don't think i have the proper dev skill level to work on it :)

[markmandel]

Come join us in #development in Slack, we'll get you leveled up! 😎

[WilSimpson]

I'm going to start working on this. This is my first time using Helm, but this is what I researched and came up with.

It looks like the current min/max port are passed as environment variables to the controller. Following Mark's schema, I was thinking of using a helm loop to set a string encoded environment variable with commas separating the values and newlines separating the tuples. So the above example would become debug,1000,2000\nhealth,4000,5000.

Please let me know if you think there are any better approaches!

[WeetA34]

Hello @WilSimpson When i checked how to handle this, i arrived to the same conclusion. I was thinking of using an extra environment variable EXTRA_PORT_RANGES (ie) with the following format: <PORT_RANGE_NAME>:<PORT_RANGE_MIN>:<PORT_RANGE_MAX>. And multiple port ranges separated by commas: (ie: tools:5000:5999,whatever:6000:6999)

[WeetA34]

Then first thing is port ranges checks, ensure:

• that for each extra port ranges, min is less than max
• there is no overlapping port ranges (between each extra port range and between any of extra port range and min/max)

[markmandel]

Hello @WilSimpson When i checked how to handle this, i arrived to the same conclusion. I was thinking of using an extra environment variable EXTRA_PORT_RANGES (ie) with the following format: <PORT_RANGE_NAME>:<PORT_RANGE_MIN>:<PORT_RANGE_MAX>. And multiple port ranges separated by commas: (ie: tools:5000:5999,whatever:6000:6999)

I like this one 👍🏻

But yeah - that's an excellent point, I hadn't considered how to pass it to the controller. Comma delimited list seems like a legit way to go.

As a possible other suggestion: Use a URL query param encoded list (there are built in parsers already, that's why we use it on Feature Flags), but then you still have to work out how to specify min/max on it 🤔

EXTRA_PORT_RANGES="tools=5000%3A6000&whatever=7000%3A9000"

Would still need to split on ":", but would at least take out some of the parsing pain? (although that %3A is... not... pretty 😬 maybe this is a bad idea).

[WilSimpson]

I was thinking how they are encoded is less important because they will be set with a helm set or helm -f. If used with helm set then it would become helm ... --set gameserver.alternatePortRanges [{name: "debug", min: "1000", max: "2000"}, {name: "health", min: "4000", max: "5000"}]. Then in the case of helm -f you would just specify a yaml file with

gameservers:
  alternatePortRanges:
    - name: debug
      min: 1000
      max: 2000
    - name: health
      min: 4000
      max: 5000

The reason I chose comma was arbitrary, but I do like semicolon better and that's a simple change. The reason I chose \n was because it's a natural conversion with helm range loop:

{{- if  .Values.gameserver.altPortRanges }}
- name: ALT_PORTS
  value: |-
  {{- range .Values.gameserver.altPortRanges }}
  - {{ printf "%s:%d:%d" .name .min .max }}
  {{- end }}
{{- end }}

[WilSimpson]

@markmandel How would you like to handle specifying ports for allocation for backwards compatibility? I know you talked about having one PortAllocator per range, but I thought that would be quite a bit of duplicate memory.

Instead I took this approach, but I'm happy to change:

// Map a allocation name to a set of port allocations for a node
type portAllocation map[string]map[int32]bool

This way you can access portAllocation by name, then get the map of ports that are available.

[WeetA34]

The reason I chose \n was because it's a natural conversion with helm range loop

ok, i see. If we don't want \n in the environment variable, we can use the following in helm.

{{- with .Values.gameservers.alternatePortRanges }}
{{-   $alternatePortRanges := list }}
{{-   range . }}
{{-     $alternatePortRanges = append $alternatePortRanges (printf "%s:%.0f:%.0f" .name .min .max) }}
{{-   end }}
- name: ALTERNATE_PORT_RANGES
  value: {{ $alternatePortRanges | join "," }}
{{- end }}

[markmandel]

Seems like y'all would have a handle on the delimiters for environment variables 👍🏻

@markmandel How would you like to handle specifying ports for allocation for backwards compatibility? I know you talked about having one PortAllocator per range, but I thought that would be quite a bit of duplicate memory.

My original thought was to have a PortAllocator per name stored in a map / struct somewhere that would distribute the work out, with something like default being the default (or something similar) one being the main one. I don't think there's much/any memory duplication.

I feel like it might be easier to update a PortAllocator to take a name on creation, and then make some small changes to only look at ports with a given name, than change the data structure to be name + node allocations - it could start to get pretty deep.

I don't have strong opinions though, if you think it'll be easier doing it all in one PortAllocator, have at it - happy to review a draft PR if that helps as well.

[EricFortin]

@WilSimpson Are you still working on this?

[WilSimpson]

@WilSimpson Are you still working on this?

Unfortunately I've been pulled other directions and haven't had time to finish the implementation.

[github-actions[bot]]

'This issue is marked as Stale due to inactivity for more than 30 days. To avoid being marked as 'stale' please add 'awaiting-maintainer' label or add a comment. Thank you for your contributions '

[markmandel]

Seems like this would be useful, so moving to awaiting-maintainer to get done at some point in the future.

[Ullaakut]

👋 Like we discussed on Slack before, we are interested in helping this issue move forward at Nitrado.

Using the aforementioned snippets of code, I wrote a few minimal changes to the business logic that could be part of implementing the requested feature. They do not include new tests yet, nor do they update the configuration, this would have to be done if the solution is accepted, but the changes to the allocation business logic should be enough to have a pretty clear picture of what it might look like.

Here is a quick overview of the proposed changes:

1. Adding a Pool attribute to the agones/v1/GameServerPort resource:

// GameServerPort defines a set of Ports that
// are to be exposed via the GameServer
type GameServerPort struct {
	// Name is the descriptive name of the port
	Name string `json:"name,omitempty"`
	// Pool is the name of the port pool to which this port belongs.
	Pool string `json:"pool,omitempty"`

2. Making the portAllocator take a slice of port pools instead of only a min and max port

// PortPool is a named count of ports for allocate from.
type PortPool struct {
	// Name of the pool.
	Name string
	// Min is the lowest port number for that pool.
	Min int32
	// Max is the lowest port number for that pool.
	Max int32
}

3. Changing the findOpenPorts algorithm to look for ports for each pool

	findOpenPorts := func(pool string, count int) []pn {
		if count <= 0 {
			return []pn{}
		}

		var ports []pn
		for _, allocs := range pa.portAllocations {
			for p, taken := range allocs[pool] {
				if taken {
					continue
				}

				ports = append(ports, pn{pa: allocs[pool], port: p})

				// Once a pool has enough ports allocated, move on to next pool.
				if len(ports) == count {
					break
				}
			}
		}
		return ports
	}

4. Changing the way ports are allocated to use the specified Pool attribute in the GameServerPort

	var allocate func(gs *agonesv1.GameServer) *agonesv1.GameServer
	allocate = func(gs *agonesv1.GameServer) *agonesv1.GameServer {
		pools := gs.CountPorts(func(policy agonesv1.PortPolicy) bool {
			return policy == agonesv1.Dynamic || policy == agonesv1.Passthrough
		})
		allocations := make(map[string][]pn, len(pools))
		var want, got int
		for name, count := range pools {
			allocations[name] = findOpenPorts(name, count)
			want += count
			got += len(allocations[name])
		}

		if got >= want {
			pa.gameServerRegistry[gs.ObjectMeta.UID] = true

			var extraPorts []agonesv1.GameServerPort

			for i, p := range gs.Spec.Ports {
				if p.PortPolicy != agonesv1.Dynamic && p.PortPolicy != agonesv1.Passthrough {
					continue
				}

				if _, ok := allocations[p.Pool]; !ok {
					pa.logger.Error("Unknown pool name found on game server port, using generic pool instead")
				}
				// pop off allocation for matching pool.
				var a pn
				a, allocations[p.Pool] = allocations[p.Pool][0], allocations[p.Pool][1:]

Other changes are essentially just about adapting to using poolAllocation instead of portAllocation everywhere, which is a new alias that represents a map[string]portAllocation (which itself was not changed and is still a map[int32]bool).

Does that approach work for you? So far in testing, it looks like it might hold up, but we would obviously like to get your opinion before investing more time into its implementation.

[markmandel]

This look good - I only have minor thoughts:

1. I think I prefer the name range vs pool, since it's a "port range" we are working with - the name feels more intuitive to me. WDYT? Also assuming we are going with the alternatePortRanges as the helm configuration variable above (which is an assumption), then it all uses the same language.
2. I probably would have gone with a separate PortAllocator per name or each range -- that feels easier in my head. But your implementation seems reasonable enough, and you are actually writing the code, so I'm not 100% wedded to it 😄 I think it will be one of those things that will be easier to review in a PR.
3. Please also wrap this new feature in a feature flag, so we can go through our feature gating process (code)

Otherwise - no issues on my end! Thanks!

[Ullaakut]

Hi @markmandel !

1. Port range works fine for me 👍
2. It would probably be easier to review in a proper PR indeed, and I have to admit that it's still not easy for me to wrap my head around this piece of code overall 😄 My goal being minimal changes though, this seemed like the right approach to avoid breaking anything, but if you'd prefer another solution, that could work too :)
3. 👍