From 163da483454506eee8e298a452048d8f97f0b8c1 Mon Sep 17 00:00:00 2001 From: Paul Meyer Date: Wed, 22 Jun 2016 16:04:13 -0700 Subject: [PATCH] builder/azure-arm: Make tenant_id optional Look up tenant id if not specified by the user --- builder/azure/TODO.md | 2 +- builder/azure/arm/builder.go | 8 ++- builder/azure/arm/config.go | 4 -- builder/azure/arm/config_retriever.go | 26 ++++++++ builder/azure/arm/config_retriever_test.go | 62 +++++++++++++++++++ builder/azure/arm/config_test.go | 8 +-- builder/azure/common/devicelogin.go | 15 +---- contrib/azure-setup.sh | 1 - examples/azure/debian.json | 2 - examples/azure/opensuse.json | 2 - examples/azure/ubuntu.json | 2 - examples/azure/windows.json | 2 - .../source/docs/builders/azure-setup.html.md | 16 +++-- website/source/docs/builders/azure.html.md | 4 +- 14 files changed, 109 insertions(+), 45 deletions(-) create mode 100644 builder/azure/arm/config_retriever.go create mode 100644 builder/azure/arm/config_retriever_test.go diff --git a/builder/azure/TODO.md b/builder/azure/TODO.md index 96c0eccf7..c7ac4762c 100644 --- a/builder/azure/TODO.md +++ b/builder/azure/TODO.md @@ -7,7 +7,7 @@ Here's a list of things we like to get done in no particular order: - [ ] support cross-storage account image source (ie. pre-build blob copy) - [ ] look up object id when using device code (graph api /me ?) - [ ] device flow support for Windows -- [ ] look up tenant id in all cases (see device flow code) +- [x] look up tenant id in all cases (see device flow code) - [ ] look up resource group of storage account - [ ] include all _data_ disks in artifact too - [ ] windows sysprep provisioner (since it seems to generate a certain issue volume) diff --git a/builder/azure/arm/builder.go b/builder/azure/arm/builder.go index 524eb8f38..95eb8811d 100644 --- a/builder/azure/arm/builder.go +++ b/builder/azure/arm/builder.go @@ -52,7 +52,11 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, error) { } func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packer.Artifact, error) { - ui.Say("Preparing builder ...") + ui.Say("Running builder ...") + + if err := newConfigRetriever().FillParameters(b.config); err != nil { + return nil, err + } log.Print(":: Configuration") packerAzureCommon.DumpConfig(b.config, func(s string) { log.Print(s) }) @@ -224,7 +228,7 @@ func (b *Builder) getServicePrincipalTokens(say func(string)) (*azure.ServicePri var err error if b.config.useDeviceLogin { - servicePrincipalToken, err = packerAzureCommon.Authenticate(*b.config.cloudEnvironment, b.config.SubscriptionID, say) + servicePrincipalToken, err = packerAzureCommon.Authenticate(*b.config.cloudEnvironment, b.config.SubscriptionID, b.config.TenantID, say) if err != nil { return nil, nil, err } diff --git a/builder/azure/arm/config.go b/builder/azure/arm/config.go index 89457c900..660dca94c 100644 --- a/builder/azure/arm/config.go +++ b/builder/azure/arm/config.go @@ -362,10 +362,6 @@ func assertRequiredParametersSet(c *Config, errs *packer.MultiError) { errs = packer.MultiErrorAppend(errs, fmt.Errorf("A client_secret must be specified")) } - if c.TenantID == "" { - errs = packer.MultiErrorAppend(errs, fmt.Errorf("A tenant_id must be specified")) - } - if c.SubscriptionID == "" { errs = packer.MultiErrorAppend(errs, fmt.Errorf("A subscription_id must be specified")) } diff --git a/builder/azure/arm/config_retriever.go b/builder/azure/arm/config_retriever.go new file mode 100644 index 000000000..428b159ea --- /dev/null +++ b/builder/azure/arm/config_retriever.go @@ -0,0 +1,26 @@ +package arm + +import ( + "github.com/Azure/go-autorest/autorest/azure" + "github.com/mitchellh/packer/builder/azure/common" +) + +type configRetriever struct { + // test seams + findTenantID func(azure.Environment, string) (string, error) +} + +func newConfigRetriever() configRetriever { + return configRetriever{common.FindTenantID} +} + +func (cr configRetriever) FillParameters(c *Config) error { + if c.TenantID == "" { + tenantID, err := cr.findTenantID(*c.cloudEnvironment, c.SubscriptionID) + if err != nil { + return err + } + c.TenantID = tenantID + } + return nil +} diff --git a/builder/azure/arm/config_retriever_test.go b/builder/azure/arm/config_retriever_test.go new file mode 100644 index 000000000..2fd84845f --- /dev/null +++ b/builder/azure/arm/config_retriever_test.go @@ -0,0 +1,62 @@ +package arm + +import ( + "errors" + "testing" + + "github.com/Azure/go-autorest/autorest/azure" +) + +func TestConfigRetrieverFillsTenantIDWhenEmpty(t *testing.T) { + c, _, _ := newConfig(getArmBuilderConfiguration(), getPackerConfiguration()) + if expected := ""; c.TenantID != expected { + t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) + } + + sut := newTestConfigRetriever() + retrievedTid := "my-tenant-id" + sut.findTenantID = func(azure.Environment, string) (string, error) { return retrievedTid, nil } + if err := sut.FillParameters(c); err != nil { + t.Errorf("Unexpected error when calling sut.FillParameters: %v", err) + } + + if expected := retrievedTid; c.TenantID != expected { + t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) + } +} + +func TestConfigRetrieverLeavesTenantIDWhenNotEmpty(t *testing.T) { + c, _, _ := newConfig(getArmBuilderConfiguration(), getPackerConfiguration()) + userSpecifiedTid := "not-empty" + c.TenantID = userSpecifiedTid + + sut := newTestConfigRetriever() + sut.findTenantID = nil // assert that this not even called + if err := sut.FillParameters(c); err != nil { + t.Errorf("Unexpected error when calling sut.FillParameters: %v", err) + } + + if expected := userSpecifiedTid; c.TenantID != expected { + t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) + } +} + +func TestConfigRetrieverReturnsErrorWhenTenantIDEmptyAndRetrievalFails(t *testing.T) { + c, _, _ := newConfig(getArmBuilderConfiguration(), getPackerConfiguration()) + if expected := ""; c.TenantID != expected { + t.Errorf("Expected TenantID to be %q but got %q", expected, c.TenantID) + } + + sut := newTestConfigRetriever() + errorString := "sorry, I failed" + sut.findTenantID = func(azure.Environment, string) (string, error) { return "", errors.New(errorString) } + if err := sut.FillParameters(c); err != nil && err.Error() != errorString { + t.Errorf("Unexpected error when calling sut.FillParameters: %v", err) + } +} + +func newTestConfigRetriever() configRetriever { + return configRetriever{ + findTenantID: func(azure.Environment, string) (string, error) { return "findTenantID is mocked", nil }, + } +} diff --git a/builder/azure/arm/config_test.go b/builder/azure/arm/config_test.go index 51e4facca..02b193fb2 100644 --- a/builder/azure/arm/config_test.go +++ b/builder/azure/arm/config_test.go @@ -27,7 +27,6 @@ var requiredConfigValues = []string{ "storage_account", "resource_group_name", "subscription_id", - "tenant_id", } func TestConfigShouldProvideReasonableDefaultValues(t *testing.T) { @@ -350,8 +349,8 @@ func TestUseDeviceLoginIsDisabledForWindows(t *testing.T) { } multiError, _ := err.(*packer.MultiError) - if len(multiError.Errors) != 3 { - t.Errorf("Expected to find 3 errors, but found %d errors", len(multiError.Errors)) + if len(multiError.Errors) != 2 { + t.Errorf("Expected to find 2 errors, but found %d errors", len(multiError.Errors)) } if !strings.Contains(err.Error(), "client_id must be specified") { @@ -360,9 +359,6 @@ func TestUseDeviceLoginIsDisabledForWindows(t *testing.T) { if !strings.Contains(err.Error(), "client_secret must be specified") { t.Errorf("Expected to find error for 'client_secret must be specified") } - if !strings.Contains(err.Error(), "tenant_id must be specified") { - t.Errorf("Expected to find error for 'tenant_id must be specified") - } } func TestConfigShouldRejectMalformedCaptureNamePrefix(t *testing.T) { diff --git a/builder/azure/common/devicelogin.go b/builder/azure/common/devicelogin.go index 65dcb90c7..d5cd259cd 100644 --- a/builder/azure/common/devicelogin.go +++ b/builder/azure/common/devicelogin.go @@ -39,21 +39,12 @@ var ( // Authenticate fetches a token from the local file cache or initiates a consent // flow and waits for token to be obtained. -func Authenticate(env azure.Environment, subscriptionID string, say func(string)) (*azure.ServicePrincipalToken, error) { +func Authenticate(env azure.Environment, subscriptionID, tenantID string, say func(string)) (*azure.ServicePrincipalToken, error) { clientID, ok := clientIDs[env.Name] if !ok { return nil, fmt.Errorf("packer-azure application not set up for Azure environment %q", env.Name) } - // First we locate the tenant ID of the subscription as we store tokens per - // tenant (which could have multiple subscriptions) - say(fmt.Sprintf("Looking up AAD Tenant ID: subscriptionID=%s.", subscriptionID)) - tenantID, err := findTenantID(env, subscriptionID) - if err != nil { - return nil, err - } - say(fmt.Sprintf("Found AAD Tenant ID: tenantID=%s", tenantID)) - oauthCfg, err := env.OAuthConfigForTenant(tenantID) if err != nil { return nil, fmt.Errorf("Failed to obtain oauth config for azure environment: %v", err) @@ -205,10 +196,10 @@ func validateToken(env azure.Environment, token *azure.ServicePrincipalToken) er return nil } -// findTenantID figures out the AAD tenant ID of the subscription by making an +// FindTenantID figures out the AAD tenant ID of the subscription by making an // unauthenticated request to the Get Subscription Details endpoint and parses // the value from WWW-Authenticate header. -func findTenantID(env azure.Environment, subscriptionID string) (string, error) { +func FindTenantID(env azure.Environment, subscriptionID string) (string, error) { const hdrKey = "WWW-Authenticate" c := subscriptionsClient(env.ResourceManagerEndpoint) diff --git a/contrib/azure-setup.sh b/contrib/azure-setup.sh index 446334e37..381bb9e7e 100755 --- a/contrib/azure-setup.sh +++ b/contrib/azure-setup.sh @@ -174,7 +174,6 @@ showConfigs() { echo " \"resource_group_name\": \"$azure_group_name\"," echo " \"storage_account\": \"$azure_storage_name\"," echo " \"subscription_id\": \"$azure_subscription_id\"," - echo " \"tenant_id\": \"$azure_tenant_id\"," echo "" } diff --git a/examples/azure/debian.json b/examples/azure/debian.json index b4f6fbf55..1e3f48952 100644 --- a/examples/azure/debian.json +++ b/examples/azure/debian.json @@ -5,7 +5,6 @@ "resource_group": "{{env `ARM_RESOURCE_GROUP`}}", "storage_account": "{{env `ARM_STORAGE_ACCOUNT`}}", "subscription_id": "{{env `ARM_SUBSCRIPTION_ID`}}", - "tenant_id": "{{env `ARM_TENANT_ID`}}", "ssh_user": "packer", "ssh_pass": null }, @@ -17,7 +16,6 @@ "resource_group_name": "{{user `resource_group`}}", "storage_account": "{{user `storage_account`}}", "subscription_id": "{{user `subscription_id`}}", - "tenant_id": "{{user `tenant_id`}}", "capture_container_name": "images", "capture_name_prefix": "packer", diff --git a/examples/azure/opensuse.json b/examples/azure/opensuse.json index e27ebcb9c..2bb462191 100644 --- a/examples/azure/opensuse.json +++ b/examples/azure/opensuse.json @@ -5,7 +5,6 @@ "resource_group": "{{env `ARM_RESOURCE_GROUP`}}", "storage_account": "{{env `ARM_STORAGE_ACCOUNT`}}", "subscription_id": "{{env `ARM_SUBSCRIPTION_ID`}}", - "tenant_id": "{{env `ARM_TENANT_ID`}}", "ssh_user": "packer", "ssh_pass": null }, @@ -17,7 +16,6 @@ "resource_group_name": "{{user `resource_group`}}", "storage_account": "{{user `storage_account`}}", "subscription_id": "{{user `subscription_id`}}", - "tenant_id": "{{user `tenant_id`}}", "capture_container_name": "images", "capture_name_prefix": "packer", diff --git a/examples/azure/ubuntu.json b/examples/azure/ubuntu.json index 80c759e30..0d5b176b4 100644 --- a/examples/azure/ubuntu.json +++ b/examples/azure/ubuntu.json @@ -5,7 +5,6 @@ "resource_group": "{{env `ARM_RESOURCE_GROUP`}}", "storage_account": "{{env `ARM_STORAGE_ACCOUNT`}}", "subscription_id": "{{env `ARM_SUBSCRIPTION_ID`}}", - "tenant_id": "{{env `ARM_TENANT_ID`}}" }, "builders": [{ "type": "azure-arm", @@ -15,7 +14,6 @@ "resource_group_name": "{{user `resource_group`}}", "storage_account": "{{user `storage_account`}}", "subscription_id": "{{user `subscription_id`}}", - "tenant_id": "{{user `tenant_id`}}", "capture_container_name": "images", "capture_name_prefix": "packer", diff --git a/examples/azure/windows.json b/examples/azure/windows.json index 8cb217218..9250ed92e 100644 --- a/examples/azure/windows.json +++ b/examples/azure/windows.json @@ -5,7 +5,6 @@ "resource_group": "{{env `ARM_RESOURCE_GROUP`}}", "storage_account": "{{env `ARM_STORAGE_ACCOUNT`}}", "subscription_id": "{{env `ARM_SUBSCRIPTION_ID`}}", - "tenant_id": "{{env `ARM_TENANT_ID`}}", "object_id": "{{env `ARM_OBJECT_ID`}}" }, "builders": [{ @@ -17,7 +16,6 @@ "storage_account": "{{user `storage_account`}}", "subscription_id": "{{user `subscription_id`}}", "object_id": "{{user `object_id`}}", - "tenant_id": "{{user `tenant_id`}}", "capture_container_name": "images", diff --git a/website/source/docs/builders/azure-setup.html.md b/website/source/docs/builders/azure-setup.html.md index 4e05dc778..5436b108d 100644 --- a/website/source/docs/builders/azure-setup.html.md +++ b/website/source/docs/builders/azure-setup.html.md @@ -9,7 +9,6 @@ page_title: Authorizing Packer Builds in Azure In order to build VMs in Azure Packer needs 6 configuration options to be specified: -- `tenant_id` - UUID identifying your Azure account (where you login) - `subscription_id` - UUID identifying your Azure subscription (where billing is handled) - `client_id` - UUID identifying the Active Directory service principal that will run your Packer builds - `client_secret` - service principal secret / password @@ -35,7 +34,7 @@ There are three pieces of information you must provide to enable device login mo 1. Resource Group - parent resource group that Packer uses to build an image. 1. Storage Account - storage account where the image will be placed. -> Device login mode is enabled by not setting client_id, client_secret, and tenant_id. +> Device login mode is enabled by not setting client_id and client_secret. The device login flow asks that you open a web browser, navigate to http://aka.ms/devicelogin, and input the supplied code. This authorizes the Packer for Azure application to act on your behalf. An OAuth token will be created, and stored @@ -71,16 +70,15 @@ Get your account information azure account list --json | jq .[].name azure account set ACCOUNTNAME - azure account show --json | jq ".[] | .tenantId, .id" + azure account show --json | jq ".[] | .id" --> Throughout this document when you see a command pipe to `jq` you may instead omit `--json` and everything after it, but the output will be more verbose. For example you can simply run `azure account list` instead._ +-> Throughout this document when you see a command pipe to `jq` you may instead omit `--json` and everything after it, but the output will be more verbose. For example you can simply run `azure account list` instead. -This will print out two lines that look like this: +This will print out one line that look like this: "4f562e88-8caf-421a-b4da-e3f6786c52ec" - "b68319b-2180-4c3e-ac1f-d44f5af2c6907" -The first one is your `tenant_id`. The second is your `subscription_id`. Note these for later. +This is your `subscription_id`. Note it for later. ### Create a Resource Group @@ -138,9 +136,9 @@ There are a lot of pre-defined roles and you can define your own with more granu Now (finally) everything has been setup in Azure. Let's get our configuration keys together: -Get `tenant_id` and `subscription_id`: +Get `subscription_id`: - azure account show --json | jq ".[] | .tenantId, .id" + azure account show --json | jq ".[] | .id" Get `client_id` diff --git a/website/source/docs/builders/azure.html.md b/website/source/docs/builders/azure.html.md index 82385d20e..471cc9f24 100644 --- a/website/source/docs/builders/azure.html.md +++ b/website/source/docs/builders/azure.html.md @@ -35,8 +35,6 @@ builder. - `subscription_id` (string) Subscription under which the build will be performed. **The service principal specified in `client_id` must have full access to this subscription.** -- `tenant_id` (string) The account identifier with which your `client_id` and `subscription_id` are associated. - - `capture_container_name` (string) Destination container name. Essentially the "folder" where your VHD will be organized in Azure. - `capture_name_prefix` (string) VHD prefix. The final artifacts will be named `PREFIX-osDisk.UUID` and `PREFIX-vmTemplate.UUID`. @@ -72,6 +70,8 @@ builder. - `image_url` (string) Specify a custom VHD to use. If this value is set, do not set image_publisher, image_offer, image_sku, or image_version. +- `tenant_id` (string) The account identifier with which your `client_id` and `subscription_id` are associated. If not specified, `tenant_id` will be looked up using `subscription_id`. + - `object_id` (string) Specify an OAuth Object ID to protect WinRM certificates created at runtime. This variable is required when creating images based on Windows; this variable is not used by non-Windows builds. See `Windows`