platform/azure: add kola/ore commands for Azure #771

arithx · 2017-11-29T18:03:13Z

Add support for Azure to kola and add additional ore commands utilizing the ARM API.

arithx · 2017-11-29T18:04:03Z

Marking WIP until I can perform additional testing (have been hitting an error HTTP_1_1_REQUIRED when creating subnets the past few days)

arithx · 2017-12-06T18:48:04Z

All tests that aren't excluded are now passing. Unmarking as WIP. Good to review.

dm0-

Would it work to just move this file somewhere else and set it with OPENSSL_CONF in the cert creation service?

(This was intended to be on the locksmith TLS test change; wrong text box.)

arithx · 2017-12-14T17:22:04Z

@dm0- that works on Azure, testing on other platforms currently and if everything is passing I'll push the change

arithx · 2018-05-17T16:10:25Z

Updates are pushed and this is ready for review.

I've tested all of the new ARM based functionality but do not have access to the ASM creds to test those components (the old ore commands & plume).

arithx · 2018-05-17T16:11:26Z

@dm0- in the updated locksmith TLS tests I've changed them to generate the cert into /etc/ssl/etcd.cnf and then pass that profile/extfile to the openssl commands.

auth/azure.go

ajeddeloh · 2018-05-17T19:24:06Z

auth/azure.go

@@ -148,3 +168,25 @@ func ReadAzureProfile(path string) (*AzureProfile, error) {

 	return &ap, nil
 }
+
+func DecodeUtfbomFile(contents []byte) ([]byte, error) {


Why not use https://godoc.org/golang.org/x/text/encoding/unicode

We already have dimchansky/utfbom as a vendored dependency because of Azure, switching to x/text/encoding/unicode will also require x/text/transform neither of which are currently vendored.

Do you have a preference between the two?

platform/api/azure/api.go

ajeddeloh · 2018-05-17T19:35:54Z

platform/api/azure/image.go

+		return fmt.Errorf("unable to fetch release bucket %v version: %v", a.opts.Sku, err)
+	}
+
+	scanner := bufio.NewScanner(resp.Body)


I think I remember seeing parsing this format somewhere else. Should break that out into a util package or something after this PR.

platform/api/azure/instance.go

ajeddeloh · 2018-05-17T19:51:13Z

platform/machine/azure/cluster.go

+		if err != nil {
+			return nil, err
+		}
+		ac.sshKey = keys[0].String()


is keys always going to have at least 1 element?

I believe so, when a new SSH agent (which is what the Keys function is listing) is created a key is always generated (see NewSSHAgent in network/ssh.go). We're also doing this same direct access of keys[0] in DigitalOcean.

platform/machine/azure/cluster.go

ajeddeloh · 2018-05-17T19:54:34Z

kola/tests/ignition/sethostname.go

@@ -59,17 +59,23 @@ func init() {
 		              ]
 		          }
 		      }`)
+
+	// These tests are disabled on Azure because the hostname


Maybe list the disabled tests in the commit message

ajeddeloh · 2018-05-17T19:56:03Z

README.md

+```
+$ az login`
+```
+It also requires that the environment variable `AZURE_AUTH_LOCATION` points to a JSON file. The JSON file will require a service provider active directory account to be created.


Can mantle set this if it's unset so users do not need to?

I could either add a parameter that points to this and/or make a standardized location for it and handle that if it's not already present.

But as it's user generated and not used by anything other than Microsoft/go-autorest there isn't currently a standardized location.

I'd say just pick one and go with it, needing to set an environment variable to run kola is a pain.

@bgilbert: do you have a preference?

One thing I'd add about setting this environment variable is that there isn't going to be a good way to Unset the environment variable. We tend to do platform specific configuration inside of the Cluster or API but those do not provide sufficient points for deferred cleanup of an environment variable.

IIRC environment vars should die with the process.

ajeddeloh · 2018-05-17T19:57:04Z

README.md

+The JSON file exported to the variable `AZURE_AUTH_LOCATION` should be generated by hand and have the following contents:
+```
+{
+  "clientId": "<service provider id>", 


should note this is from the az ad sp create-for-rbac

arithx · 2018-05-21T18:50:20Z

Update pushed

ajeddeloh

one nit then LGTM

ajeddeloh · 2018-06-07T21:55:36Z

cmd/ore/azure/azure.go

@@ -42,6 +43,7 @@ func init() {

 	sv := Azure.PersistentFlags().StringVar
 	sv(&azureProfile, "azure-profile", "", "Azure Profile json file")
+	sv(&azureAuth, "azure-auth", "", "Azure auth location")


can we default this to something like .azure/credentials.json. I don't really care what, but we should have a canonical location so kola runs with azure don't need special args/envs

arithx · 2018-06-13T15:28:59Z

Updates pushed

Automatically importing the credentials file (defaults to ~/.azure/credentials.json)
Fixed a bug where the API would try to resolve the image when running ore
Enabled AcceleratedNetworking on the NIC (required bumping the default size to Standard_D2_v2).

arithx · 2018-06-18T19:49:43Z

Made a small change to the create-image-arm command to output a JSON struct with the ID & Location for future jenkins work.

ajeddeloh

LEBTM

ajeddeloh

LGTM (again)

stale

arithx · 2019-06-18T17:40:18Z

rebased & updated

arithx · 2019-06-18T17:45:45Z

platform/machine/azure/flight.go

+		ac.sshKey = af.FakeSSHKey
+	}
+
+	ac.ResourceGroup, err = af.api.CreateResourceGroup("kola-cluster")


I'm going to open up an issue once this PR merges for moving the creation of the Resource Group & Storage Account into the Flight. It would have required a non-insignificant amount of refactoring to include in this change set, as such to reduce review complexity (@ajeddeloh has reviewed this code previously) I've left it as is.

jlebon

Looks sane at a high level! Will let @ajeddeloh do a final more informed ACK.

jlebon · 2019-07-03T18:42:37Z

auth/azure.go

 )

-const AzureProfilePath = ".azure/azureProfile.json"
+// A version of the Options struct from platform/api/azure that only
+// contains the ASM values. Otherwise there's a cyclical depdendency


jlebon · 2019-07-03T18:45:09Z

cmd/ore/azure/create-image-arm.go

+	Azure.AddCommand(cmdCreateImageARM)
+}
+
+func runCreateImageARM(cmd *cobra.Command, args []string) error {


I was really confused for a sec and thought Azure supported ARM.

Yeah ARM (Azure Resource Manager) can be a confusing name when looking from our usual scope.

imcleod · 2019-07-10T21:35:53Z

I tested the ARM functionality here end to end with @arithx earlier today. It LGTM and, in fact, is absolutely essential for us to push images to Azure using the type of credentials available to us these days. Put more bluntly, without this, the existing azure implementation is not useful for COSA/RHCOS/FCOS.

Any effort to get this merged quickly would be greatly appreciated.

(cc: @yuqi-zhang )

ajeddeloh

LGTM code wise. Couple minor nits. Needs a go mod tidy.

ajeddeloh · 2019-07-11T16:59:08Z

auth/azure.go

@@ -18,14 +18,39 @@ import (
 	"bytes"


Tiny nit: s/form/from/ in commit message.

ajeddeloh · 2019-07-11T17:07:08Z

platform/machine/azure/flight.go

@@ -0,0 +1,119 @@
+// Copyright 2016 CoreOS, Inc.


Should probably only have RH

ajeddeloh · 2019-07-11T17:07:47Z

platform/machine/azure/flight.go

+package azure
+
+import (
+	"fmt"


\n between go standard library and other imports

Bump the version fo the azure SDK to get required functionality for later PRs & add additional sections used by the kola API for testing.

Creates an Options struct inside of auth/azure which contains the ASM options. These are still present inside of the platform/api/azure/options struct but removes the dependency on platform/api/azure from auth/azure. This allows the azure API to call ReadAzureProfile inside of the New function.

GenerateFakeKey was added for DigitalOcean but it is now needed by Azure as well. Move it to a centralized area and update Digitalocean to use the new location.

Adds the Azure cluster. For each cluster a resource group will be created inside of Azure which is removed at the end of the clusters lifecycle (ensuring resource cleanup). The Azure API requires that all machines are given an SSH key, if the test is specifically not providing an SSH key the cluster will generate a fake key via the platform/util function GenerateFakeKey.

Updates the existing Azure API to have methods for spawning new instances, images, etc. for use with kola testing.

Adds a new version of the create-image & upload-blob commands which use ARM credentials (Azure AD service providers), as well as a GC command to clean up resource groups.

The Azure SDK requires a hostname to be set via the API during machine creation and then overwrites the hostname post-boot via wa-agent. Update the metadata/contents test to only validate COREOS_AZURE_IPV4_DYNAMIC as the machines being created are not behind a load balancer. The disabled tests on Azure are: - coreos.ignition.v1.sethostname - coreos.ignition.v2.sethostname - coreos.install.cloudinit - linux.nfs.v3 - linux.nfs.v4 - systemd.journal.remote

Updates the coreos.locksmith.tls test to use a seperate configuration file for SSL rather than overwriting the default. On Azure overwriting the default SSL config breaks wa-agent.

The updated New call for the API takes the API options which contains the auth based options inside of them.

arithx · 2019-07-11T17:32:10Z

Comments addressed

ajeddeloh

🚢 🇮🇹

arithx changed the title ~~platform/azure: add kola/ore commands for Azure~~ WIP: platform/azure: add kola/ore commands for Azure Nov 29, 2017

arithx force-pushed the kola-azure branch 2 times, most recently from de84589 to 4b02752 Compare November 29, 2017 18:30

arithx force-pushed the kola-azure branch from 4b02752 to 4316c1f Compare December 6, 2017 18:46

arithx changed the title ~~WIP: platform/azure: add kola/ore commands for Azure~~ platform/azure: add kola/ore commands for Azure Dec 6, 2017

arithx force-pushed the kola-azure branch from ed01f09 to 6081da4 Compare December 13, 2017 19:36

dm0- reviewed Dec 14, 2017

View reviewed changes

arithx force-pushed the kola-azure branch 2 times, most recently from 5e2a257 to 7df68b4 Compare May 17, 2018 16:07

ajeddeloh reviewed May 17, 2018

View reviewed changes

arithx force-pushed the kola-azure branch 3 times, most recently from 1134022 to f29b0eb Compare May 21, 2018 18:48

arithx force-pushed the kola-azure branch from f29b0eb to f2fee51 Compare June 1, 2018 16:23

ajeddeloh reviewed Jun 7, 2018

View reviewed changes

arithx force-pushed the kola-azure branch 4 times, most recently from cb358d9 to 637504d Compare June 13, 2018 15:25

ajeddeloh approved these changes Jun 18, 2018

View reviewed changes

arithx force-pushed the kola-azure branch from 637504d to 5b0b98a Compare June 18, 2018 19:48

ajeddeloh approved these changes Jun 18, 2018

View reviewed changes

arithx force-pushed the kola-azure branch from cc22acb to ab86b37 Compare June 27, 2018 19:14

ajeddeloh previously approved these changes Jun 27, 2018

View reviewed changes

arithx force-pushed the kola-azure branch from ab86b37 to 4b059cc Compare June 18, 2019 17:29

arithx commented Jun 18, 2019

View reviewed changes

This was referenced Jun 25, 2019

README: Add azure configuration #1014

Closed

WIP: Add and enable cmd-buildext-azure command coreos/coreos-assembler#569

Closed

jlebon reviewed Jul 3, 2019

View reviewed changes

arithx force-pushed the kola-azure branch 2 times, most recently from 110ec5f to a108da6 Compare July 10, 2019 14:12

ajeddeloh approved these changes Jul 11, 2019

View reviewed changes

arithx added 11 commits July 11, 2019 12:30

vendor: bump azure version & pull in additional libraries

b59ed2e

Bump the version fo the azure SDK to get required functionality for later PRs & add additional sections used by the kola API for testing.

platform: move GenerateFakeKey to platform/util

05bee9c

GenerateFakeKey was added for DigitalOcean but it is now needed by Azure as well. Move it to a centralized area and update Digitalocean to use the new location.

platform/api/azure: add kola related Azure APIs

c084b9c

Updates the existing Azure API to have methods for spawning new instances, images, etc. for use with kola testing.

cmd/ore/azure: add ARM based image creation/upload & GC

d282473

Adds a new version of the create-image & upload-blob commands which use ARM credentials (Azure AD service providers), as well as a GC command to clean up resource groups.

cmd/kola: add Azure

a3a1a2e

kola/tests/locksmith/locksmith: update coreos.locksmith.tls

b0162fb

Updates the coreos.locksmith.tls test to use a seperate configuration file for SSL rather than overwriting the default. On Azure overwriting the default SSL config breaks wa-agent.

cmd/plume: update Azure to build API options

499ed4a

The updated New call for the API takes the API options which contains the auth based options inside of them.

README: add Azure configuration docs

f9bd137

arithx force-pushed the kola-azure branch from a108da6 to f9bd137 Compare July 11, 2019 17:31

ajeddeloh approved these changes Jul 11, 2019

View reviewed changes

arithx merged commit 01320db into coreos:master Jul 11, 2019

arithx deleted the kola-azure branch July 11, 2019 17:42

dustymabe mentioned this pull request Aug 26, 2020

mantle: add support for IBM cloud coreos/fedora-coreos-tracker#614

Open

platform/azure: add kola/ore commands for Azure #771

platform/azure: add kola/ore commands for Azure #771

Conversation

arithx commented Nov 29, 2017

arithx commented Nov 29, 2017

arithx commented Dec 6, 2017

dm0- left a comment • edited Loading

Choose a reason for hiding this comment

arithx commented Dec 14, 2017

arithx commented May 17, 2018

arithx commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arithx May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arithx commented May 21, 2018

ajeddeloh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arithx commented Jun 13, 2018

arithx commented Jun 18, 2018

ajeddeloh left a comment

Choose a reason for hiding this comment

ajeddeloh left a comment

Choose a reason for hiding this comment

arithx commented Jun 18, 2019

arithx Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imcleod commented Jul 10, 2019

ajeddeloh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arithx commented Jul 11, 2019

ajeddeloh left a comment

Choose a reason for hiding this comment

dm0- left a comment •

edited

Loading

arithx May 17, 2018 •

edited

Loading

arithx Jun 18, 2019 •

edited

Loading