Azure cni networking

[feend78]

This PR adds support for building clusters with Azure CNI networking. This approach requires users to create a virtual network and subnet for the cluster, define additional networking parameters within config.auto.tfvars, and pass the subnet ID into the "aks_zero" module(s) in clusters.tf.

To support this interaction between externally-defined resources and the modules comprising Kubestack, I added outputs to the _modules/aks and cluster module.

One slightly awkward piece to achieving a working setup is that the azurerm_kubernetes_cluster provider creates a network security group that holds firewall rules and is maintained by the cluster. This security group has to be associated with the externally-defined cluster subnet, but the name of that NSG is not exposed as an output by the azurerm_kubernetes_cluster resource provider. I was able to find a work-around that involves using the external data provider and running a shell script to retrieve the group's name.

This is less than ideal, however the likely "correct" solution would be for the maintainers of the azurerm_kubernetes_cluster provider to expose the NSG name as an output on the resource. I plan to raise this issue/feature request with that project separately.

[pst]

Thanks for the PR. I will review it as soon as I can.

[pst]

Up to now, each cluster module has only the three common module attributes.

All additional configuration is wrapped inside the configuration attribute, which allows the inheritance between the environments.

By adding it here, you can reference the ID from another TF resource, but lose the ability to leverage inheritance to have different IDs per environment.

I generally err on the side of keeping the API of the provider specific cluster modules the same.

What options do we have?

[feend78]

I was not happy with adding this attribute either, and first tried to see if there was a way to retrieve the external subnet id dynamically into the aks_zero map, but it seems Terraform doesn't allow anything other than literals in config.auto.tfvars.

With the current approach, it is possible to include strategic use of ${terraform.workspace} in the azurerm_subnet.external definition to ensure a separate vnet per environment, but that requires the user to understand what's needed and how it will interact with kubestack.

Perhaps moving the vnet definition into the _modules/aks folder would be a solution, but the trade-off would be that some of the reason I wanted to define the vnet outside of kubestack and pass it in somehow is that I may also want to add service endpoints or delegations to the vnet or subnet to allow access to other Azure-specific resources, such as Key Vaults, Container Registries, CosmosDB, etc.

[feend78]

However, it'd certainly be possible to include a service_endpoints key in config.auto.tfvars to populate the same key in the subnet. Is there any concern about this change adding so many optional-but-needed-for-CNI keys to config.auto.tfvars?

[pst]

Defining the vnet inside the module seems like it will limit users too much. So configuring it outside and passing the ID in seems the best approach based on what we know so far and is what you implemented so far. Maybe a good compromise is to simply move the attribute inside the configuration with the other ones for now and then document that it is important to overwrite this per environment.

This is a limitation of the current way how the inheritance is implemented. I think there may be better ways, this was built pre 0.12. But I haven't looked into it and it would certainly be a bigger change so it should be tackled in its own PR.

What do you think about moving it into the configuration for now. Then I can start testing this and if everything is fine I can merge it.

[feend78]

When you suggest moving the attribute inside the configuration, what attribute are you referring to specifically? If you mean the vnet_subnet_id, it's referring to a value that is not known until the resources are created. I get the following when I try to point to that ID within config.auto.tfvars:

[pst]

Yes, the value would need to be the hardcoded string of the id inside the tfvars file. It can't be a reference, that's what the error says.

[feend78]

So the vision I've been pursuing is one where I can maintain my infrastructure entirely with terraform, which includes PaaS data services like CosmosDB or MSSQL, as well as Virtual Network Gateways to provide access to on-premises resources. For these resources external to Kubestack, my intention was to maintain the apps/ops/loc workspaces, and ensure that the external resources are created so that they are isolated appropriately in the various environments. Putting the subnet ID in config means that at best, the external resources have to live in a separate terraform module/project, and cannot be deployed in a single terraform apply with the rest of the kubestack resources.

Given this, I would be more interested in moving the custom vnet into the module and exposing it via outputs to allow users to associate it to other PaaS resources, if needed/desired.

[pst]

I am fine with that too.

[feend78]

I have made this update to the PR. Configuration variables now contain a set of variables that are used to create a vnet and subnet, and to pass them into the azurerm_kubernetes_cluster resource appropriately.

I also engaged with Microsoft support with a question on the behavior of the NSG, and learned that when using Advanced Networking in a cluster, the NSG gets associated with the VM Scale Set's NICs, rather than the subnet generally. This meant that we can safely remove the work-around script that was present in previous iterations of this PR.

[pst]

Sounds great, I'll take a look tomorrow.

[pst]

Overall this looks great. I left a few comments on parts that I think need some discussion.

[pst]

@feend78 quick thing for us to discuss, I've been working on the TF 0.13 upgrade. The release introducing TF 0.13, just like I did for 0.12, and how Hashicorp recommends should not include any changes to infrastructure, just update to TF 0.13. (It also will require a manual terraform state replace-provider due to the new namespaced provider feature).

Long story short, the question now is, for your use case, would you prefer to have a 0.12 based Kubestack release that includes your CNI feature work, because you're not planning to upgrade to TF 0.13 yourself just yet, or do you prefer the TF 0.13 update to happen first?

Depending on your preference, I could focus next week on getting your feature in and released or on the getting TF 0.13 out.

[feend78]

My use case is still pre-production, so we have the flexibility to upgrade to TF 0.13 as soon as you get it incorporated. I'm fine with holding off on merging this until after that update. In the interim, I have simply pointed my cluster module to this PR's branch in my fork of the project, and that should work until I'm able to update this PR for TF 0.13 compatibility

[pst]

I've tested this PR today and did not find any issues.

Existing clusters require setting network_policy = null, because your PR makes Calico the default, and previously there was no network policy support. Either it did not exist, or I missed it when writing the module.

Either way, I prefer Calico as the default and will leave an upgrade note so existing clusters have to actively disable network policies to retain previous behavior.

Thanks for contributing this feature @feend78. I'm sure many future AKS users will benefit from your work.