Allow for graceful handling of errored users within users data source

[patrickmarabeas]

Return all user data in users parameter aray

• Add users parameter - an array of users which mirrors that of the user
  data source. This allows for additional validation of users - such as
  checking whether the account is enabled.

Allow user lookup to gracefully fail

• Add graceful_errors parameter to allow user lookup to gracefully fail.
  This allows only users which were successfully looked up to return. The
  data source will still result in an error if no users were returned.

Documentation updated to include the two new parameters.

Add missing MailNickname validation logic

[manicminer]

Hi @patrickmarabeas, thanks for this change suggestion. I'm looking to add similar functionality to the azuread_users data source (#258), however I'm wary of adding this to a singular data source as we'd usually expect a data source to always err when a resource isn't found. Would you be able to make use of #258 in your configuration?

[patrickmarabeas]

@manicminer that data source would also need to be uplifted to gracefully fail when a user being queried returns an error. I'm certainly happy to expand the scope of this PR to provide graceful failure to multiple data sources, but would like to confirm the approach before spending the time.

[manicminer]

@patrickmarabeas thanks for replying, I got mixed up and see now your change is quite different - sorry about that.

If I'm honest, I'm not sure we should enable that behavior in a data source as it abstracts from API semantics and may arguably go against provider design principles. However, if we did enable this, I also don't think we could implement it reliably. There are cases where the API returns 404 for a resource that does or should exist - due to replication delays or other circumstances. In such events, we couldn't distinguish between a genuine error and one the operator wishes to ignore.

[patrickmarabeas]

@manicminer We are simply looking for graceful failure - regardless of the error. A renaming of the parameters would probably help this intent. But basically, if you opt into graceful failure you are expecting this result. This is certainly not something that should be enabled by default.

In our case, we are looking for accounts to be gracefully and automatically removed from teams if the user data source returns an error. On/off-boarding of accounts in AzureAD is disconnected from the management of teams and we are looking to solve the blocker of Terraform failing when accounts no longer exist. We were hoping that accounts were simply being disabled (which we can check for via the account_enabled prop) - but this is only true for AD.

[tombuildsstuff]

@patrickmarabeas it's worth mentioning there's a couple of conventions in Terraform for Data Sources:

• Singular Data Sources (e.g. azuread_user) should fail if the specified resource can't be found
• Plural Data Sources (e.g. azuread_users) should fail if 0 items can be found

As such in this instance I'm wondering if the azuread_users data source should be updated to support passing in a list of email addresses (or similar) identifiers (potentially with an "enabled" filter) - which should solve this the other way.

Whilst we've (unintentionally) had some Data Sources in the past which didn't raise an error when the resource wasn't found - we ended up reverting these to bring this into line with Terraform, as such I think we'd be better to add this to the Plural Data Source here rather than the Singular.

What do you think? :)

[patrickmarabeas]

@tombuildsstuff I'm happy with this approach. I will migrate the changes across to the azuread_users data source allowing for graceful handling of errors - even in the case of all accounts erroring.

[tombuildsstuff]

@patrickmarabeas to match the behaviour of the rest of Terraform, we'd still need this to error if no accounts are returned from the azurerm_users data source. That said the Azure API should still return accounts when they're disabled - presumably just with the "Disabled/Enabled" flag set to "Disabled" - so are these accounts being deleted?

[patrickmarabeas]

Yes, the accounts are being deleted.

Disabled filtering is something that can be performed by the implementor.

Having thought on it, the behaviour of an error when no users are returned makes sense - as the group would have no members. Probably a worthy time to call it out.

I will follow this logic.

EDIT:

Looking at the azuread_users data source, I think the whole thing should be uplifted to return an array of user objects with the same parameters as azuread_user - this could be done as an additional users param to retain backwards compatibility.

Additional params to add:

graceful_account_errors - bool defaulting to false

users.user.account_error - bool computed

[patrickmarabeas]

@manicminer @tombuildsstuff

graceful_errors parameter added. Data source will error if no users are returned, however.

users parameter added which mirrors the user data source.

account_error was not required, as it's obviously filtered out on error.

[manicminer]

Hi @patrickmarabeas, many thanks, this looks great! I've added inline comments for some small changes, and a question on sanity checking the API response.

[patrickmarabeas]

Changes have been amended and branch rebased.

[manicminer]

@patrickmarabeas This is looking great. Sorry but an omission on my part - we'll need some additional test coverage for the case of ignore_missing = true and one user out of say 3 being omitted from the result. I'm happy to add this if you can give me contributor push access to your branch, else feel free to do so :)

[patrickmarabeas]

@manicminer I don't have anyway to run the tests, so if you could do this that would be fantastic. I've given you collaborator access on my fork.

[manicminer]

I've rebased and added a test for the ignoreMissing attribute, and altered behavior slightly to allow returning no results in all cases, as this is probably expected given the current behavior of the data source (with a note to consider changing this in future).

Since I've touched the code, I'm opening for review from another contributor :)

[katbyte]

LGTM 👍

[patrickmarabeas]

@manicminer

Getting an error when testing this now.

panic: runtime error: invalid memory address or nil pointer dereference
..
2020-06-30T18:33:37.488+1000 [DEBUG] plugin.terraform-provider-azuread_v0.11.0: 	/Users/marabeap/Code/github-com/terraform-providers/terraform-provider-azuread/azuread/data_users.go:163 +0x1b27

Which is this block:

if ignoreMissing && ar.ResponseWasNotFound(u.Response) {
    break
}

[manicminer]

@patrickmarabeas Can you share configuration that reproduces the panic? Tests seem to work for me.

[manicminer]

I found the issue, will work up a fix shortly.

[manicminer]

@patrickmarabeas this should fix it: #289

Thanks for testing and reporting!

[ghost]

This has been released in version 0.11.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azuread" {
    version = "~> 0.11.0"
}
# ... other configuration ...

[ghost]

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!