Uplift azure by aiyengar2 · Pull Request #72 · rancher/machine

aiyengar2 · 2020-03-04T01:53:57Z

This PR updates github.com/Azure/azure-sdk-for-go to 39.1.0 and github.com/Azure/go-autorest to 13.3.3.

Major updates:

Redefines AuthenticateDeviceFlow and AuthenticateServicePrincipal (now renamed AuthenticateClientCredentials) as functions that return Authorizer objects after running ValidateAuthorizer instead of tokens
Simplifies / eliminates several helper functions to use newer autorest libraries for Azure authentication
Using contexts to make calls via the Azure client and evaluating the status of async requests using Azure futures (which support WaitForCompletionRef and future.Result), which removes redundant Get calls
Node driver now implements Kill() as Azure now supports a force kill of an instance
Remove isSwarm logic as docker swarm is no longer supported

Misc. Updates:

rename Azure Driver.ctx to Driver.deploymentContext to avoid ambiguity with Go contexts
Registered Microsoft.Subscription and Microsoft.Resources as resource providers
Added some more necessary client logic to clients.go for future commits
moved some Azure fmts to naming.go to centralize naming logic in one file
Added more structs for different types of Azure resources to cleanup.go to create one common way of cleaning up resources instead of having redundant code
Changes supportedEnvironments from a map to a list and adds azure.EnvironmentFromName to get the corresponding environment instead.
Renamed HasAttachedResources to CanBeDeleted and flipped boolean logic accordingly

aiyengar2 · 2020-03-04T20:01:56Z

Apologize for the large PR; I split this change into 4 separate commits with verbose commit messages to make it easier to read, but a broad majority of the file changes are due to changes in vendor/ from dependency upgrades

luthermonson · 2020-03-04T23:41:00Z

+			ltrimmed[i] = prefixRemoved[1]
+		} else if i < numExpectedRequiredValues {
+			// failed to parse required values
+			log.Info("image parsed", l)


probably dont need

removed log in latest commit

aiyengar2 · 2020-03-05T00:08:28Z

Squashed 4 commits to 2 which separate vendor changes from code changes

superseb

Does creating a node with the current driver have the same outcome as creating/replacing a node with this driver? Asking for existing clusters and adding a node after upgrading to a version with this driver for instance.

aiyengar2 · 2020-03-17T18:45:22Z

@superseb yes with the following exceptions:

Calling Kill() in the previous version of the driver would log Azure does not implement kill. Calling Stop instead. and proceed to stop a virtual machine via an Azure VM PowerOff API call with no flag for a force stop. In the new driver, Kill() directly calls the Azure VM PowerOff API and passes in a flag for a force stop to Azure.

The old driver still responds to opening up Docker Swarm ports based on d.BaseDriver.SwarmMaster here, which the new driver does not support:

machine/drivers/azure/util.go

Lines 120 to 135 in 8cfc2ad

    
           // Open swarm port if configured 
        
           if d.BaseDriver.SwarmMaster { 
        
           	swarmHost := d.BaseDriver.SwarmHost 
        
           	log.Debugf("Swarm host is configured as %q", swarmHost) 
        
           	u, err := url.Parse(swarmHost) 
        
           	if err != nil { 
        
           		return nil, fmt.Errorf("Cannot parse URL %q: %v", swarmHost, err) 
        
           	} 
        
           	_, swarmPort, err := net.SplitHostPort(u.Host) 
        
           	if err != nil { 
        
           		return nil, fmt.Errorf("Could not parse swarm port in %q: %v", u.Host, err) 
        
           	} 
        
           	rl = append(rl, mkRule(500, "DockerSwarmAllowAny", "Allow swarm manager access (TLS-protected)", "*", swarmPort, network.TCP)) 
        
           } else { 
        
           	log.Debug("Swarm host is not configured.") 
        
           }

If a user to submitted an invalid FaultDomainCount value (i.e. the number of availability zones that you want fault domains in for your VM) or an OS disk name that references an object that exists in another region, the old driver would fail on launch; the new driver fails on PreCreate.
The old driver failed if a user supplied an image name that was not in the format publisher:offer:sku:version; the new driver allows both the original format as well as a custom image format (i.e. any string that contains an ARM resource identifier for the object)

Other than the above exceptions, all changes are just updating the underlying Azure APIs, so the behavior for creating a node with the current driver should have the same outcome as creating/replacing a node with this driver.

superseb · 2020-04-09T14:18:30Z

+		} else if i < numExpectedRequiredValues {
+			// failed to parse required values
+			if i == 3 {
+				log.Warn(versionNotSuppliedWarnString)


If this is required when using gallery, can we make it part of the error? The error should contain enough to root cause the issue, and while the generic error probably covers it, we do know what the error is but now its only logged in a different level and on a separate line. If it's not required/breaking then this can be the additional warning

Good idea! Should be fixed in the latest commit

superseb · 2020-04-09T14:18:55Z

+		if resp.Err != nil {
+			return err
+		} else if d.FaultCount > resp.MaximumFaultDomainCount {
+			return fmt.Errorf("location %s has MaximumPlatformFaultDomainCount=%d, recieved FaultDomainCount=%d",


typo received

Thanks for catching this! Fixed in the latest commit.

superseb · 2020-04-09T14:41:36Z

Can we double check there are no fixes we want in later releases of vendored libraries? And can we also check the API versions used are generally available

aiyengar2 · 2020-04-12T23:47:49Z

FYI in my latest commit, I ported a couple of changes / fixes relevant to this PR from #77 since it seems like they make more sense as part of this PR (i.e. uplifting Azure) rather than the NSG PR:

Added arm.go: this file will be used to centralize all ARM resource identifier logic as the same logic is used for NSGs in the commit mentioned above as well as for custom images in this commit.
Modified customimages.go: now uses the logic within arm.go
Modified cleanup.go: Renamed HasAttachedResources to CanBeDeleted and flipped boolean logic accordingly in all cleanup functions
Modified azureutil.go: misc. bugs / rewords in RegisterResourceProviders + OSDiskExists + virtualNetworkExists + CreateVirtualMachine + CreateAvailabilitySetIfNotExists
Modified sku.go: misc. bug in GetMaximumFaultDomainCount

aiyengar2 · 2020-04-13T05:36:47Z

Can we double check there are no fixes we want in later releases of vendored libraries? And can we also check the API versions used are generally available

Good call! As I was double checking I discovered that github.com/Azure/azure-sdk-for-go/storage will be deprecated soon and realized that we could just use the other storage API now to make all the relevant calls. I've updated the PR accordingly in the latest commit.

Also I just checked / updated the following vendored libraries:

github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-12-01/compute
github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-12-01/network
github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-05-01/resources
github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-11-01/subscriptions
github.com/Azure/azure-sdk-for-go/services/storage/mgmt/2019-06-01/storage

Afaik there are no more pending fixes that we require from these libraries for machine as they should be in their latest versions. Azure puts any service libraries that are on public preview under github.com/Azure/azure-sdk-for-go/services/preview/<pkg>, so the versioned APIs should also be GA.

aiyengar2 · 2020-04-13T21:43:01Z

This PR will resolve the following issues:

Azure Cloud Credentials: Wrong AzureUSGovernmentCloud Endpoint rancher#23350
Azure node driver fixes rancher#25342 (only milestone 1 + 2, SLES and NSG fixes are part of another PR)
Support for Custom Image Name or ID on Azure Node Templates rancher#23010

dramich · 2020-04-14T22:54:09Z

@@ -0,0 +1,25 @@
+package azureutil


The timeouts should be moved to where they are needed, one is even exported as it's used in a different package. It also looks like some of these are not used. If we don't need them they can be deleted.

And can these be set somehow? If they can't be they should be changed to a const.

Yeah that makes sense. Moving them to their respective files in the latest commit.

They shouldn't be set so I'll change them to a const too.

luthermonson · 2020-04-14T23:01:48Z

+		log.Warn("Timed out trying to parse the MaximumFaultDomainCount. Continuing execution...")
+	case resp := <-r:
+		if resp.Err != nil {
+			return err


shouldnt this return resp.Err? err at this point is going to be from the new client call above.

ah good catch. fixed in the latest commit!

This commit updates github.com/Azure/azure-sdk-for-go to 39.1.0 and github.com/Azure/go-autorest to 13.3.3. Major updates: - Redefines AuthenticateDeviceFlow and AuthenticateServicePrincipal (now renamed AuthenticateClientCredentials) as functions that return Authorizer objects after running ValidateAuthorizer instead of tokens - Simplifies / eliminates several helper functions to use newer autorest libraries for Azure authentication - Introduced arm.go + customimage.go to create a unified way of referring to resources that are parsed from ARM resource identifiers - Using contexts to make calls via the Azure client and evaluating the status of async requests using Azure futures (which support WaitForCompletionRef and future.Result), which removes redundant Get calls - Node driver now implements Kill() as Azure now supports a force kill of an instance - Remove isSwarm logic as docker swarm is no longer supported Misc. Updates: - rename Azure Driver.ctx to Driver.deploymentContext to avoid ambiguity with Go contexts - Registered Microsoft.Subscription and Microsoft.Resources as resource providers - Added some more necessary client logic to clients.go for future commits - moved some Azure fmts to naming.go to centralize naming logic in one file - Added more structs for different types of Azure resources to cleanup.go to create one common way of cleaning up resources instead of having redundant code - Changes supportedEnvironments from a map to a list and adds azure.EnvironmentFromName to get the corresponding environment instead. - Renamed HasAttachedResources to CanBeDeleted and flipped boolean logic accordingly

Reverts a change added as part of rancher#72 to fail on PreCreateCheck if the OS disk already exists. Without this check, provisioning will either still fail on create if a machine tries to attach itself to a managed disk whose settings differ from those listed in `getOSDisk` here: https://github.com/rancher/machine/blob/2fbd7ec65dea41983a8222141df4ce58df7eaeac/drivers/azure/azureutil/azureutil.go#L633-L656. However, currently this also prevents a VM from using a previously created OS Disk (i.e. one that was left over from a previously failed provisioning process if it is a managed disk) that is configured correctly.

Reverts a change added as part of rancher#72 to fail on PreCreateCheck if the OS disk already exists. Without this check, provisioning will either still fail on create if a machine tries to attach itself to a managed disk.

This was referenced Mar 4, 2020

Azure node driver fixes rancher/rancher#25342

Closed

Add validation test for Azure custom images rancher/rancher#25755

Closed

aiyengar2 force-pushed the uplift_azure branch 4 times, most recently from 4687480 to eee5869 Compare March 4, 2020 19:55

aiyengar2 requested a review from a team March 4, 2020 19:59

luthermonson reviewed Mar 4, 2020

View reviewed changes

aiyengar2 force-pushed the uplift_azure branch from eee5869 to 900aefb Compare March 5, 2020 00:05

aiyengar2 requested a review from a team March 5, 2020 01:01

aiyengar2 mentioned this pull request Mar 7, 2020

[UI] Uplift Azure node template + custom images changes rancher/rancher#25859

Closed

superseb reviewed Mar 17, 2020

View reviewed changes

luthermonson mentioned this pull request Mar 23, 2020

Azure Cloud Credentials: Wrong AzureUSGovernmentCloud Endpoint rancher/rancher#23350

Closed

aiyengar2 mentioned this pull request Mar 27, 2020

Enable NSGs shared between multiple machines #77

Merged

aiyengar2 requested review from luthermonson and superseb April 6, 2020 17:34

aiyengar2 mentioned this pull request Apr 6, 2020

Support for Custom Image Name or ID on Azure Node Templates rancher/rancher#23010

Closed

superseb reviewed Apr 9, 2020

View reviewed changes

aiyengar2 force-pushed the uplift_azure branch from 900aefb to b46ba08 Compare April 12, 2020 23:37

aiyengar2 force-pushed the uplift_azure branch from b46ba08 to c75c2ef Compare April 13, 2020 05:36

aiyengar2 requested a review from superseb April 13, 2020 05:36

aiyengar2 force-pushed the uplift_azure branch from c75c2ef to ea52f1c Compare April 13, 2020 21:30

superseb self-requested a review April 14, 2020 20:21

superseb previously approved these changes Apr 14, 2020

View reviewed changes

dramich reviewed Apr 14, 2020

View reviewed changes

luthermonson reviewed Apr 14, 2020

View reviewed changes

luthermonson self-requested a review April 14, 2020 23:02

aiyengar2 dismissed stale reviews from superseb and luthermonson via b315426 April 15, 2020 17:42

aiyengar2 force-pushed the uplift_azure branch from 7373ee5 to b315426 Compare April 15, 2020 17:42

aiyengar2 requested a review from dramich April 15, 2020 17:43

aiyengar2 force-pushed the uplift_azure branch 3 times, most recently from 8020420 to 8acc0d5 Compare April 15, 2020 22:03

luthermonson previously approved these changes Apr 16, 2020

View reviewed changes

superseb self-requested a review April 17, 2020 04:34

superseb previously approved these changes Apr 17, 2020

View reviewed changes

luthermonson removed the request for review from dramich April 17, 2020 05:17

aiyengar2 dismissed stale reviews from superseb and luthermonson via b13f35b April 17, 2020 21:46

aiyengar2 force-pushed the uplift_azure branch from 8acc0d5 to b13f35b Compare April 17, 2020 21:46

dramich approved these changes Apr 17, 2020

View reviewed changes

luthermonson merged commit c963cca into rancher:master Apr 17, 2020

This was referenced May 16, 2020

Update to support the latest Azure features #17

Closed

update Azure/go-autorest to 9.7.1 #40

Closed

Vendor updates #46

Closed

Updating Azure Vendor dependencies #64

Closed

This was referenced May 28, 2020

Errors seen during azure cluster creation when using ubuntu custom images in the node template rancher/rancher#27316

Closed

Add ability to delete unused managed disks on Azure #85

Merged

Conversation

aiyengar2 commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aiyengar2 commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiyengar2 commented Mar 5, 2020

Uh oh!

superseb left a comment

Choose a reason for hiding this comment

Uh oh!

aiyengar2 commented Mar 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superseb commented Apr 9, 2020

Uh oh!

aiyengar2 commented Apr 12, 2020

Uh oh!

aiyengar2 commented Apr 13, 2020

Uh oh!

aiyengar2 commented Apr 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luthermonson Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aiyengar2 commented Mar 4, 2020 •

edited

Loading

aiyengar2 commented Mar 4, 2020 •

edited

Loading

luthermonson Apr 14, 2020 •

edited

Loading