Fix Network Security Group Gameserver rule not applied on AKS cluster #2124

WeetA34 · 2021-06-07T17:15:48Z

/kind cleanup

What this PR does / Why we need it:
Hello

This PR fixes AKS gameserver network security rule not added to the effective AKS node pool network security group.

AKS install terraform included a network security group and two network security rules which were not used.
This PR follows AKS create cluster documentation by adding the incoming gameserver security rule to the aks-agentpool-******-nsg network security group

Regards

google-cla · 2021-06-07T17:15:51Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

WeetA34 · 2021-06-07T17:18:24Z

@googlebot I signed it!

google-cla · 2021-06-07T17:18:32Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

agones-bot · 2021-06-07T17:40:34Z

Build Succeeded 👏

Build Id: d0cdfe57-5bfe-427e-94fb-8601c641113e

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

image: gcr.io/agones-images/agones-controller:1.15.0-132ff85
image: gcr.io/agones-images/agones-ping:1.15.0-132ff85
Linux C++ SDK (build): agonessdk-1.15.0-132ff85-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.15.0-132ff85.zip

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

https://132ff85-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2124/head:pr_2124 && git checkout pr_2124
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.15.0-132ff85

WeetA34 · 2021-06-07T20:11:23Z

@googlebot I signed it!

google-cla · 2021-06-07T20:11:46Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

WeetA34 · 2021-06-07T20:19:55Z

@googlebot I signed it!

agones-bot · 2021-06-08T09:09:45Z

Build Succeeded 👏

Build Id: c4c7dbcc-d720-4b3d-903e-d7e8e726c1ca

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

image: gcr.io/agones-images/agones-controller:1.15.0-e62bfcf
image: gcr.io/agones-images/agones-ping:1.15.0-e62bfcf
Linux C++ SDK (build): agonessdk-1.15.0-e62bfcf-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.15.0-e62bfcf.zip

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

https://e62bfcf-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2124/head:pr_2124 && git checkout pr_2124
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.15.0-e62bfcf

agones-bot · 2021-06-08T09:23:49Z

Build Succeeded 👏

Build Id: cc72d086-9547-49ef-96b2-52ee35ec3eb3

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

image: gcr.io/agones-images/agones-controller:1.15.0-3f5a491
image: gcr.io/agones-images/agones-ping:1.15.0-3f5a491
Linux C++ SDK (build): agonessdk-1.15.0-3f5a491-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.15.0-3f5a491.zip

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

https://3f5a491-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2124/head:pr_2124 && git checkout pr_2124
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.15.0-3f5a491

…fix-aks-install-terraform

agones-bot · 2021-06-09T09:21:15Z

Build Succeeded 👏

Build Id: a27a3d9c-bd99-4d62-8c98-8e85faf0f222

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

image: gcr.io/agones-images/agones-controller:1.15.0-7da58f5
image: gcr.io/agones-images/agones-ping:1.15.0-7da58f5
Linux C++ SDK (build): agonessdk-1.15.0-7da58f5-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.15.0-7da58f5.zip

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

https://7da58f5-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2124/head:pr_2124 && git checkout pr_2124
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.15.0-7da58f5

google-oss-robot · 2021-06-10T10:01:34Z

@dzmitry-lahoda: changing LGTM is restricted to collaborators

In response to this:

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.

WeetA34 · 2021-06-10T10:04:18Z

/assign @aLekSer

…fix-aks-install-terraform

agones-bot · 2021-06-10T11:15:46Z

Build Succeeded 👏

Build Id: 9d53821a-02d9-4e73-8e84-fae7998e59d0

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

image: gcr.io/agones-images/agones-controller:1.16.0-79a3e7a
image: gcr.io/agones-images/agones-ping:1.16.0-79a3e7a
Linux C++ SDK (build): agonessdk-1.16.0-79a3e7a-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.16.0-79a3e7a.zip

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

https://79a3e7a-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2124/head:pr_2124 && git checkout pr_2124
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.16.0-79a3e7a

dzmitry-lahoda · 2021-06-10T11:34:40Z

when I do this

data "azurerm_resources" "network_security_groups" {
  #MC_rgagones_agones-dev_westeurope
  resource_group_name = azurerm_kubernetes_cluster.agones.resource_group_name //lower(azurerm_kubernetes_cluster.agones.node_resource_group)

I can see resources in created cluster. But the code in PR, and directly having name of node pool RG does not shows anything inside, while there are resource.

So I have run copy pasted parts of PR, not full PR.

dzmitry-lahoda · 2021-06-10T11:40:15Z

I made output, without any filters,that group is shown empty.... :*

nsg = {
  "id" = "resource-eaeffc3e-2844-440f-967f-55abd5ea15c2"
  "resource_group_name" = "MC_rgagones_agones-dev_westeurope"
  "resources" = []
}

data "azurerm_resources" "network_security_groups" {
  resource_group_name = azurerm_kubernetes_cluster.agones.node_resource_group
}

WeetA34 · 2021-06-10T11:41:31Z

you forgot type = "Microsoft.Network/networkSecurityGroups" in data
in my PR, it's:

data "azurerm_resources" "network_security_groups" {
  resource_group_name = azurerm_kubernetes_cluster.agones.node_resource_group
  type                = "Microsoft.Network/networkSecurityGroups"
}

dzmitry-lahoda · 2021-06-10T11:46:51Z

nsg = {
  "id" = "resource-cc61f29c-1d33-40fd-a0ff-ef8dc559c637"
  "resource_group_name" = "MC_rgagones_agones-dev_westeurope"
  "resources" = []
  "type" = "Microsoft.Network/networkSecurityGroups"
}

data "azurerm_resources" "network_security_groups" {
  resource_group_name = azurerm_kubernetes_cluster.agones.node_resource_group
  type = "Microsoft.Network/networkSecurityGroups"
}

Type is filter, I checked whole grope - nothing. It is aks 18.19

WeetA34 · 2021-06-10T11:52:45Z

weird. no issue on my side. i have the security group as a member of data resources attribute array.
To test my filter on the security rule resource, i also tested with extra security groups.
I always have secgroups in data resources attribute

google-oss-robot · 2021-06-23T04:12:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dzmitry-lahoda, markmandel, WeetA34

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:

~~OWNERS~~ [markmandel]

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

dzmitry-lahoda · 2021-07-01T09:53:35Z

Weird thing.

Seems like group name is arbitrary. Like if it is either MC_ or mc_, so get weird artifacts.

resource_group_name                        = "MC_rgagones_agonesdev_westeurope" -> "mc_rgagones_agonesdev_westeurope" # forces replacement

Will try to run and see if it works. It is kind of sometimes lower, sometimes not (not the PR issue, but Azure one).

WeetA34 · 2021-07-01T09:56:55Z

perhaps depending on the provider version.
When i created this MR, i had to force lowercase because it was saved in lowercase.
which Azurerm provider version do you use right now?

WeetA34 · 2021-07-01T09:59:10Z

ha, i reread your message.
With the same azurerm provider version, sometimes you have MC and sometimes mc?

WeetA34 · 2021-07-01T10:07:07Z

hashicorp/terraform-provider-azurerm#10077

WeetA34 · 2021-07-01T10:11:05Z

what you can do right now to ignore this random case issue is adding a lifecycle ignore_changes block inside the azurerm_network_security_rule resource:

resource "azurerm_network_security_rule" "gameserver" {
  name                       = "gameserver"
  ...
  lifecycle {
    ignore_changes = [
      resource_group_name
    ]
  }
}

dzmitry-lahoda · 2021-07-01T10:12:35Z

With the same azurerm provider version, sometimes you have MC and sometimes mc?

let me test new version.

WeetA34 · 2021-07-01T10:15:19Z

with a new version it will be the same i guess.
What i wanted to be sure is do you have random results with the same provider?

dzmitry-lahoda · 2021-07-01T10:16:21Z

Used 2.62. Replaced with 2.65. I will rerun script couple of times within month and check (script is not above, but my local modification for windows cluster).

WeetA34 · 2021-07-01T10:30:55Z

Can you perform terraform plan with different azurerm version to see if it's random or related to a provider version?
Thank you

dzmitry-lahoda · 2021-07-01T10:33:57Z

created in 2.62, applied in 2.65:

 resource_group_name                        = "MC_rgagones_agonesdev_westeurope" -> "mc_rgagones_agonesdev_westeurope" # forces replacement

Seems ignore will help

WeetA34 · 2021-07-01T11:33:54Z

Ok. It seems forcing resource_group_name to lowercase is no more needed.
I’ll deploy a new cluster with latest azurerm provider.
If needed i’ll create a PR to remove lower function.

WeetA34 · 2021-07-01T12:17:45Z

i created a new cluster and encountered your previous issue with data security groups which returned no resources.
i had to lowercase the resource_group_name to get results

data "azurerm_resources" "network_security_groups" {
  resource_group_name = lower(azurerm_kubernetes_cluster.agones.node_resource_group)

  type = "Microsoft.Network/networkSecurityGroups"
}

-->

$ terraform state show 'module.aks_cluster.data.azurerm_resources.network_security_groups'
# module.aks_cluster.data.azurerm_resources.network_security_groups:
data "azurerm_resources" "network_security_groups" {
    id                  = "resource-xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
    resource_group_name = "mc_agonesrg_test-cluster_eastus"
    resources           = [
        {
            id       = "/subscriptions/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/resourceGroups/mc_agonesrg_test-cluster_eastus/providers/Microsoft.Network/networkSecurityGroups/aks-agentpool-xxxxxxxx-nsg"
            location = "eastus"
            name     = "aks-agentpool-xxxxxxxx-nsg"
            tags     = {}
            type     = "Microsoft.Network/networkSecurityGroups"
        },
    ]
    type                = "Microsoft.Network/networkSecurityGroups"
}

Without the lowercase

data "azurerm_resources" "network_security_groups" {
  resource_group_name = azurerm_kubernetes_cluster.agones.node_resource_group

  type = "Microsoft.Network/networkSecurityGroups"
}

-->

$ terraform state show 'module.aks_cluster.data.azurerm_resources.network_security_groups'
# module.aks_cluster.data.azurerm_resources.network_security_groups:
data "azurerm_resources" "network_security_groups" {
    id                  = "resource-adc23b9e-a9c7-4bb2-8ded-fce0863d7a3d"
    resource_group_name = "MC_agonesRG_test-cluster_eastus"
    resources           = []
    type                = "Microsoft.Network/networkSecurityGroups"
}

WeetA34 · 2021-07-01T12:28:33Z

hummm weird
with or without lower, it finds the security group now

WeetA34 · 2021-07-01T12:35:14Z

regarding the security group rule with provider 2.65.0, resource_group_name has been saved in lowercase so when i apply again, it wants to recreate the security group rule

  # module.aks_cluster.azurerm_network_security_rule.gameserver must be replaced
-/+ resource "azurerm_network_security_rule" "gameserver" {
        access                                     = "Allow"
        destination_address_prefix                 = "*"
      - destination_address_prefixes               = [] -> null
      - destination_application_security_group_ids = [] -> null
        destination_port_range                     = "7000-8000"
      - destination_port_ranges                    = [] -> null
        direction                                  = "Inbound"
      ~ id                                         = "/subscriptions/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/resourceGroups/mc_agonesrg_test-cluster_eastus/providers/Microsoft.Network/networkSecurityGroups/aks-agentpool-xxxxxxxx-nsg/securityRules/gameserver" -> (known after apply)
        name                                       = "gameserver"
        network_security_group_name                = "aks-agentpool-xxxxxxxx-nsg"
        priority                                   = 100
        protocol                                   = "Udp"
      ~ resource_group_name                        = "mc_agonesrg_test-cluster_eastus" -> "MC_agonesRG_test-cluster_eastus" # forces replacement
        source_address_prefix                      = "*"
      - source_address_prefixes                    = [] -> null
      - source_application_security_group_ids      = [] -> null
        source_port_range                          = "*"
      - source_port_ranges                         = [] -> null
    }

WeetA34 · 2021-07-02T12:30:40Z

hello @dzmitry-lahoda,
can you check PR2165 please?
Thank you

dzmitry-lahoda · 2021-07-04T10:21:28Z

Sure, will check PR branch, so started from master

azurerm_kubernetes_cluster_node_pool.metrics: Creation complete after 4m41s [id=/subscriptions/xxxxxx/resourcegroups/rglahoda/providers/Microsoft.ContainerService/managedClusters/dztestcluster/agentPools/metrics]
╷
│ Error: Invalid index
│
│   on aks.tf line 109, in resource "azurerm_network_security_rule" "gameserver":
│  109:   network_security_group_name = [for network_security_group in data.azurerm_resources.network_security_groups.resources : network_security_group.name if length(regexall("^aks-agentpool-\\d+-nsg$", network_security_group.name)) > 0][0]
│     ├────────────────
│     │ data.azurerm_resources.network_security_groups.resources is empty list of object
│
│ The given key does not identify an element in this collection value.
╵
xxxxxx\agones\install\terraform\modules\aks [main ≡ +0 ~3 -0 !]>

WeetA34 · 2021-07-04T10:25:18Z

Hello,
don't use master as it doesn't include my PR
see #2165 (comment)

dzmitry-lahoda · 2021-07-04T10:31:24Z

Sure, just to check that.

On second attempt to run again, master passed well. No changes, just retried after some time.

So kube_config is now

sensitive = true

Switching to branch.

WeetA34 · 2021-07-04T10:40:49Z

let's continue discussion on PR2165

dzmitry-lahoda · 2021-11-12T08:55:56Z

@WeetA34 used this for reference of hash https://github.com/Azure/aks-engine/blob/master/pkg/api/types.go#L1014-L1030

google-cla bot added the cla: no label Jun 7, 2021

google-oss-robot added the size/M label Jun 7, 2021

google-oss-robot requested review from EricFortin and roberthbailey June 7, 2021 17:15

google-cla bot added cla: yes and removed cla: no labels Jun 7, 2021

Fix Network Security Group Gameserver rule not applied

3f5a491

WeetA34 force-pushed the fix-aks-install-terraform branch from e62bfcf to 3f5a491 Compare June 8, 2021 08:57

Merge branch 'main' of https://github.com/googleforgames/agones into …

7da58f5

…fix-aks-install-terraform

dzmitry-lahoda approved these changes Jun 10, 2021

View reviewed changes

google-oss-robot assigned aLekSer Jun 10, 2021

Merge branch 'main' of https://github.com/googleforgames/agones into …

79a3e7a

…fix-aks-install-terraform

google-oss-robot assigned markmandel Jun 23, 2021

google-oss-robot added the lgtm label Jun 23, 2021

google-oss-robot added the approved label Jun 23, 2021

markmandel added this to the 1.16.0 milestone Jun 23, 2021

markmandel added area/operations Installation, updating, metrics etc kind/cleanup Refactoring code, fixing up documentation, etc labels Jun 23, 2021

markmandel merged commit 4620efe into googleforgames:main Jun 23, 2021

WeetA34 deleted the fix-aks-install-terraform branch July 1, 2021 10:26

dzmitry-lahoda mentioned this pull request Nov 4, 2021

Node Public IP Host Network NSG Control Azure/AKS#2553

Closed

Fix Network Security Group Gameserver rule not applied on AKS cluster #2124

Fix Network Security Group Gameserver rule not applied on AKS cluster #2124

Conversation

WeetA34 commented Jun 7, 2021

google-cla bot commented Jun 7, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

WeetA34 commented Jun 7, 2021

google-cla bot commented Jun 7, 2021

agones-bot commented Jun 7, 2021

WeetA34 commented Jun 7, 2021 • edited Loading

google-cla bot commented Jun 7, 2021

WeetA34 commented Jun 7, 2021

agones-bot commented Jun 8, 2021

agones-bot commented Jun 8, 2021

agones-bot commented Jun 9, 2021

google-oss-robot commented Jun 10, 2021

WeetA34 commented Jun 10, 2021

agones-bot commented Jun 10, 2021

dzmitry-lahoda commented Jun 10, 2021

dzmitry-lahoda commented Jun 10, 2021 • edited Loading

WeetA34 commented Jun 10, 2021 • edited Loading

dzmitry-lahoda commented Jun 10, 2021

WeetA34 commented Jun 10, 2021 • edited Loading

google-oss-robot commented Jun 23, 2021

dzmitry-lahoda commented Jul 1, 2021 • edited Loading

WeetA34 commented Jul 1, 2021

WeetA34 commented Jul 1, 2021

WeetA34 commented Jul 1, 2021

WeetA34 commented Jul 1, 2021 • edited Loading

dzmitry-lahoda commented Jul 1, 2021

WeetA34 commented Jul 1, 2021

dzmitry-lahoda commented Jul 1, 2021 • edited Loading

WeetA34 commented Jul 1, 2021

dzmitry-lahoda commented Jul 1, 2021

WeetA34 commented Jul 1, 2021

WeetA34 commented Jul 1, 2021 • edited Loading

WeetA34 commented Jul 1, 2021

WeetA34 commented Jul 1, 2021

WeetA34 commented Jul 2, 2021

dzmitry-lahoda commented Jul 4, 2021

WeetA34 commented Jul 4, 2021

dzmitry-lahoda commented Jul 4, 2021

WeetA34 commented Jul 4, 2021

dzmitry-lahoda commented Nov 12, 2021

WeetA34 commented Jun 7, 2021 •

edited

Loading

dzmitry-lahoda commented Jun 10, 2021 •

edited

Loading

WeetA34 commented Jun 10, 2021 •

edited

Loading

WeetA34 commented Jun 10, 2021 •

edited

Loading

dzmitry-lahoda commented Jul 1, 2021 •

edited

Loading

WeetA34 commented Jul 1, 2021 •

edited

Loading

dzmitry-lahoda commented Jul 1, 2021 •

edited

Loading

WeetA34 commented Jul 1, 2021 •

edited

Loading