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

[OPAL-7906] Support AWS accounts in Terraform #24

Conversation

andrewsy-opal
Copy link
Collaborator

@andrewsy-opal andrewsy-opal commented Aug 10, 2023

Description of the change

We were out of parity with the SDK, this supports it.

Test AWS accounts:
Screenshot 2023-08-10 at 11 15 34 AM

Test AWS role:
Screenshot 2023-08-10 at 11 31 57 AM

Checklist

  • I performed a self-review of my code
  • I manually tested my code change (please list details in description)
  • I added unit tests
  • I updated the changelog
  • I updated the public facing docs

@linear
Copy link

linear bot commented Aug 10, 2023

OPAL-7906 [P1] - Sophos | AWS Organizations - Terraform fails for Opal AWS IAM Role Resource

Terraform is failing for AWS Organizations using AWS IAM Roles:

TF update, I've tried to get the IAM Roles added via TF and get an error "Error: opal api error: 400 Bad Request: account_id is required for aws sso app" Looking at the API docs there is an option to add account_id for AWS_IAM_ROLE type Opal resource, https://docs.opal.dev/reference/createresource

"remote_info": { "aws_iam_role": { "account_id": 234234234234, "arn": "arn:aws:iam::179308207300:role/MyRole" } },
I've tried the API without the account_id and get the same error, so it looks like its required but not available in the TF provider, you can only specify the arn https://registry.terraform.io/providers/opalsecurity/opal/latest/docs/resources/resource#nested-schema-for-remote_infoaws_iam_role

Please can we get this added to the remaining TF provider work along with getting the "AWS_ACCOUNT" Opal resource type available in the TF provider, as they're both in the same area, thanks.

@andrewsy-opal andrewsy-opal force-pushed the andrewsy/opal-7906-p1-sophos-aws-organizations-terraform-fails-for-opal-aws-iam branch from 356075f to d54835e Compare August 10, 2023 14:23
@andrewsy-opal andrewsy-opal force-pushed the andrewsy/opal-7906-p1-sophos-aws-organizations-terraform-fails-for-opal-aws-iam branch from d54835e to c2d860e Compare August 10, 2023 14:26
@andrewsy-opal andrewsy-opal force-pushed the andrewsy/opal-7906-p1-sophos-aws-organizations-terraform-fails-for-opal-aws-iam branch from c2d860e to 661382d Compare August 10, 2023 14:28
@andrewsy-opal andrewsy-opal temporarily deployed to Test August 10, 2023 14:35 — with GitHub Actions Inactive
@andrewsy-opal andrewsy-opal force-pushed the andrewsy/opal-7906-p1-sophos-aws-organizations-terraform-fails-for-opal-aws-iam branch from 1901ab6 to 15a80e7 Compare August 10, 2023 15:14
@andrewsy-opal andrewsy-opal temporarily deployed to Test August 10, 2023 15:14 — with GitHub Actions Inactive
@andrewsy-opal andrewsy-opal temporarily deployed to Test August 10, 2023 16:07 — with GitHub Actions Inactive
Copy link
Contributor

@ken-opal ken-opal left a comment

Choose a reason for hiding this comment

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

one question - do we want to support reading out the remote info for other AWS resources?

if resource.Metadata != nil {
remoteInfoIList := make([]any, 0, 1)
switch *resource.ResourceType {
case opal.RESOURCETYPEENUM_AWS_SSO_PERMISSION_SET:
// TODO: Handle other AWS Orgs resource types
var metadata opal.AwsPermissionSetMetadata
if err := json.Unmarshal([]byte(*resource.Metadata), &metadata); err != nil {
return diagFromErr(ctx, err)
}
permissionSetIList := make([]any, 0, 1)
permissionSetIList = append(permissionSetIList, map[string]any{
"arn": metadata.AwsPermissionSet.Arn,
"account_id": metadata.AwsPermissionSet.AccountId,
})
remoteInfoIList = append(remoteInfoIList, map[string]any{
"aws_permission_set": permissionSetIList,
})
}
if len(remoteInfoIList) == 1 {
d.Set("remote_info", remoteInfoIList)
}
}

some more context here: https://github.com/opalsecurity/terraform-provider-opal/pull/19/files#r1205825077

@andrewsy-opal andrewsy-opal temporarily deployed to Test August 10, 2023 21:00 — with GitHub Actions Inactive
@andrewsy-opal andrewsy-opal force-pushed the andrewsy/opal-7906-p1-sophos-aws-organizations-terraform-fails-for-opal-aws-iam branch from 48f9ed7 to 86b9fe5 Compare August 11, 2023 16:43
@andrewsy-opal andrewsy-opal temporarily deployed to Test August 11, 2023 16:43 — with GitHub Actions Inactive
opal/resource.go Outdated Show resolved Hide resolved
@andrewsy-opal andrewsy-opal temporarily deployed to Test August 11, 2023 17:56 — with GitHub Actions Inactive
@andrewsy-opal andrewsy-opal temporarily deployed to Test August 11, 2023 17:57 — with GitHub Actions Inactive
Copy link
Contributor

@ken-opal ken-opal left a comment

Choose a reason for hiding this comment

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

LGTM with one comment on validating the import + plan workflow

opal/resource.go Outdated Show resolved Hide resolved
opal/resource.go Outdated Show resolved Hide resolved
opal/resource.go Outdated Show resolved Hide resolved
opal/resource.go Outdated Show resolved Hide resolved
opal/verify.go Show resolved Hide resolved
@andrewsy-opal andrewsy-opal force-pushed the andrewsy/opal-7906-p1-sophos-aws-organizations-terraform-fails-for-opal-aws-iam branch from 3ee8f54 to 74643c6 Compare August 11, 2023 21:24
@andrewsy-opal andrewsy-opal temporarily deployed to Test August 11, 2023 21:24 — with GitHub Actions Inactive
@andrewsy-opal andrewsy-opal force-pushed the andrewsy/opal-7906-p1-sophos-aws-organizations-terraform-fails-for-opal-aws-iam branch from 74643c6 to 0f8eb10 Compare August 11, 2023 21:31
@andrewsy-opal andrewsy-opal temporarily deployed to Test August 11, 2023 21:31 — with GitHub Actions Inactive
@andrewsy-opal andrewsy-opal merged commit 4c51ad9 into main Aug 11, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants