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

aws_sagemaker_domain sharing settings s3 kms key id fails with kms id expecting kms arn #23429

Closed
purplexed opened this issue Mar 1, 2022 · 4 comments · Fixed by #32661
Closed
Labels
bug Addresses a defect in current functionality. service/sagemaker Issues and PRs that pertain to the sagemaker service.

Comments

@purplexed
Copy link

purplexed commented Mar 1, 2022

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

Providers:

  • Using hashicorp/archive v2.2.0 from the shared cache directory
  • Using hashicorp/aws v4.3.0 from the shared cache directory
  • Using hashicorp/template v2.2.0 from the shared cache directory

Affected Resource(s)

aws_sagemaker_domain (maybe aws_sagemaker_user_profile too)

Terraform Configuration Files

Please include all Terraform configurations required to reproduce the bug. Bug reports without a functional reproduction may be closed without investigation.

resource "aws_sagemaker_domain" "studio-domain" {
  domain_name             = var.app
  auth_mode               = "IAM"
  vpc_id                  = data.terraform_remote_state.vpc.outputs.vpc_id
  subnet_ids              = flatten(data.terraform_remote_state.vpc.outputs.subnets)
  kms_key_id              = aws_kms_key.key.arn
  app_network_access_type = "VpcOnly"

  default_user_settings {
    execution_role  = aws_iam_role.studio.arn
    security_groups = [aws_security_group.studio.id]
    sharing_settings {
      notebook_output_option = "Allowed"
      s3_kms_key_id = aws_kms_key.key.id
      s3_output_path = "s3://${aws_s3_bucket.mybucket.id}/sharing"
    }
  }
}

Debug Output

Panic Output

Plan output:

��� Error: "default_user_settings.0.sharing_settings.0.s3_kms_key_id" (<KMS KEY ID>) is an invalid ARN: arn: invalid prefix
��� 
���   with aws_sagemaker_domain.studio-domain,
���   on sagemaker_studio.tf line 14, in resource "aws_sagemaker_domain" "studio-domain":
���   14:       s3_kms_key_id = aws_kms_key.key.id

Expected Behavior

The KMS Key ID to be accepted.

Actual Behavior

See plan output error.

Steps to Reproduce

terraform apply
change the `s3_kms_key_id = aws_kms_key.key.id` to `s3_kms_key_id = aws_kms_key.key.arn`
terraform apply

Important Factoids

It errors when I provide the KMS key ID. Terraform, the CLI and the console they all point to the KMS key ID, and not the ARN, so not sure why it expects the ARN.

@github-actions github-actions bot added needs-triage Waiting for first response or review from a maintainer. service/sagemaker Issues and PRs that pertain to the sagemaker service. labels Mar 1, 2022
@purplexed
Copy link
Author

purplexed commented Mar 1, 2022

I think it's related to this: https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/service/sagemaker/domain.go#L108

I don't think there should be an ARN check, since the expected input is an id.

@justinretzolk justinretzolk added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Mar 1, 2022
@christrotter
Copy link

This same thing is also happening in the s3 resource. e.g.

      apply_server_side_encryption_by_default {
        kms_master_key_id = aws_kms_key.keynamehere.key_id
        sse_algorithm     = "aws:kms"
      }

That code applies fine, but you end up with weirdo behaviour, like ACM Private CA trying to call for that key-id on an AccountId that belongs to AWS. Correct application needs to be .arn, but you can apply with .key_id and it shows up "correct" inside of the S3 properties console. So it also looks like there isn't field validation there, too?

@bpmcd
Copy link
Contributor

bpmcd commented Jul 11, 2023

The same error is true for the kms_key_id property at the top-level of the SageMaker Domain schema. The provider code should not provide the validation function for an ARN.

@github-actions
Copy link

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/sagemaker Issues and PRs that pertain to the sagemaker service.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants