aws_identitystore_group and _user both fail badly when no result is returned.

G42makes commented 3 years ago

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform CLI and Terraform AWS Provider Version

Terraform v0.13.5
+ provider registry.terraform.io/hashicorp/aws v3.25.0

Affected Resource(s)

aws_identitystore_gorup
aws_identitystore_user

Terraform Configuration Files

The simplest code to generate this issue is just the example with a Group or User name that does not exist.

data "aws_ssoadmin_instances" "example" {}

data "aws_identitystore_group" "example" {
  identity_store_id = tolist(data.aws_ssoadmin_instances.example.identity_store_ids)[0]

  filter {
    attribute_path  = "DisplayName"
    attribute_value = "ThisDoesNotExist"
  }
}

output "group_id" {
  value = data.aws_identitystore_group.example.group_id
}

Expected Behavior

2 Options:

Return an empty list or object(preferred, as I expect to not find every lookup I'm doing)
Return an error with at least the line it was called from and what was searched

Actual Behavior

Error: no Identity Store Group found matching criteria; try different search

There are no other details around this error, in my tests I was doing a module call and got it 18 times in a row with no context(lines, search terms, etc).

Steps to Reproduce

terraform apply

Important Factoids

I'm hoping to be able to check if the group exists and is synced via this call. If it's not there, I can fall back to a default group, but I need an output I can test against. I would prefer this failed gracefully and recoverably here. If the output is empty it can fail nicely where referenced if no checking is in place.

anGie44 commented 3 years ago

Relates #17214

Hi @G42makes, thank you for raising this issue and apologies you've run into the confusing error logging. The proposed changes here #17252 address and add info in the error logging to assist in debugging.

G42makes commented 3 years ago

Relates #17214

Hi @G42makes, thank you for raising this issue and apologies you've run into the confusing error logging. The proposed changes here #17252 address and add info in the error logging to assist in debugging.

While that patch does some effort in fixing the error, I don't think that dropping an error that exits the Terraform run when the group is not found is the appropriate behaviour.

The API is obviously designed to allow future features that will return multiple groups, and 0 is a valid number of returned results, even now.

anGie44 commented 3 years ago

Definitely pre-v3.0.0 of the Provider you could find data sources that did not error on the Not Found case or 0 results, but since that major release, all singular data sources must now return an error on 0 results (Github Issue reference and rationale : https://github.com/hashicorp/terraform-provider-aws/issues/13410). So atm, our best option is to return the error with more contextual info for users, though file and line number context will still be absent for the time being (ref: https://github.com/hashicorp/terraform-plugin-sdk/issues/561).

G42makes commented 3 years ago

I think that this should not be considered an object that returns a singular data source, it just happens to return a single exact match right now. The return type of the API call is a list of group objects. The current provider code just assumes that it got one result because of the deficiencies of the wildcard matching in the Filter AttributeValue, but there is nothing I've found in the documentation that says it will return only 1 or 0 results. (And based on the console experience, multiple results are supported by the backend.)

A reference I might make is to the aws_ssoadmin_instances data source. It is similar to this in that it only really has one good output at this time, but the API specified that it returned a list and so the data source is a list. At this time AWS does not appear to support multiple SSO Instances in one account, but even then the option was made available.

Dlozitskiy commented 3 years ago

Hi @anGie44,

I agree with @G42makes that the datasource shouldn't be treated as a singular, it just happens that aws identitystore API doesn't support regex search on groups and users yet, however if you look into the response - it is provided as a list and empty list is a valid response:

> aws identitystore list-groups --identity-store-id d-*** --region *** --filters AttributePath=DisplayName,AttributeValue="blah"

output:
{
    "Groups": []
}

or

> aws identitystore list-users --identity-store-id d-*** --region *** --filters AttributePath=UserName,AttributeValue="bla"

output:
{
    "Users": []
}

I think removing both exceptions on len(results) == 0 and len(results) > 1 in aws/data_source_aws_identitystore_group.go and aws/data_source_aws_identitystore_user.go would be a right thing to do.

jgrumboe commented 3 years ago

I think it's also a limitation of the identity-store list-groups/list-users API call. It's somehow not possible to retrieve a list of multiple groups, only the exact match returns one group object. There are also others facing this problem https://forums.aws.amazon.com/thread.jspa?messageID=958818&#958818 but no solution yet. I've also created an AWS support ticket on how to do the API call to return multiple groups.

jgrumboe commented 3 years ago

I received feedback from AWS support that fetching multiple groups/users is not possible because not implemented in the official API. (There's an existing feature request already.)

But I also agree that an empty list returned should result in a valid response.

jamesgoodhouse commented 3 years ago

This definitely results in a non-traditional Terraform experience when no users/groups are returned. The data resource shouldn't fail because it did not find anything.

lorengordon commented 2 years ago

I actually appreciate the hard failure. That means someone created the name incorrectly in my environment. I'd appreciate both singular and plural data sources (e.g. aws_identitystore_group and aws_identitystore_groups), which seems to be the current way forward to handle this distinction for other data sources.

tata2000 commented 2 years ago

Is there any update or workaround this issue?

essjayhch commented 7 months ago

it would be better if we didn't set this up in a manner that forces users to adopt a specific workflow without good reason. Returning a null set of data is a perfectly reasonable scenario when the group has yet to be provisioned, and suggesting that it is an error presumes that there is no parallelisation or division of responsibility in a DevOps environment. There isn't anything specific in any DevOps law that says that the groups have to be provisioned before the accounts to which they are being bound.

hashicorp / terraform-provider-aws