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

Allow ports to be added to any container in a GS pod #1396

Merged
merged 18 commits into from Mar 18, 2020
Merged

Allow ports to be added to any container in a GS pod #1396

merged 18 commits into from Mar 18, 2020

Conversation

benclive
Copy link
Contributor

@benclive benclive commented Mar 6, 2020

We have a use case for mapping ports to sidecars for auth purposes. Our pod contains a sidecar container for auth as well as a main game server container.

We require clients to be able to access the auth sidecar as well as the main game container, but Agones only maps ports to the main game container right now. This PR adds support for specifying a "ContainerName" in the GameServerPort object. It will default to the main game container if omitted so should be backwards compatible with existing configs.

Example
Using a modified simple-udp fleet.yaml gives a gameserver with ports allocated to both containers. (I ran nc inside alpine to avoid docker networking issues with KinD)

$ kubectl get gs
NAME                     STATE   ADDRESS      PORT   NODE                   AGE
simple-udp-trwmb-rflmk   Ready   172.17.0.5   7460   agones-control-plane   22m
$ docker run -i alpine nc -u 172.17.0.5 7460
Hello
ACK: Hello
^Cpunt!
$ docker run -i alpine nc -u 172.17.0.5 7602
Hello, 5000!
ACK: Hello, 5000!
^Cpunt!

Game Server Description:

  Ports:
    Container Name:  simple-udp
    Container Port:  7654
    Host Port:       7460
    Name:            default
    Port Policy:     Dynamic
    Protocol:        UDP
    Container Name:  simple-udp-5000
    Container Port:  5000
    Host Port:       7602
    Name:            other
    Port Policy:     Dynamic
    Protocol:        UDP

fleet.yaml

apiVersion: "agones.dev/v1"
kind: Fleet
metadata:
  name: simple-udp
spec:
  replicas: 1
  template:
    spec:
      container: simple-udp
      ports:
      - name: default
        containerPort: 7654
      - name: other
        containerPort: 5000
        containerName: simple-udp-5000
      template:
        spec:
          containers:
          - name: simple-udp
            image: gcr.io/agones-images/udp-server:0.18
            resources:
              requests:
                memory: "64Mi"
                cpu: "20m"
              limits:
                memory: "64Mi"
                cpu: "20m"
          - name: simple-udp-5000
            image: gcr.io/agones-images/udp-server:0.18
            args:
              - -port
              - "5000"
            resources:
              requests:
                memory: "64Mi"
                cpu: "20m"
              limits:
                memory: "64Mi"
                cpu: "20m"

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@aLekSer
Copy link
Collaborator

aLekSer commented Mar 6, 2020

Thanks for this PR, it seems that this feature would be really useful.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: fdf4096e-9153-44c6-bdfa-69a14fe1dbeb

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Copy link
Contributor

@domgreen domgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall would be really helpful for us ... only questions are around if any other tests are needed.


for _, p := range gs.Spec.Ports {
cp := corev1.ContainerPort{
ContainerPort: p.ContainerPort,
HostPort: p.HostPort,
Protocol: p.Protocol,
}
gsContainer.Ports = append(gsContainer.Ports, cp)
portApplied := false
for j, container := range pod.Spec.Containers {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - j is this just index?

@@ -417,6 +423,22 @@ func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bo
Message: ErrHostPortDynamic,
})
}

if p.ContainerName != "" && gss.Container != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there tests around one or both of these being an empty string?

HostPort: 5002,
PortPolicy: Static,
ContainerPort: 5002,
ContainerName: "anothertest",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the only place it needs to be tested?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the comment below on splitting things up, I don't see any other unit tests that need updating at this point.

But like i mentioned in the main comment - would definitely want a e2e test.

@google-oss-robot
Copy link

@domgreen: changing LGTM is restricted to collaborators

In response to this:

Looks good overall would be really helpful for us ... only questions are around if any other tests are needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 3a42fde6-c3cb-4779-b717-312b709fd616

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@aLekSer
Copy link
Collaborator

aLekSer commented Mar 6, 2020

make test-gen-api-docs have failed.
Please run make gen-api-docs and add resulted html file here into the PR.
Also you should run make gen-install that way overall make test with all internal checks should be successful.
By the way you should update proper md file to reflect this change.

@markmandel markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones labels Mar 6, 2020
@markmandel
Copy link
Member

markmandel commented Mar 6, 2020

Hi! Thanks so much for doing this work! I had a TODO from a previous Slack conversation to write a ticket for this for ages, and never got around to it! This is going to be really useful.

A few high level things, and then I'll drop some comments on more specific stuff:
Since this is a CRD and functionality change, we'll need it to go through Feature Stages as an Alpha feature, and be implemented as such.

Which means it will also need a feature gate - you can add one here.

We should also add an e2e test for this - I wonder if we can run 2 simple-udp containers side by side and open them both up to UDP traffic separately? Or run a nginx image as a sidecar, and connect to the TCP port as a http request even?

Right now the e2e test installation doesn't enable any feature flags, but I'll enable that today so it's ready for you as well.

Please let us know if you have any questions on the above.

@@ -192,7 +192,9 @@ type GameServerPort struct {
// When `Static` portPolicy is specified, `HostPort` is required, to specify the port that game clients will
// connect to
PortPolicy PortPolicy `json:"portPolicy,omitempty"`
// ContainerPort is the port that is being opened on the game server process
// ContainerName is the container on which to open the port. Defaults to the game server container.
ContainerName string `json:"containerName,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Container string to match what we have at the top level for specifying the "game server container"? Seems like it should be more consistent.

Also, when you do the Alpha feature flagging - this should be made into a pointer, so it's easily omitted.


if p.ContainerName != "" && gss.Container != "" {
containerFound := false
for _, container := range gss.Template.Spec.Containers {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to update FindGameServerContainer to something that can FindContainer which takes a container name passed in?

Might make this code a little nicer, and likely easier to test.

gsContainer.Ports = append(gsContainer.Ports, cp)
portApplied := false
for j, container := range pod.Spec.Containers {
if container.Name == p.ContainerName {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably also makes sense to update and rename ApplyToPodGameServerContainer to use with this logic, since it's almost the same, except you would want to pass in a name, rather than use the gs.Spec.Container by default.

assert.Equal(t, corev1.Protocol("UDP"), pod.Spec.Containers[0].Ports[0].Protocol)
assert.True(t, metav1.IsControlledBy(pod, fixture))

sidecar := corev1.Container{Name: "sidecar", Image: "container/sidecar"}
fixture.Spec.Template.Spec.ServiceAccountName = "other-agones-sdk"
fixture.Spec.Ports = append(fixture.Spec.Ports, GameServerPort{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to split this into two tests? Once with the default port setup, and one with this new functionality - just to get coverage on both scenarios?

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a bunch of comments, but this is definitely headed in a great direction. Some other things that also in mind:

We'll need some docs here: https://agones.dev/site/docs/reference/gameserver/
(Check out the documentation guide on how to hide new features until they are complete), and updates here:
https://github.com/googleforgames/agones/blob/master/examples/gameserver.yaml

I think that's about it though 👍

@benclive
Copy link
Contributor Author

benclive commented Mar 9, 2020

I've iterated on all your suggestions: thanks for the feedback! There are a couple of instances where errors are newly returned but I opted to ignore them due to the game server name definitely being set at that point.
Do you want me to propagate them through the call stack to handle them correctly, or is there a logger to track those without re-writing a lot of method signatures?

I've also run into a problem with the docs after make site-server. When I add the hugo feature block I get some odd formatting issues. Any ideas there?

{{% feature publishVersion="1.5.0" %}}
    # the name of the container to open the port on. Defaults to the game server container if omitted or empty.
    container: simple-udp
{{% /feature %}}

results in large white space
image

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 8e827621-4aa4-4ac3-83d7-b1d4581b0689

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@aLekSer
Copy link
Collaborator

aLekSer commented Mar 9, 2020

@benclive thanks for noticing this. As a workaround you can put the whole "```yaml" block into the shortcode:

{{% feature publishVersion="1.5.0" %}}
{{% / feature %}}

And old one into expiryVersion="1.5.0"

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 0282760d-9dec-4d6c-a35c-705091e560b2

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 699ea887-91d4-468d-97b9-8b5893d28e88

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 48cbac67-c4e8-4c97-bfd9-52c2db6ae162

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@benclive
Copy link
Contributor Author

@markmandel The E2E tests are now failing on my change; I'm waiting for #1397 to be merged to make any more progress as I need to enable the feature flag in the E2E tests.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 45217172-d1a7-4684-b960-0edfa6d882cc

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: f35b3a3c-22ae-4cd1-8103-dd17c5e3ce15

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@benclive
Copy link
Contributor Author

I can't tell if the current failure is related to my changes or not: Running test/e2e/gameserver_test.go locally on kind seems to work for me but I haven't been able to run the whole suite consistently due to my machine not being powerful enough.
I will attempt to get GKE working. In the meantime, is there any other way to debug these tests?

@markmandel
Copy link
Member

Ah, TestGameServerReserve is flaky - #1276 - I'll restart it.

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome 🔥

I just realised one thing that will need to be added, and then this is good to go (assuming all tests pass).

https://agones.dev/site/docs/guides/feature-stages/#feature-gates < we should add a section here for your feature flag and link it to the gameserver reference page.

Outside of that, this is ready to merge!

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 26235e87-2ff5-4702-af99-b794ff267625

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1396/head:pr_1396 && git checkout pr_1396
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.5.0-a65b505

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 6f186ecf-078b-4ded-a2b2-34780244ee85

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1396/head:pr_1396 && git checkout pr_1396
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.5.0-2bbc1a6

@@ -28,6 +28,7 @@ The current set of `alpha` and `beta` feature gates are:
|--------------|---------|---------|-------|-------|
| Multicluster Allocation<sup>*</sup> | N/A | Enabled | `Alpha` | 0.11.0 |
| Example Gate (not in use) | `Example` | Disabled | None | 0.13.0 |
| [Port Allocations to Multiple Containers]({{< ref "/docs/Reference/gameserver.md" >}}) | `ContainerPortAllocation` | Disabled | `Alpha` | 1.5.0 |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll hate me, but this need to be wrapped in a feature shortcode like you did previously, so it doesn't show up with this release. 🤦‍♂️ I should have mentioned that in my comments previously.

(Might need to make a copy of the table, not sure, depends how formatting works out)

I swear this is the last thing 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I had to wrap the table as you mentioned but all done now :)

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 7160b526-0161-4014-83f2-0eb04c2e52b6

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👯‍♂️ 👯‍♀️

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benclive, domgreen, markmandel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: a7c4e336-2a75-4a6f-9ae9-669a180b1f0d

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1396/head:pr_1396 && git checkout pr_1396
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.5.0-392b92f

@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ca227e84-eedd-4376-be3e-cfc7f635cf89

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1396/head:pr_1396 && git checkout pr_1396
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.5.0-787a771

@markmandel markmandel merged commit 812c897 into googleforgames:master Mar 18, 2020
@markmandel markmandel added this to the 1.5.0 milestone Mar 18, 2020
@benclive benclive deleted the feature/ports-on-multiple-containers branch March 23, 2020 10:39
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
…1396)

* Allow ports to be added to any container in a gs
* Adding E2E tests + feature flag
* Update docs
* Add feature info to feature stage page

Co-authored-by: Mark Mandel <markmandel@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants