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

Add an Azure CloudProvider Implementation #28821

Merged
merged 4 commits into from Jul 27, 2016

Conversation

Projects
None yet
@colemickens
Contributor

colemickens commented Jul 12, 2016

This PR adds Azure as a cloudprovider provider for Kubernetes. It specifically adds support for native pod networking (via Azure User Defined Routes) and L4 Load Balancing (via Azure Load Balancers).

I did have to add clusterName as a parameter to the LoadBalancers methods. This is because Azure only allows one "LoadBalancer" object per set of backend machines. This means a single "LoadBalancer" object must be shared across the cluster. The "LoadBalancer" is named via the cluster-name parameter passed to kube-controller-manager so as to enable multiple clusters per resource group if the user desires such a configuration.

There are few things that I'm a bit unsure about:

  1. The implementation of the Instances interface. It's not extensively documented, it's not really clear what the different functions are used for, and my questions on the ML didn't get an answer.
  2. Counter to the comments on the LoadBalancers Interface, I modify the api.Service object in EnsureLoadBalancerDeleted, but not with the intention of affecting Kube's view of the Service. I simply do it so that I can remove the Ports on the Service object and then re-use my reconciliation logic that can handle removing stale/deleted Ports.
  3. The logging is a bit verbose. I'm looking for guidance on the appropriate log level to use for the chattier bits.

Due to the (current) lack of Instance Metadata Service and lack of Virtual Machine Identity in Azure, the user is required to do a few things to opt-in to this provider. These things are called-out as they are in contrast to AWS/GCE:

  1. The user must provision an Azure Active Directory ServicePrincipal with Contributor level access to the resource group that the cluster is deployed in. This creation process is documented by Hashicorp or on the MSDN Blog.
  2. The user must place a JSON file somewhere on each Node that conforms to the AzureConfig struct defined in azure.go. (This is automatically done in the Azure flavor of Kubernetes-Anywhere.)
  3. The user must specify --cloud-config=/path/to/azure.json as an option to kube-apiserver and kube-controller-manager similarly to how the user would need to pass --cloud-provider=azure.

I've been running approximately this code for a month and a half. I only encountered one bug which has since been fixed and covered by a unit test. I've just deployed a new cluster (and a Type=LoadBalancer nginx Service) using this code (via kubernetes-anywhere) and have posted the kube-controller-manager logs for anyone who is interested in seeing the logs of the logic.

If you're interested in this PR, you can use the instructions in my azure-kubernetes-demo repository to deploy a cluster with minimal effort via kubernetes-anywhere. (There is currently a pending PR in kubernetes-anywhere that is needed in conjuncture with this PR). I also have a pre-built hyperkube image: docker.io/colemickens/hyperkube-amd64:v1.4.0-alpha.0-azure, which will be kept in sync with the branch this PR stems from.

I'm hoping this can land in the Kubernetes 1.4 timeframe.

CC (potential code reviewers from Azure): @ahmetalpbalkan @brendandixon @paulmey

CC (other interested Azure folk): @brendandburns @johngossman @anandramakrishna @jmspring @jimzim

CC (others who've expressed interest): @codefx9 @edevil @thockin @rootfs

@@ -81,21 +81,21 @@ type LoadBalancer interface {
// GetLoadBalancer returns whether the specified load balancer exists, and
// if so, what its status is.
// Implementations must treat the *api.Service parameter as read-only and not modify it.

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

Document the new parameter please.

This comment has been minimized.

@colemickens

colemickens Jul 15, 2016

Contributor

I did document it on each method on the Interface, github just didn't notice properly here.

const AzureProviderName = "azure"
type AzureConfig struct {
Cloud string `json:"cloud"`

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

yaml tags too?

This comment has been minimized.

@colemickens

colemickens Jul 15, 2016

Contributor

And presumably change the json decode in azure.go to yaml so we can allow both; sounds good.

This comment has been minimized.

@colemickens

colemickens Jul 15, 2016

Contributor

Actually, it looks like both https://github.com/ghodss/yaml and http://godoc.org/gopkg.in/yaml.v2 are in Godeps, but ghodss flavor is used more (not a scientific study). It also seems to translate yaml->json and then decodes into the struct, obviating the need for yaml tags, but I suppose it can't hurt and I'll add tests either way.

}
func init() {
cloudprovider.RegisterCloudProvider(AzureProviderName, func(configReader io.Reader) (cloudprovider.Interface, error) {

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

move the anonymous function out to be a defined function (and add tests ;)

oauthConfig, err := az.Environment.OAuthConfigForTenant(az.TenantID)
if err != nil {
glog.Errorf("init: failed to determine oauth configuration")

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

In general, if I'm returning an error, I generally don't log, I figure that's the caller's responsibility.

// List lists instances that match 'filter' which is a regular expression which must match the entire instance name (fqdn)
func (az *AzureCloud) List(filter string) ([]string, error) {
return nil, fmt.Errorf("not supported")

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

why not implement this in this function using a list all method?

This comment has been minimized.

@colemickens

colemickens Jul 15, 2016

Contributor

Hm, do I attempt to distinguish Kubernetes Node VMs from other "regular" VMs in the resource group? Is there a convention for tagging VMs and then filtering on the tag and then filtering on the filter regex string?

This comment has been minimized.

@colemickens

colemickens Jul 15, 2016

Contributor

Maybe something like if a tag exists: "k8s.io/clusterName": "{clusterName}" then include it as part of the list to filter with filter?

lbName := getLoadBalancerName(clusterName)
pipName := getPublicIPName(clusterName, service)
serviceName := getServiceName(service)
glog.Infof("get(%s): START clusterName=%q lbName=%q", serviceName, clusterName, lbName)

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

These should be at least glog.V(2) if not glog.V(> 2)

glog.Infof("get(%s): START clusterName=%q lbName=%q", serviceName, clusterName, lbName)
glog.Infof("get(%s): lb(%s) - retrieving", serviceName, lbName)
_, err = az.LoadBalancerClient.Get(az.ResourceGroup, lbName, "")

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

looks like you aren't checking the err and you are dropping the other result, is this call needed?

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

oh, now I see you are passing it to the check function. Please use different names for clarity. (here and elsewhere)

glog.Infof("get(%s): lb(%s) - retrieving", serviceName, lbName)
_, err = az.LoadBalancerClient.Get(az.ResourceGroup, lbName, "")
if existsLb, err := checkResourceExistsFromError(err); err != nil {
glog.Errorf("get(%s): lb(%s) - retrieving failed: %q", serviceName, lbName, err)

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

as elsewhere wrt to both logging and returning an error.

glog.Errorf("get(%s): lb(%s) - retrieving failed: %q", serviceName, lbName, err)
return nil, false, err
} else if !existsLb {
glog.Infof("get(%s): lb(%s) - doesn't exist", serviceName, lbName)

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

definitely glog.v(2) at least.

return nil, false, nil
}
glog.Infof("get(%s): pip(%s) - retrieving", serviceName, pipName)

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

glog.vp(5)

return nil, false, nil
}
glog.Infof("get(%s): FINISH")

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

delete.

_, err := az.SecurityGroupsClient.CreateOrUpdate(az.ResourceGroup, *sg.Name, sg, nil)
if err != nil {
glog.Errorf("ensure(%s): sg(%s) - updating failed: %q", serviceName, *sg.Name, err)
return nil, fmt.Errorf("failed to update security group. err=%q", err)

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

don't wrap the error, just return it.

if strings.EqualFold(*config.Name, lbFrontendIPConfigName) {
glog.Infof("reconcile(%s)(%t): lb frontendconfig(%s) - dropping", serviceName, wantLb, lbFrontendIPConfigName)
newConfigs = append(newConfigs[:i],
newConfigs[i+1:]...)

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

nit: remove line break.

return fmt.Errorf("failed to retrieve vm instance. instance=%q", machineName)
}
primaryNicID, err := getPrimaryNicID(machine)

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

by general golang style this should be getPrimaryNICID I realize NICID might be confusing, so perhaps getPrimaryIDForNIC?

This comment has been minimized.

@colemickens

colemickens Jul 15, 2016

Contributor

getPrimaryNICID is accurate though, while I feel getPrimaryIDForNIC is not. (It's retrieving the NIC ID, and passing in the machine object, rather than getting the NIC object by it's ID as I feel getPrimaryIDForNIC would imply).

Maybe something like getPrimaryNetworkInterfaceID since the azure-sdk-for-go already refers to the NICs as "Interface".

switch az.Cloud {
case "fairfax":
az.Environment = azure.USGovernmentCloud
case "mooncake":

This comment has been minimized.

@ahmetb

ahmetb Jul 12, 2016

Member

these are most definitely internal-only code names for these regions.

This comment has been minimized.

@colemickens

colemickens Jul 15, 2016

Contributor

Agreed, let's sort it out here and then I'll consume it: Azure/go-autorest#80

// Note that if the instance does not exist or is no longer running, we must return ("", cloudprovider.InstanceNotFound)
func (az *AzureCloud) InstanceID(name string) (string, error) {
machine, err := az.VirtualMachinesClient.Get(az.ResourceGroup, name, "")
if existsMachine, err := checkResourceExistsFromError(err); err != nil {

This comment has been minimized.

@ahmetb

ahmetb Jul 12, 2016

Member

lots of az.VirtualMachinesClient.Get then checkResourceExistsFromError, maybe extract this into a utility function?

lbBackendPoolName := getBackendPoolName(clusterName)
lbBackendPoolID := az.getBackendPoolID(lbName, lbBackendPoolName)
wantLb := (len(service.Spec.Ports) > 0)

This comment has been minimized.

@ahmetb

ahmetb Jul 12, 2016

Member

parens not necessary

// update rules
dirtyRules := false
updatedRules := []network.LoadBalancingRule{}

This comment has been minimized.

@ahmetb

ahmetb Jul 12, 2016

Member

nit: no need to allocate this, maybe just do a var declaration?

// update security rules
dirtySg := false
updatedRules := []network.SecurityRule{}

This comment has been minimized.

@ahmetb

ahmetb Jul 12, 2016

Member

nit: no need to allocate an empty slice either, could be just nil.

}
nicName := getLastSegment(nicID)
nic, err := az.InterfacesClient.Get(

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

nit: this formatting is weird, doesn't it all fit on one line?

// returns the full identifier of a machine
func (az *AzureCloud) getMachineID(machineName string) string {
return fmt.Sprintf(
"/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/virtualMachines/%s",

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

nit: maybe extract these out into consts?

// returns the deepest child's identifier from a full identifier string.
func getLastSegment(ID string) string {
parts := strings.Split(ID, "/")

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

should you check if len(parts) is zero here?

// returns the equivalent LoadBalancerRule, SecurityRule and LoadBalancerProbe
// protocol types for the given Kubernetes protocol type.
func getProtosFromKubeProto(protocol api.Protocol) (network.TransportProtocol, network.SecurityRuleProtocol, network.ProbeProtocol, error) {

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

nit: getProtocolFromKubernetesProtocol

// returns the equivalent LoadBalancerRule, SecurityRule and LoadBalancerProbe
// protocol types for the given Kubernetes protocol type.
func getProtosFromKubeProto(protocol api.Protocol) (network.TransportProtocol, network.SecurityRuleProtocol, network.ProbeProtocol, error) {
if protocol == api.ProtocolTCP {

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

total nit, but I feel like:

switch protocol {
  case api.ProtocolTCP:
    ...
  default:
    ...
}

is more self-documenting.

func getPrimaryNicID(machine compute.VirtualMachine) (string, error) {
var nicRef *compute.NetworkInterfaceReference
if len(*machine.Properties.NetworkProfile.NetworkInterfaces) == 1 {

This comment has been minimized.

@brendandburns

brendandburns Jul 12, 2016

Contributor

personally, I would structure this:

if (...) {
   return &(*machine...)
} else {
   for {
     if *ref.Properties.Primary {
         return &ref
    }
 }
return nil, err
}

Your call, though.
@colemickens

This comment has been minimized.

Contributor

colemickens commented Jul 26, 2016

  • Factored out the find{Probe,Rule,SecurityRule} functions as suggested.
  • Removed a couple other noisy log lines.
  • Fixed the other small unnecessary variable.
  • Implemented Instances List

Looks like the build is green as well.

@rootfs

This comment has been minimized.

Member

rootfs commented Jul 26, 2016

@colemickens @brendandburns when do we expect this can be merged? thanks.

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Jul 26, 2016

LGTM. Thanks for the patience.

@brendandburns brendandburns added the lgtm label Jul 26, 2016

@brendandburns brendandburns changed the title from Azure CloudProvider Implementation to Add an Azure CloudProvider Implementation Jul 26, 2016

@mikedanese

This comment has been minimized.

Member

mikedanese commented Jul 26, 2016

Can you squash fix-up commits @colemickens?

@k8s-merge-robot k8s-merge-robot removed the lgtm label Jul 26, 2016

@k8s-bot

This comment has been minimized.

k8s-bot commented Jul 26, 2016

GCE e2e build/test passed for commit e1f30cc.

@colemickens

This comment has been minimized.

Contributor

colemickens commented Jul 26, 2016

@brendandburns @mikedanese Rebase, squashed appropriately. Build looks green, but the LGTM label got dropped.

(Thanks for the patience as well, my turnaround on feedback was slower than I might've hoped.)

@brendandburns brendandburns added the lgtm label Jul 26, 2016

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jul 27, 2016

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot

This comment has been minimized.

k8s-bot commented Jul 27, 2016

GCE e2e build/test passed for commit 2ebffb4.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jul 27, 2016

Automatic merge from submit-queue

@k8s-merge-robot k8s-merge-robot merged commit 994239d into kubernetes:master Jul 27, 2016

6 of 7 checks passed

Submit Queue Github CI tests are not green.
Details
Jenkins GCE Node e2e Build finished. 136 tests run, 12 skipped, 0 failed.
Details
Jenkins GCE e2e Build finished. 338 tests run, 149 skipped, 1 failed.
Details
Jenkins GKE smoke e2e Build finished. 337 tests run, 335 skipped, 0 failed.
Details
Jenkins unit/integration Build finished. 3494 tests run, 15 skipped, 0 failed.
Details
Jenkins verification Build finished.
Details
cla/google All necessary CLAs are signed
@edevil

This comment has been minimized.

Contributor

edevil commented Jul 27, 2016

Great work!

@mikedanese

This comment has been minimized.

Member

mikedanese commented Jul 27, 2016

Thanks @colemickens, this looks great

@colemickens colemickens deleted the colemickens:azure-cloudprovider-pr branch Jul 27, 2016

@thockin

This comment has been minimized.

Member

thockin commented Aug 19, 2016

This has plumbed clusterName throughout, but what is the definition of
cluster name? Is it guaranteed to be set? What format and length is it?
How does it get set?

On Tue, Jul 26, 2016 at 6:42 AM, Huamin Chen notifications@github.com
wrote:

@colemickens https://github.com/colemickens @brendandburns
https://github.com/brendandburns when do we expect this can be merged?
thanks.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28821 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVPx407fRzyld35ofWM3QBg8k4S7Uks5qZg6ugaJpZM4JKELW
.

@colemickens

This comment has been minimized.

Contributor

colemickens commented Aug 19, 2016

@thockin: the clusterName is the user specified --cluster-name given to kube-controller-manager. It defaults to kubernetes. It is used for naming the load balancer in Azure so that two Kubernetes deployments could live side-by-side in a single Azure Resource Group. Since we don't create Azure Load Balancers per-service, and instead share one for the cluster, I needed some sort of name to be unique per cluster and chose clusterName.

This clusterName was an existing parameter used in the Routes interface.

@thockin

This comment has been minimized.

Member

thockin commented Aug 19, 2016

huh, right you are. I didn't know we had such a flag. Thanks!

On Thu, Aug 18, 2016 at 9:06 PM, Cole Mickens notifications@github.com
wrote:

@thockin https://github.com/thockin: the clusterName is the user
specified --cluster-name given to kube-controller-manager. It defaults to
kubernetes. It is used for naming the load balancer in Azure so that two
Kubernetes deployments could live side-by-side in a single Azure Resource
Group. Since we don't create Azure Load Balancers per-service, and instead
share one for the cluster, I needed some sort of name to be unique per
cluster and chose clusterName.

This clusterName was an existing parameter used in the Routes interface.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28821 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVGXbyxQ7V9N8v7vODNGG8zGobSdyks5qhSvEgaJpZM4JKELW
.

@jadbox

This comment has been minimized.

jadbox commented Aug 30, 2016

How does this interact with the Azure VMSS cluster that may sit behind the load balancer? E.g. will I be able to tell the cluster to scale VMs up/down within kubernetes?

@jadbox

This comment has been minimized.

jadbox commented Aug 30, 2016

Never mind, I read the scope doc that says VMSS will not be considered: https://github.com/colemickens/azure-kubernetes-status

dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018

Merge pull request kubernetes#28821 from colemickens/azure-cloudprovi…
…der-pr

Automatic merge from submit-queue

Add an Azure CloudProvider Implementation

This PR adds `Azure` as a cloudprovider provider for Kubernetes. It specifically adds support for native pod networking (via Azure User Defined Routes) and L4 Load Balancing (via Azure Load Balancers).

I did have to add `clusterName` as a parameter to the `LoadBalancers` methods. This is because Azure only allows one "LoadBalancer" object per set of backend machines. This means a single "LoadBalancer" object must be shared across the cluster. The "LoadBalancer" is named via the `cluster-name` parameter passed to `kube-controller-manager` so as to enable multiple clusters per resource group if the user desires such a configuration.

There are few things that I'm a bit unsure about:

1. The implementation of the `Instances` interface. It's not extensively documented, it's not really clear what the different functions are used for, and my questions on the ML didn't get an answer.

2. Counter to the comments on the `LoadBalancers` Interface, I modify the `api.Service` object in `EnsureLoadBalancerDeleted`, but not with the intention of affecting Kube's view of the Service. I simply do it so that I can remove the `Port`s on the `Service` object and then re-use my reconciliation logic that can handle removing stale/deleted Ports. 

3. The logging is a bit verbose. I'm looking for guidance on the appropriate log level to use for the chattier bits.

Due to the (current) lack of Instance Metadata Service and lack of Virtual Machine Identity in Azure, the user is required to do a few things to opt-in to this provider. These things are called-out as they are in contrast to AWS/GCE:

1. The user must provision an Azure Active Directory ServicePrincipal with `Contributor` level access to the resource group that the cluster is deployed in. This creation process is documented [by Hashicorp](https://www.packer.io/docs/builders/azure-setup.html) or [on the MSDN Blog](https://blogs.msdn.microsoft.com/arsen/2016/05/11/how-to-create-and-test-azure-service-principal-using-azure-cli/).

2. The user must place a JSON file somewhere on each Node that conforms to the `AzureConfig` struct defined in `azure.go`. (This is automatically done in the Azure flavor of [Kubernetes-Anywhere](https://github.com/kubernetes/kubernetes-anywhere).)

3. The user must specify `--cloud-config=/path/to/azure.json` as an option to `kube-apiserver` and `kube-controller-manager` similarly to how the user would need to pass `--cloud-provider=azure`.

I've been running approximately this code for a month and a half. I only encountered one bug which has since been fixed and covered by a unit test. I've just deployed a new cluster (and a Type=LoadBalancer nginx Service) using this code (via `kubernetes-anywhere`) and have posted [the `kube-controller-manager` logs](https://gist.github.com/colemickens/1bf6a26e7ef9484a72a30b1fcf9fc3cb) for anyone who is interested in seeing the logs of the logic.

If you're interested in this PR, you can use the instructions in my [`azure-kubernetes-demo` repository](https://github.com/colemickens/azure-kubernetes-demo) to deploy a cluster with minimal effort via [`kubernetes-anywhere`](https://github.com/kubernetes/kubernetes-anywhere). (There is currently [a pending PR in `kubernetes-anywhere` that is needed](kubernetes/kubernetes-anywhere#172) in conjuncture with this PR). I also have a pre-built `hyperkube` image: `docker.io/colemickens/hyperkube-amd64:v1.4.0-alpha.0-azure`, which will be kept in sync with the branch this PR stems from.

I'm hoping this can land in the Kubernetes 1.4 timeframe.

CC (potential code reviewers from Azure): @ahmetalpbalkan @brendandixon @paulmey

CC (other interested Azure folk): @brendandburns @johngossman @anandramakrishna @jmspring @jimzim

CC (others who've expressed interest): @codefx9 @edevil @thockin @rootfs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment