Skip to content
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

[WIP] Data sources for azure applications #1349

Closed

Conversation

monkey-jeff
Copy link

Hi this is really just a proof of concept of a feature. I know i still need to write tests and documentation but before i went to far down that road I wanted to get some feedback on this feature. This would be the first support to the AAD apis in azure to get a list of Azure Directory Objects. The use case I have put forth is getting application names from a filter.

This is quite rough but wondering if I should continue down the road of improving and getting some direction before I go to far in a direction that might not be the way you guys would want to go with this.

Here is some example code and its outputs based upon the code I wrote here.

Code example

data "azurerm_aad_application" "selected" {
  filter {
    name = "displayName"
    values = ["sample-ms"]
  }
}

data "azurerm_aad_application_names" "selected" {
  filter {
    name = "displayName"
    prefix = true
    values = ["sample-ms","DoesThisWork"]
  }
}

result

application_id = 2be2cc44-0627-48ee-b52f-3f53fa6ff5f1
applications = [
    sample-ms,
    sample-ms.test.account,
    sample-ms2,
    DoesThisWork,
    DoesThisWork2,
    DoesThisWork3
]
homepage = http://sample-ms
object_id = 2b66442c-52b9-4cf8-995a-dbabc0f16574

Let me know if this is something you guys would be interested in. I know I have another PR open right now and will address those comments but thought I would get some feedback on this potential feature as well.

Also note: There seems to have been a bug in the auth code that was only an issue if not authenticating via service principal. I added the following code to get a new token that would be good for microsoft graph

	err = spt.Refresh()

	if err != nil {
		return nil, err
	}

According to the go sdk documentation when using adal.NewServicePrincipalTokenFromManualToken you need to refresh the token to get a new one. Otherwise you get a 401 on the graph resource.

Thank you for your time and consideration!

-Jeff

@monkey-jeff
Copy link
Author

@tombuildsstuff Now that the other PR was merged I will continue on this as well. Any suggestions on the path forward here or am I going in the right direction and should just continue with it?

tombuildsstuff added a commit that referenced this pull request Jul 11, 2018
This fix was originally contributed by @sophos-jeff in #1349 but has been split-out

This allows Azure CLI auth to be used to accessing Key Vaults, which fixes #656.
return nil, err
}

err = spt.Refresh()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a great spot - so that we can ship this fix faster I've split this out into #1544 (and credited you in that PR) - I hope you don't mind

@tombuildsstuff
Copy link
Contributor

hey @sophos-jeff

Thanks for this PR - apologies for the delayed review here, we needed to get the AzureAD Application Resource in first so that we could work out how's best to proceed with this.

I've spent a little time looking into how we'd add an AzureAD Application Data Source and I think there's two Data Sources needed here:

  • azurerm_azuread_application - which returns the AD Application based on it's ID or Name
  • azurerm_azuread_applications - which returns a list of Object ID's, which can then be used in conjunction with the azurerm_azuread_application data source, something like this:
data "azurerm_azuread_applications" "examples" {
  # could also be name_prefixes, or name_regex
  name_prefix = "sample-ms"
}


data "azurerm_azuread_application" "example" {
  count = "${len(data.azurerm_azuread_applications.examples.object_ids)}"
  object_id = "${data.azurerm_azuread_applications.examples.object_ids[count.index]}"
}

output "application_names" {
  value = "${azurerm_azuread_application.example.*.name}"
}

Whilst I've been working through this, I've ended up prototyping enough of azurerm_azuread_application that I'm going to open a PR for it - in which case can we focus this PR on supporting the azurerm_azuread_applications resource. What do you think of the above proposal?

Thanks!

@monkey-jeff
Copy link
Author

That sounds good @tombuildsstuff

If you can please tag me in your PR so I am aware of it and I will pivot this to be azurerm_azuread_applications (Or actualyl probably close this one and open a new one for applications. Ill let you make the first step here though and will follow your lead.

tombuildsstuff added a commit that referenced this pull request Jul 16, 2018
#1544)

This fix was originally contributed by @sophos-jeff in #1349 but has been split-out

This allows Azure CLI auth to be used to accessing Key Vaults, which fixes #656.
@tombuildsstuff
Copy link
Contributor

@sophos-jeff sorry I'd missed I'd not replied to this - support for the AD Application Data Source was added in #1552

@tombuildsstuff
Copy link
Contributor

hey @sophos-jeff

Support for AzureAD Applications and Service Principals is now available in Terraform (and there's Data Sources which allow filtering for these by name) - as such I believe the purpose of this PR has changed and I'm going to close this PR for the moment. If you're still looking to have support for filtering Azure AD Applications based on the prefix of the name - would you mind opening a new Issue about this / opening a PR with a new Data Source specifically for that?

Thanks!

@monkey-jeff
Copy link
Author

Awesome thanks tom. Sorry I never got to this. Been very busy but glad to see these features added.

@ghost
Copy link

ghost commented Mar 6, 2019

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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 6, 2019
@ghost ghost removed the waiting-response label Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants