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.

Sign up for GitHub

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

Jump to bottom

New Resource: `azurerm_azuread_application` #1269

Merged

tombuildsstuff merged 5 commits into hashicorp:master from Stavrus:ad_application

Jul 11, 2018

Contributor

Stavrus commented May 21, 2018

Split from PR #961.

tombuildsstuff added new-resource service/authentication labels

tombuildsstuff requested review from katbyte and tombuildsstuff and removed request for katbyte

May 22, 2018 19:43

tombuildsstuff suggested changes

View reviewed changes

Contributor

tombuildsstuff left a comment

Thanks for this PR & splitting it out from the other PR :)

I've taken a look through and left some comments in-line - my main question at this point is would it be prudent to split the AD Application up into multiple sub-resources? e.g. azurerm_azuread_application / azurerm_azuread_application_password / azurerm_azuread_application_certificate since I could see these being added independent of an application. For example, a user may by assigned an Application by a central team, and have permissions to add Passwords/Certificates to it, but doesn't have permissions to create a new one.

I've also got a separate question around the naming of this resource - given the product name is Azure AD perhaps these resources should contain that phrase? All that said, this is off to a good start :)

Thanks!

azurerm/import_arm_ad_application_test.go Outdated

+              	"github.com/hashicorp/terraform/helper/resource"
+              )
+              func TestAccAzureRMAdApplication_importSimple(t *testing.T) {

Contributor

tombuildsstuff May 24, 2018

can we update the test prefixes to be TestAccAzureRMActiveDirectoryApplication_ to make them consistent with the other resources?

azurerm/key_credentials.go Outdated

+              	l["type"] = *cred.Type
+              	l["usage"] = *cred.Usage
+              	l["start_date"] = string((*cred.StartDate).Format(time.RFC3339))
+              	l["end_date"] = string((*cred.EndDate).Format(time.RFC3339))

Contributor

tombuildsstuff May 24, 2018

can we nil-check each of these, else there's a potential crash here?

azurerm/key_credentials.go Outdated

+              	return hashcode.String(buf.String())
+              }
+              func flattenAzureRmKeyCredential(cred *graphrbac.KeyCredential) interface{} {

Contributor

tombuildsstuff May 24, 2018

looks like we can remove the pointer from this and avoid making a nil-check for the cred object?

azurerm/key_credentials.go Outdated

+              }
+              func flattenAzureRmKeyCredentials(creds *[]graphrbac.KeyCredential) []interface{} {
+              	result := make([]interface{}, 0, len(*creds))

Contributor

tombuildsstuff May 24, 2018

there's a potential crash here if creds is nil (which can happen in the case of a bad API response) - as such can we make this:

result := make([]interface{}, 0)

azurerm/key_credentials.go Outdated

+              func flattenAzureRmKeyCredentials(creds *[]graphrbac.KeyCredential) []interface{} {
+              	result := make([]interface{}, 0, len(*creds))
+              	for _, cred := range *creds {
+              		result = append(result, flattenAzureRmKeyCredential(&cred))

Contributor

tombuildsstuff May 24, 2018

minor since it's minor can we in-line this function?

azurerm/resource_arm_ad_application.go Outdated


		"password_credential": passwordCredentialsSchema(),

		"app_id": {

Contributor

tombuildsstuff May 24, 2018

can we make this application_id to be consistent with other fields in the Provider? also - is this ID different to the one used for the id field?

Contributor Author

Stavrus May 30, 2018

Yes, the app_id is different from id. It's object_id that's redundant so I'll be removing that.

azurerm/resource_arm_ad_application.go Outdated

+              			"available_to_other_tenants": {
+              				Type:     schema.TypeBool,
+              				Optional: true,
+              				Computed: true,

Contributor

tombuildsstuff May 24, 2018

IMO we should make this a Required field?

Contributor Author

Stavrus May 30, 2018

Sure, not a problem. To give some context when picking what to set as default I followed the Azure CLI's az ad app create which only has the display name as a required parameter.

I think there's also some extra AD permissions required in setting it to true (default is false), but I would have to look into which permissions these are.

azurerm/resource_arm_ad_application.go Outdated

+              				Type:     schema.TypeList,
+              				Optional: true,
+              				Computed: true,
+              				MinItems: 1,

Contributor

tombuildsstuff May 24, 2018

given this has to be specified at create time - isn't this Required?

Contributor Author

Stavrus May 30, 2018

You can edit these at any time to add/remove as needed, and they're not required for creation. I believe the bare minimum properties required for creating an application are the display name, 1 identifier URI, and the homepage. Those last two properties attempt to set default values for you based on the display name you give in their expand functions as a convenience, but the expand for reply_urls doesn't as it's not required. I modeled the default functionality based on how Azure CLI's az ad app create works.

azurerm/resource_arm_ad_application.go Outdated

+              			"identifier_uris": {
+              				Type:     schema.TypeList,
+              				Optional: true,

Contributor

tombuildsstuff May 24, 2018

is this field user-specifyable in the portal?

Contributor Author

Stavrus May 30, 2018

I believe this is the "App ID URI" under settings->properties. You can't specify it during creation in the portal but can modify it afterwards as long as it's globally unique. The manifest section, which allows you to edit all the application settings as a JSON document, is another way of changing it through the portal but that's probably cheating :)

azurerm/resource_arm_ad_application.go Outdated

+              		return fmt.Errorf("Error loading Application %q: %+v", d.Id(), err)
+              	}
+              	d.Set("display_name", resp.DisplayName)

Contributor

tombuildsstuff May 24, 2018

can we access all of these properties on resp.[XX]Properties? it's how they're returned from the API - these fields aren't necessarily guaranteed to be populated. This also means we can do a nil-check e.g.

if props := resp.[XX]Properties; props != nil {
  d.Set("name", resp.DisplayName)
  # ...
}

tombuildsstuff added the waiting-response label

tombuildsstuff changed the title ~~Add an AD Application provider.~~ New Resource: azurerm_azuread_application

Contributor Author

Stavrus commented May 30, 2018

@tombuildsstuff Yes, it totally makes sense to split the KeyCredentials and PasswordCredentials out like that and I'll pull them out into new resources with new PRs* as I make your requested fixes to this one.

I do want to clarify that it is possible to assign those credentials to ServicePrincipals as well. I believe this is to support multi-tenanted applications, allowing individual credentials per tenant. It's not a feature that appears in the portal but the endpoints are there in the SDK. Given the above, this will leave us with 4 credentials resources in total: azurerm_azuread_application_password / azurerm_azuread_application_certificate / azurerm_azuread_serviceprincipal_password / azurerm_azuread_serviceprincipal_certificate.

* Since their tests are dependent on this application resource I'll have to wait on actually creating them until this one's merged to simplify the PR process.

tombuildsstuff removed the waiting-response label

Contributor

tombuildsstuff commented Jun 1, 2018

I do want to clarify that it is possible to assign those credentials to ServicePrincipals as well. I believe this is to support multi-tenanted applications, allowing individual credentials per tenant. It's not a feature that appears in the portal but the endpoints are there in the SDK. Given the above, this will leave us with 4 credentials resources in total: azurerm_azuread_application_password / azurerm_azuread_application_certificate / azurerm_azuread_serviceprincipal_password / azurerm_azuread_serviceprincipal_certificate.

@Stavrus that's fair - my understanding is that these endpoints are for the v2 / new authorization endpoints which are still being rolled out; for the moment (e.g. until these are more widely available) I think we only need to support the Application, Service Principal, Service Principal Certificate and Service Principal Key resources (e.g. we can ignore the azurerm_azuread_application_password / azurerm_azuread_application_certificate resources)?

Contributor Author

Stavrus commented Jul 4, 2018

@tombuildsstuff Hey, sorry for the delay in updating this PR. I've made most of the requested changes and it should be ready for re-review. I'll be working on the Password/Key resources in the meantime.

What I left out/need further comment on:

"available_to_other_tenants" is still optional. I'm leaning towards keeping the smallest use-case of the resource -- that is, the minimum required parameters -- to just the name as this is how it is for the Azure CLI az ad app create command.
resp.[XX]Properties - I believe you're referring to something like this? It doesn't exist for an AD object. The closest I can get is the AdditionalProperties map, but I don't think that's what you're intending.
Password/Key resources should be for Applications and not Service Principals, according to this.

tombuildsstuff mentioned this pull request

Feature Request: azurerm_service_principal #16

Closed

Xavier Gallardo added 4 commits

July 11, 2018 18:54


          Add an AD Application provider.

0c827de

Split from PR hashicorp#961.


          Rename resource to azurerm_azuread_application and update function na…

0eddb37

…mes to match.


          Address code review comments.

11d6f1c


          Remove Password/Key Credentials to turn them into resources.

9e45c65

tombuildsstuff self-assigned this


          Refactoring:

ef6167d

- updating the acceptance tests to prefix the name with `acctest` so it's easily identifyable
- wrapping the error messages to display more useful information
- ensuring we disable apps or other tenants before deleting them
- making available_to_other_tenants / oauth2_allow_implicit_flow not computed
- updating the docs

```
 $ acctests azurerm TestAccAzureRMActiveDirectoryApplication_
=== RUN   TestAccAzureRMActiveDirectoryApplication_importBasic
--- PASS: TestAccAzureRMActiveDirectoryApplication_importBasic (38.38s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_importComplete
--- PASS: TestAccAzureRMActiveDirectoryApplication_importComplete (25.42s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_basic
--- PASS: TestAccAzureRMActiveDirectoryApplication_basic (20.18s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_availableToOtherTenants
--- PASS: TestAccAzureRMActiveDirectoryApplication_availableToOtherTenants (21.60s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_complete
--- PASS: TestAccAzureRMActiveDirectoryApplication_complete (20.19s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_update
--- PASS: TestAccAzureRMActiveDirectoryApplication_update (39.92s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	165.735s
```

tombuildsstuff approved these changes

View reviewed changes

Contributor

tombuildsstuff left a comment

Thanks for pushing those changes - I've taken a look through and spent some time testing this and it's pretty close. Whilst testing this I've noticed a couple of issues which I'm going to push a commit to fix (I hope you don't mind!) - and this now LGTM 👍

Thanks!

azurerm/resource_arm_azuread_application.go Outdated

+              	d.Set("application_id", resp.AppID)
+              	d.Set("homepage", resp.Homepage)
+              	d.Set("identifier_uris", resp.IdentifierUris)
+              	d.Set("reply_urls", resp.ReplyUrls)

Contributor

tombuildsstuff Jul 11, 2018

given these are complex objects - I'm going to push a commit to add a flatten function for each of these and check for errors when setting them

azurerm/resource_arm_azuread_application.go Outdated

+              	d.Set("homepage", resp.Homepage)
+              	d.Set("identifier_uris", resp.IdentifierUris)
+              	d.Set("reply_urls", resp.ReplyUrls)
+              	d.Set("available_to_other_tenants", resp.AvailableToOtherTenants)

Contributor

tombuildsstuff Jul 11, 2018

in some manual testing of this resource - it appears this property needs to be false upon deletion; I'll push a commit to ensure this is set before deleting the application

azurerm/resource_arm_azuread_application.go Outdated

+              	}
+              	if len(identifiers) == 0 {
+              		identifiers = append(identifiers, fmt.Sprintf("http://%s", name))
+              	}

Contributor

tombuildsstuff Jul 11, 2018

looks like we don't need this from testing - so I'll push a commit to remove it

website/docs/r/azuread_application.html.markdown Outdated

+              ---
+              layout: "azurerm"
+              page_title: "Azure Resource Manager: azurerm_azuread_application"
+              sidebar_current: "docs-azurerm-resource-active-directory-application"

Contributor

tombuildsstuff Jul 11, 2018

minor let's update this to docs-azurerm-resource-azuread-application

tombuildsstuff force-pushed the ad_application branch from 3d151ad to ef6167d Compare

July 11, 2018 20:05

tombuildsstuff removed the request for review from katbyte

July 11, 2018 20:14

Contributor

tombuildsstuff commented Jul 11, 2018

Tests pass:

 $ acctests azurerm TestAccAzureRMActiveDirectoryApplication_
=== RUN   TestAccAzureRMActiveDirectoryApplication_importBasic
--- PASS: TestAccAzureRMActiveDirectoryApplication_importBasic (38.38s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_importComplete
--- PASS: TestAccAzureRMActiveDirectoryApplication_importComplete (25.42s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_basic
--- PASS: TestAccAzureRMActiveDirectoryApplication_basic (20.18s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_availableToOtherTenants
--- PASS: TestAccAzureRMActiveDirectoryApplication_availableToOtherTenants (21.60s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_complete
--- PASS: TestAccAzureRMActiveDirectoryApplication_complete (20.19s)
=== RUN   TestAccAzureRMActiveDirectoryApplication_update
--- PASS: TestAccAzureRMActiveDirectoryApplication_update (39.92s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	165.735s

tombuildsstuff added this to the 1.9.0 milestone

tombuildsstuff merged commit 30f0dfa into hashicorp:master

tombuildsstuff added a commit that referenced this pull request


          Updating to include #1269

365903b

torresdal added a commit to torresdal/terraform-provider-azurerm that referenced this pull request


          Merge branch 'api-management' into api-management-api

2004c75

* api-management:
  Updating to include hashicorp#1565
  resource/servicebus_subscription_rule: Fix the correlation_filter optional values (hashicorp#1565)
  Updating to include hashicorp#1563
  resouce/app_insights: Allow different application_type deployments (hashicorp#1563)
  VMSS: changed sku property from a set to list
  Update CHANGELOG.md to include hashicorp#1552
  Renamed azurerm_azuread_application.html.markdown to azuread_application.html.markdown
  New Data Source: `azurerm_azuread_application`
  VMSS: Updating the code samples to be valid. Fixes hashicorp#1539 (hashicorp#1549)
  Creation Data -> Creation Date (hashicorp#1548)
  Updating to include hashicorp#1546
  Workaround upstream issue in creating azureEndpoints in traffic manager (hashicorp#1546)
  Cleanup after v1.9.0 release
  v1.9.0
  Updating to include hashicorp#1269
  New Resource: `azurerm_azuread_application` (hashicorp#1269)
  Remove tags validate in preConfig of TestAccAzureRMKeyVault_update (hashicorp#1534)
  Updating to include hashicorp#1535
  `azurerm_key_vault_key` - handling the parent Key Vault being deleted (hashicorp#1535)

ghost commented Mar 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

ghost locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

new-resource service/authentication