-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(iam): Role.withoutPolicyUpdates()
#5569
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet. Small comments / suggestions on wording.
* | ||
* @default false | ||
*/ | ||
readonly mustCreate?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not crazy about this name... How about lazy?: boolean
?
result.push('Policy created with mustCreate=true is empty. You must add statements to the policy'); | ||
} | ||
if (!this.mustCreate && this.referenceTaken) { | ||
result.push('Policy name has been read of empty policy. You must add statements to the policy so it can exist.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can word this a little more clearly... how about: This Policy has been referenced by another resource, and so it must contain at least one statement
.
result.push(`Policy created with mustCreate=true must be attached to at least one principal: user, group or role`); | ||
} | ||
if (!this.mustCreate && this.referenceTaken) { | ||
result.push('Policy name has been read of unattached policy. Attach to at least one principal: user, group or role.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: This Policy has been referenced by another resource, and so it must be attached to at least one principal: user, group or role
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I fully understand the use case for this feature, and I couldn't dig those up from the referenced issues. #2985 is about immutable imported roles, #4465 is about maximum policy size limitations and I am not sure I fully understand how immutable roles help here.
Also, missing README, which might be a good place to provide a useful example that demonstrates when I'd want to use these immutable roles.
I also think that the name ImmutableRole
is a bit misleading, because I'd expect an immutable "thing" to fail if I try to mutate it, and this is basically ignoring mutations. But perhaps this ship has sailed since we already have immutable: true
when we import roles, so whateveeeee.
The maximum policy size is related to the immutable roles because immutability decreases the size of the statements added to the role when it's passed into constructs that add statements to it. An example from #4465: const myRole = new iam.Role(this, 'Role', { ... });
// use myRole in a bunch of places...
// then:
new codepipeline_actions.CodeBuildAction({
// ...
role: myRole,
}); Now, it might happen that passing new codepipeline_actions.CodeBuildAction({
// ...
role: new iam.ImmutableRole(myRole),
}); And this way, all statement-adding expressions in Hope the motivation for this is a little more clear now! |
"Changes requested" is not a reasonable way to say "I don't understand the motivation for this change"
README missing is "changes requested" and also I wanted to make sure the motivation for this, and the use case is clear before it's merged. To be honest, the motivation @skinny85 provided above doesn't sounds like a "proper" use case. Maybe it's some kind of an escape hatch to allow people to work around bugs or issues we have in the system, but then I'd argue that a whole new type of Let's discuss this before we introduce additional public APIs that don't have a clear use case. I think trying to write a README entry for this will be a good exercise as it will require you to articulate a reasonable situation where someone would want to use this feature. |
I think we can chunk this PR smaller. There are some useful changes in there which are separate from the |
ImmutableRole
classImmutableRole
class
ImmutableRole
classImmutableRole
wrapper class
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Added a README explaining why you might want to use this class. Pre-emptive FAQ: Can we not do anything else if the policy size is growing too big?We need to apply judicious use of wildcards then, and we decide not to do IAM policy manipulation because it's scary. Therefore, we have to punt to the user in some way. This is that way. We could make it slightly easier for the user to get an ImmutableRole, maybe instead of new codepipeline.Pipeline(this, 'Pipeline', {
role: new iam.ImmutableRole(role),
}); they could do: new codepipeline.Pipeline(this, 'Pipeline', {
role: role.immutableCopy(),
}); But that's about the extent of it, and that's not appreciably different imo. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thanks for adding the README and the FAQ, they are very helpful. I think this API is much better: new codepipeline.Pipeline(this, 'Pipeline', {
role: role.immutableCopy(),
}); As it doesn’t introduce a whole new type and is more discoverable. You just return an IRole this way!. It can also be applied both to owned and imported roles, so there won’t be a difference (maybe even deprecate “mutable: false”). I am still not sure about the terminology of immutable. Perhaps something more around “role.withoutPolicyUpdates()” or something around “disable automatic policy” which is more in line with the README narrative. |
Mmyeah. I agree that a I don't buy adding it to the
const role = Role.fromRoleArn(....).withoutPolicyUpdates(); Which is a little silly. My proposal is:
|
@rix0rrr wrote:
Sounds good.
Yeah I agree that this makes more sense to be honest. And then I would deprecate the name |
Are you sure? This ( |
I personally think |
This is where we introduced the We added support for adding policies to imported roles here: #2805. This is where I think we went wrong though. Motivating issues for that: So it seems like the correct choice to me. You're right the change is not backwards compatible but I think we'll do everyone a favor in the long run if we rip off the band-aid now. The flag is |
ImmutableRole
wrapper classRole.withoutPolicyUpdates()
allows manual policy management
Role.withoutPolicyUpdates()
allows manual policy managementRole.withoutPolicyUpdates()
prevents automatic policies
Role.withoutPolicyUpdates()
prevents automatic policiesRole.withoutPolicyUpdates()
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
I actually don't agree that this was the error in the general case, as #2651 was a way to workaround an actual bug in CodeBuild (missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update PR description
@@ -301,4 +301,34 @@ export = { | |||
|
|||
test.done(); | |||
}, | |||
|
|||
'can use an ImmutableRole for a Project within a VPC'(test: Test) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'can use an ImmutableRole for a Project within a VPC'(test: Test) { | |
'can use role.withoutPolicyUpdates() for a Project within a VPC'(test: Test) { |
? new MutableImport(scope, id) | ||
: new ImmutableImport(scope, id); | ||
? new Import(scope, id) | ||
: new ImmutableRole(new Import(scope, id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be new Import().withoutPolicyUpdates()
?
* If you do, you are responsible for adding the correct statements to the | ||
* Role's policies yourself. | ||
*/ | ||
public withoutPolicyUpdates(): IRole { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going back to our conversation, why can't this be in IRole
and works both for Role
and imported roles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
The interface is already starting to get onerous to implement and I don't want to make it worse. There are 2 different parts to it: "what makes an
IRole
a role", and "operations that can be done to anyIRole
, which are the same for every role anyway". This is part of the 2nd, but I'd argue those shouldn't actually be onIRole
at all. -
Even if we put it on
IRole
, my gut says the 99% case will be immutable, hence following up the import by calling this method. We want to optimize for the common case, and adding more verbiage seems the opposite. Either we (ALSO?) add awithPolicyUpdates()
to switch it back on, or we just leave it as import properties for thefromRoleArn()
.
So either we make IRole
even heavier or it doesn't need to be there at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. It’s not that critical.
It looks like this PR actually broke |
Yes please! |
Created issue: #5943 |
In the refactoring done in #5569, we introduced a bug. The `ImmutableRole` class correctly ignored policies directly added to it, but did not ignore policies added via `Grant.addToPrincipal()`. That's because its `IGrantable#grantPrincipal` field was being used as the principal to grant to, which was pointing to the wrapped role instead of the `ImmutableRole` itself. Fix this oversight and add a test to cement it in. Fixes #5943.
In the refactoring done in #5569, we introduced a bug. The `ImmutableRole` class correctly ignored policies directly added to it, but did not ignore policies added via `Grant.addToPrincipal()`. That's because its `IGrantable#grantPrincipal` field was being used as the principal to grant to, which was pointing to the wrapped role instead of the `ImmutableRole` itself. Fix this oversight and add a test to cement it in. Fixes #5943. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Add a new method to
Role
calledwithoutPolicyUpdates()
, which returns a wrapper object that will drop all policy updates. This can be used in situations where you want to skip the default IAM permission adding behavior of CDK.Needs only be used with roles that are created in the same CDK application; for imported roles, supplying
mutable=false
when callingRole.fromRoleArn()
is sufficient.Fixes #2985.
Fixes #4465.
Closes #4501.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Commit Message
feat(iam):
Role.withoutPolicyUpdates()
Add a new method to
Role
calledwithoutPolicyUpdates()
, which returns a wrapper object that will drop all policy updates. This can be used in situations where you want to skip the default IAM permission adding behavior of CDK.Needs only be used with roles that are created in the same CDK application; for imported roles, supplying
mutable=false
when callingRole.fromRoleArn()
is sufficient.Fixes #2985.
Fixes #4465.
Closes #4501.