Ability to define roles with inline policy in the iam.Role resource

[turkenf]

Description of your changes

In this PR:

• Removes the inline_policy parameter from config.MoveToStatus to be able to define inline policy roles.
• Adds an example with the inline_policy parameter.
• Removes the managed_policy_arns parameter from config.MoveToStatus

Fixes: #170

I have:

• Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

The example containing the inline_policy parameter and the existing example tested manually:

NAME                                              READY   SYNCED   EXTERNAL-NAME             AGE
role.iam.aws.upbound.io/role-with-inline-policy   True    True     role-with-inline-policy   46m
role.iam.aws.upbound.io/sample-role               True    True     sample-role               69m

NAME                                                               READY   SYNCED   EXTERNAL-NAME                            AG
E
rolepolicyattachment.iam.aws.upbound.io/sample-policy-attachment   True    True     sample-role-20230620124625013000000001   69
m

NAME                                           READY   SYNCED   EXTERNAL-NAME        AGE
policy.iam.aws.upbound.io/sample-user-policy   True    True     sample-user-policy   69m

Uptest:

• examples/iam/role.yaml => https://github.com/upbound/provider-aws/actions/runs/6001337855
• examples/iam/role-with-inline-policy.yaml => https://github.com/upbound/provider-aws/actions/runs/6001476120

[turkenf]

/test-examples="examples/iam/role.yaml"

[turkenf]

/test-examples="examples/iam/role-with-inline-policy.yaml"

[mcanevet]

FTR, we currently have a lot of api error Throttling: Rate exceeded issues. I think this feature would help because instead of creating and refreshing 3 resources for each role, it would only have 1, hence 3 times less API calls I guess.

[ivanzolotuhin]

Have not tested, but looks like exact functionality which available in deprecated community aws terrajet provider, which I am actively using to provision IAM roles with inline policy and which was missing (before this PR) and which prevented me to migrate to this provider

[sergenyalcin]

It seems that, we moved the inline_policy field to the status because this is mutually exclusive one. This means that it is already represented as a different managed resource: aws_iam_role_policy

The basic context of this approach is based on the High Fidelity principle of the Crossplane Resource Model.
Please see https://github.com/crossplane/crossplane/blob/master/design/one-pager-managed-resource-api-design.md#high-fidelity

So, if a field can be managed as a separate MR, it is a basic principle not to include it in the resource’s schema. There is an exception to this principle. For example, resource A in provider X has field b, and provider X also has another resource B representing field b. However, resource B cannot do all the work of field b that is inlined. In this case, the High Fidelity principle is sacrificed to expand the feature set.

In the context of this issue, if that’s the case, I wouldn’t worry about moving the relevant field to Spec. However, as far as I understand, the RolePolicy resource, represented separately, can already do the work of the inlined field. So I’m not sure if the case for this resource is included in the above exception.

In short, moving this field to Spec seems that against the Crossplane Resource Model’s High Fidelity principle. So I think we must not move this field to Spec.

[mcanevet]

@sergenyalcin as far as I know the RolePolicy resource, represented separately, can already do the work of the inlined field is not true. You an declare an attachment to a customer managed policy but not an inline policy.
The current limitation enforces you to create 3 resources to give permission to an IAM Role: the Role, the Policy and the Role Policy attachment. Having the possibility to declare an inline policy to the role would allow us to declare only 1 resource (an being rate limited later).

[sergenyalcin]

Thanks for your comment @mcanevet. Let me clarify my comment to explain myself better. Is the following equivalence right (in context of goal to be achieved/work to be done):

Role + RolePolicy + RolePolicyAttachment (3 resources) = Role (inline_policy)

If this is right, according to the High Fidelity principle of Crossplane, inlining this field is not a correct behavior in the context of API design. And we cannot involve them to the exceptation that explained above.

So, what I mean is, can the inline_policy field do more than any work we do using these three resources, or is the improvement only in reducing resource usage and in an easier use? If the field to be inlined is more capable and it has a concrete case that can be showed, I would be happy to approve this change. Because, in this type of case, this situation will be able to included to the above expectation.

[mcanevet]

The thing is that Role + RolePolicy + RolePolicyAttachment (3 resources) != Role (inline_policy). So for me this PR does not infringe the High Fidelity principle of Crossplane. Attaching a AWS Managed or Customer Managed policy to a role is different than declaring an inline policy to a role from AWS API point of view. There is definitely something we can't due with the current status of the provider.

[mcanevet]

@sergenyalcin aren't you convinced yet? I (or someone else) may try to explain it differently.

[rwsweeney]

@sergenyalcin:

Role + RolePolicy + RolePolicyAttachment (3 resources) != Role (inline_policy) because inline policies don't have AWS service quotas attached to them in the same way policies and policyAttachments do. AWS has a limit of 20 policy attachments per role, and 1,500 policies per account. The service quotas can be lifted to a point, but this isn't an issue with inline policies, and crossplane introduces a new limitation that doesn't exist in the underlying provider it's using. At a certain point functionality breaks down due to these limits, since there isn't parity between the underlying provider and the crossplane provider.

[JacobWeyer]

@sergenyalcin Which part of the high fidelity notes take precedence?

Crossplane managed resources should expose everything that provider exposes to its users as much as possible. A few benefits of that high fidelity:

Inline policies in AWS are explicitly treated as part of the Role itself and are a different paradigm than RolePolicy + Attachment. The inline policies do not prevent RolePolicy + RolePolicyAttachments to occur additionally, nor do they prevent the role from being changed.

Inline policies are actually somewhat prevalent and common to implement as part of terraform over role policy + attachment because they can cause issues with detach/reattachment with dynamic inputs and can cause significant reconciliation loops at the provider level on their own. Ex. deleting a role, policy and attachment can actually cause problems if the role tries to delete first. In prior versions of terraform (not sure of present) changes would fail and leave a messed up state behind.

In short, it's similar but still separate.

[jeanduplessis]

Thanks everyone for your patience here, we'll revert this week with an update on the direction we believe we should go in.

[jeanduplessis]

After further internal discussion, we will add the inline_policy and managed_policy_arns to the Role resource.

For further context, I will add that while the arguments made by community members here are not invalid (e.g., resource quotas, etc.), we feel the approach goes against the Crossplane Resource Model and, as such, should lead to a broader consideration upstream in Crossplane. We still intend to have this discussion, but we won't block proceeding with this PR.

That said, what really made us change our mind is an alternative use case that has come to the fore, which allowed us to understand that the use case is not limited to reducing resources. For example, the use of the inline policy field enables the deletion of all the associated inline policies of the role with an empty block, as described here: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role (and the same holds true for the management_policy_arns).

Other technical considerations were making us apprehensive of this change, for example, race conditions with resource reconciliation, however, we will address this in a future update.

[turkenf]

/test-examples="examples/iam/role.yaml"

[turkenf]

/test-examples="examples/iam/role-with-inline-policy.yaml"

[turkenf]

/test-examples="examples/iam/role.yaml"

[sergenyalcin]

/test-examples="examples/iam/role.yaml"

[sergenyalcin]

Thanks @turkenf LGTM!