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

IT-3988: Add role to invoke model and move inline sso policy to managed policy #1277

Merged
merged 8 commits into from
Nov 13, 2024

Conversation

xschildw
Copy link
Contributor

@xschildw xschildw commented Nov 7, 2024

This PR creates a role that gives Bedrock permission to execute model

@xschildw xschildw requested a review from a team as a code owner November 7, 2024 01:03
@xschildw xschildw requested a review from zaro0508 November 7, 2024 01:04
@@ -641,18 +641,6 @@ SsoLlmDeveloper:
managedPolicies:
- 'arn:aws:iam::aws:policy/AmazonBedrockFullAccess'
- 'arn:aws:iam::aws:policy/AWSCloudFormationFullAccess'
# https://stackoverflow.com/questions/58125181/cloud-formation-cant-upload-template-file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving to a managed policy with the hope we can tweak it manually until we get what we need. Inline, we get an error because it's under AWS control.

@@ -388,3 +388,31 @@ SynapseAthenaUserAccessPolicy:
]
}
PolicyName: SynapseAthenaUserAccessPolicy

SynapseLlmBedrockAgentPolicy:
Copy link
Contributor Author

@xschildw xschildw Nov 7, 2024

Choose a reason for hiding this comment

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

@zaro0508 , wondering if I should have the policy here or if I should move it as a resource alongside the bedrock agent role below.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zaro0508 , wondering if I should have the policy here or if I should move it as a resource alongside the bedrock agent role below.

You'll notice that existing policy definitions created in here are meant to be applied to all accounts (Account: '*') because some AWS features require us to apply the same policy. If you are intending to apply this to one account then I would say that it's more appropriate to setup this policy with the resource that needs it.

@xschildw xschildw changed the title IT-3988: Add role to invoke model IT-3988: Add role to invoke model and move inline sso policy to managed policy Nov 7, 2024
Copy link
Contributor

@zaro0508 zaro0508 left a comment

Choose a reason for hiding this comment

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

t

@xschildw xschildw requested a review from zaro0508 November 11, 2024 19:12
Copy link
Contributor

@zaro0508 zaro0508 left a comment

Choose a reason for hiding this comment

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

From reading the issue it looks like the goal is to update the sso llm developer role with a few missing permissions however I don't understand your implementation to accomplish that goal. I might be missing something but I don't see an explanation as to why you need to make all these changes to accomplish your goal. Why not just keep the format as-is and update the inline policy in org-formation/700-aws-sso/_tasks.yaml?

@xschildw
Copy link
Contributor Author

We first have to determine which permissions we need, ideally out of the PR cycle (i.e. only have one PR with everything we need). With an inline policy, we are not able to add permissions manually in the console (the policy in under AWS control). So we're trying to setup a managed policy, and in next PR we'll reference it in the role.

@zaro0508
Copy link
Contributor

With an inline policy, we are not able to add permissions manually in the console (the policy in under AWS control). So we're trying to setup a managed policy, and in next PR we'll reference it in the role.

ahh, i didn't realize that there is no way to update an inline policy in AWS console. For testing purposes, I typically use the AWS console to manually create a new inline policy with additional access and apply that to a role. That lets me craft a least privilege policy before committing it to cloudformation.

Anyways, now that I understand I don't have a problem with your approach either it just makes it more difficult to follow in this org-formation/cloudformation setup.

}
PolicyName: SynapseLlmDeveloperPolicy

BedrockAgentRole:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this role necessary when the goal is to only fix the developer role?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the policy for the user (sso-llmdeveloper) to be able to work in the console/cloudformation to create an agent.

org-formation/700-aws-sso/_tasks.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,35 @@
AWSTemplateFormatVersion: '2010-09-09'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the role used by the agent when it's running. It has the right to execute a model.

@xschildw xschildw requested a review from zaro0508 November 11, 2024 23:32
Copy link
Contributor

@zaro0508 zaro0508 left a comment

Choose a reason for hiding this comment

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

just a small fix up and should be good to go.

Description: The ARN of the Bedrock Agent Role
Value: !GetAtt bedrockAgentRole.Arn
Export:
Name: BedrockAgentRoleArn
Copy link
Contributor

Choose a reason for hiding this comment

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

outputs need to be unique therefore it would be better if you add stack name here, i.e. !Sub '${AWS::StackName}-BedrockAgentRoleArn'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@xschildw xschildw requested a review from zaro0508 November 13, 2024 01:28
@xschildw xschildw merged commit 5554047 into Sage-Bionetworks-IT:master Nov 13, 2024
3 checks passed
zaro0508 pushed a commit that referenced this pull request Nov 13, 2024
This PR fixes a deployment issue in #1277 caused by a missing '!Ref'.
@xschildw xschildw deleted the m-it-3988-role branch November 13, 2024 16:28
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.

3 participants