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

Update azurerm_kubernetes_cluster for GA #1389

Closed
holmesb opened this issue Jun 13, 2018 · 26 comments
Closed

Update azurerm_kubernetes_cluster for GA #1389

holmesb opened this issue Jun 13, 2018 · 26 comments

Comments

@holmesb
Copy link

holmesb commented Jun 13, 2018

AKS has gone GA. Main functionality now lacking is the basic\advanced network setting and the various parameters if advanced is chosen:

  • VNet
  • Subnet
  • Service address range
  • DNS IP
  • Docker bridge IP
@torresdal
Copy link
Contributor

I would gladly contribute to this, but looks like we are dependant on azure-sdk-for-go to include the new package from autorest containerservices (package-2018-03)?

Currently azure-sdk-for-go only has the 2017-09-30 package.

I haven't contributed to this project before, so I'm new to how the dependency to the azure-sdk-for-go-project and azure-rest-api-specs-project works. Anyone know how/when a new autorest package will be generated?

@katbyte katbyte added service/kubernetes-cluster enhancement upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR labels Jun 14, 2018
@robinsk
Copy link

robinsk commented Jun 14, 2018

Right, this seems relevant for us. We require advanced networking, and face problems when creating an AKS cluster from Terraform.

When creating the cluster from the Azure Portal, there are 31 "connected devices" on the vnet per NIC (i.e. per host/vm in the cluster), which makes sense with maximum 30 pods per host.

When we create the cluster from Terraform, specifing vnet_subnet_id, there's only one connected device per host, so none of our pods have any networking.

@twem
Copy link

twem commented Jun 15, 2018

Also agent_pool_profile, os_type=windows has a dependency on windowsProfile but that is not documented or in the code.

Error code InvalidParameter Message Required parameter windowsProfile is missing (null).

@torresdal
Copy link
Contributor

@twem I think Windows agent is still in private preview. At least according to this: https://docs.microsoft.com/en-us/azure/aks/faq

To run Windows Server containers, you need to run Windows Server-based nodes. Windows Server-based nodes are currently in private preview. If you need to run Windows Server containers on Kubernetes in Azure outside of the preview, please see the documentation for acs-engine.

I did try anyway of course :-), using an ARM template with additional agent pool using os_type=windows and windowsProfile defined. It bootstrapped OK, but I got some issues with failing pods in the running cluster:

NAMESPACE     NAME                                    READY     STATUS             RESTARTS   AGE
kube-system   azureproxy-899885bfb-ckhkd              0/1       ImagePullBackOff   0          3h
kube-system   heapster-56c6f9566f-dvrxx               2/2       Running            0          3h
kube-system   kube-dns-v20-7c556f89c5-bbfgw           3/3       Running            0          3h
kube-system   kube-dns-v20-7c556f89c5-spbp2           3/3       Running            0          3h
kube-system   kube-proxy-ksphx                        1/1       Running            0          3h
kube-system   kube-proxy-npkvt                        1/1       Running            0          3h
kube-system   kube-svc-redirect-6cksx                 0/1       CrashLoopBackOff   28         3h
kube-system   kube-svc-redirect-nthhr                 0/1       CrashLoopBackOff   28         3h
kube-system   kubernetes-dashboard-5ffc5c5558-6ht5g   0/1       CrashLoopBackOff   40         3h
kube-system   tunnelfront-7bcb487d9f-kns4q            1/1       Running            0          3h

It could be failing for a different reason though.

@dstrebel
Copy link
Contributor

Windows is not available in AKS yet.

@tombuildsstuff
Copy link
Contributor

The new/GA API version should be available in v17.4 of the Azure SDK for Go: Azure/azure-sdk-for-go#2069 - once that's merged we'll look into updating to that version and then it should be possible to implement this.

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Jun 20, 2018

Upgrading to the GA API also means the Private Networking Route Table bug (detailed in the docs) is resolved: #1197 (comment)

@koalalorenzo
Copy link

koalalorenzo commented Jun 20, 2018

Any ETA for this? :D
What about Azure Active Directory support?

@lfshr
Copy link
Contributor

lfshr commented Jun 21, 2018

Made a start on this. So far have the containerservices upgraded to 2018-03-31 - This is my first attempt at anything GO, so some guidance would be much appreciated! Feel free to start again and overtake me if you must.

The plan is to fully upgrade to v16.2.1 before I even attempt extra implementations. So far I've only managed containerservices. Will probably open a PR tomorrow for the upgrade standalone if nobody has issues with that.

Package Name API Version Status
containerservices 2018-03-31 Done
documentdb 2015-04-08 Was listed in updates - will check
network 2018-05-01 To Do

https://github.com/lfshr/terraform-provider-azurerm/tree/azure-sdk-for-go/v16.2.1

@hafizullah
Copy link

looks like the sdk fix is released, any plans when this will be merged?

@tombuildsstuff
Copy link
Contributor

@lfshr thanks for looking into this. We upgrade the entire SDK holistically; which we're doing in #1418 - that said the changes in your branch otherwise look good. Would you be able to remove the vendoring from your branch and then we should be able to use that once #1418 has been merged :)

Once the SDK's upgrades been merged and we've switched over to the GA version (as in your branch) - it should be possible to add the new functionality :)

Thanks!

@tombuildsstuff tombuildsstuff removed the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Jun 22, 2018
@lfshr
Copy link
Contributor

lfshr commented Jun 22, 2018

@tombuildsstuff no problem :) I'm actually really digging go so far! I'm still coming to grips with how this is all glued together. Shall I leave the inclusion of the containerservices 2018-03-31 vendor in, will that be included in #1418, or shall that go in as a separate PR altogether? I notice it's not currently included in #1418

@lfshr
Copy link
Contributor

lfshr commented Jun 22, 2018

@tombuildsstuff I've removed the autorest upgrade and the removal of containerservices 2017-09-30 for now. I can't remove the addition of 2018-03-31 as there were a few breaking changes between the two versions. I'm happy for this to go in separately with another cleanup PR to remove 2017-09-30. I'm easy either way.

@tombuildsstuff
Copy link
Contributor

@lfshr sounds good. Once the SDK upgrades merged, we can rebase that branch on top and then remove the old API version :)

@lfshr
Copy link
Contributor

lfshr commented Jun 25, 2018

Hey @tombuildsstuff I see that PR got merged. I've rebased to the latest version of master and stuck my code in a more suitable named branch. Let us know what your plan is.
https://github.com/lfshr/terraform-provider-azurerm/tree/containerservices/2018-03-31

@luisdavim
Copy link

Any updates on this?

@tombuildsstuff
Copy link
Contributor

@lfshr sorry for the delayed response here; would you mind sending a PR for that? Looking at the code, other than fixing the vendoring that branch otherwise looks good to me 👍

@lfshr
Copy link
Contributor

lfshr commented Jul 2, 2018

@tombuildsstuff should we pull this into a separate branch? This is just the update itself with no new implementations. Looking at the vendor stuff just now.

@tombuildsstuff
Copy link
Contributor

@lfshr I'd suggest we upgrade to the new API and then we can add the new fields since they're related, but not dependent tasks (and that'll allow us to fix the routing table bug mentioned above in the interim) - what do you think?

@lfshr
Copy link
Contributor

lfshr commented Jul 2, 2018

#1474 opened

@lfshr
Copy link
Contributor

lfshr commented Jul 2, 2018

Started work on advanced networking implementation. Active feedback would be appreciated 😄 PS. I have zero pride. Don't worry about hurting it! 😛
https://github.com/lfshr/terraform-provider-azurerm/tree/containerservices-advancednetworking

Currently most of the params are not flagged as ForceNew. I assume this to be needed for all of them, but I want to test scenarios before jumping to my assumption.

@lfshr
Copy link
Contributor

lfshr commented Jul 2, 2018

NetworkProfile implementation seems to be finished.. Just tests to add.
https://github.com/lfshr/terraform-provider-azurerm/tree/containerservices-advancednetworking

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Jul 3, 2018

Closing this in favour of #1434 since the GA upgrade has been merged in - thanks @lfshr :)

@ulm0

This comment has been minimized.

@acondrat

This comment has been minimized.

@tombuildsstuff
Copy link
Contributor

Since the original issue here's been resolved I'm going to lock this issue for the moment.

@hashicorp hashicorp locked as resolved and limited conversation to collaborators Dec 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests