-
Notifications
You must be signed in to change notification settings - Fork 368
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): Add RolePolicy resource for inline policies #938
feat(iam): Add RolePolicy resource for inline policies #938
Conversation
5b5a8b3
to
7f273d6
Compare
Did you also do manual tests where you:
|
@smcavallo can you rebase and change to IAM Group ? |
Hi @haarchri, we're in the same boat as @smcavallo where we'd like to be able to define inline policies for IAM Roles, so may try to find some time to contribute if this doesn't progress. Would you mind explaining why this should be changed to IAM Group please, as that is a different resource? |
No we changed the APIGroup for all IAM resources and removed the Präfix IAM for kinds |
e46deda
to
2787f61
Compare
@haarchri - this has been refactored and updated and retested. it is ready for tests to be kicked off |
Why there is a need for a new kind of Can we have something like below?
|
The reason I used This implementation attempts to closely follow the below APIs for inline role policies: A The output of |
Valid points, thanks for the clarification. |
@smcavallo do you plan to finish this anytime soon? I'd really like to see this feature merged. |
Crossplane does not currently have enough maintainers to address every issue and pull request. This pull request has been automatically marked as |
The implementation in this PR is aligned with the implementation in the universal provider. |
55822e4
to
567d004
Compare
apis/iam/v1beta1/rolepolicy_types.go
Outdated
type RolePolicyParameters struct { | ||
|
||
// The JSON policy document that is the content for the policy. | ||
Document string `json:"document"` |
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.
Misformatted raw JSON policies are a frequent cause for errors. Could this be change to a ResourcePolicy
as using in https://github.com/crossplane-contrib/provider-aws/blob/master/apis/kafka/v1alpha1/custom_types.go#L37?
That would give users a structured schema and avoids reconciler errors due to a missing comma. It is also much easier to handle in compositions.
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.
@MisterMX - fair point on the compositions
I just took a look at moving from string
to ResourcePolicy
and tested out that config.
We actually prefer strings instead. I would guess since most users of this are using crossplane provider-aws IAM Policy in string format that moving from Policy + RolePolicyAttachment to this new RolePolicy would prefer as well and make this an easier transition.
Our use case is we deploy a LOT of IAM roles for 3rd party applications - probably more IAM for 3rd party applications than for our own webservices. Those 3rd parties normally provide their IAM policies in json format. The challenge with using resource policy is that we have to then convert their policy from a string which is natively supported by crossplane into the schema of the ResourcePolicy
. We then have to diff and maintain whenever those 3rd party roles change, convert them to the new schema. There is more overhead required in doing that vs just dropping in a new json file.
For reference see -> https://github.com/sergkondr/karpenter/blob/main/website/content/en/preview/getting-started/getting-started-with-karpenter/cloudformation.yaml#L41-L221
It would be a pain to convert that to ResourcePolicy
and then have to maintain that
What we do instead is we just drop a json file in a helm files/
directory and then convert it to json and then back to a string which allows us to maintain policies in the native 3rd party provided json format, and then use helm tooling to both customize and also verify that it is valid json. This has caught many policy formatting issues.
Example:
---
apiVersion: iam.aws.crossplane.io/v1beta1
kind: Policy
metadata:
name: aws-karpenter-{{ .Values.cluster }}
spec:
forProvider:
name: aws-karpenter-{{ .Values.cluster }}
document: |
{{- .Files.Get "files/aws-karpenter.json" |
replace `${AWS_PARTITION}` .Values.aws.partition |
replace `${AWS_REGION}` .Values.aws.region |
replace `${CLUSTER_NAME}` .Values.cluster |
replace `${AWS_ACCOUNT_ID}` .Values.aws.account_id |
fromJson | mustToPrettyJson |
nindent 6 }}
description: aws-karpenter policy created by crossplane
tags:
- key: Environment
value: {{ .Values.environment }}
providerConfigRef:
name: {{ .Values.providerConfigRef }}
You bring up a good point about the compositions and there are people who may prefer ResourcePolicy
I attempted to see if I could convert a json policy to the schema required by ResourcePolicy - I am still working on this - it's a challenge because the schema of ResourcePolicy
differs so much from aws policy.
What are your thoughts with starting out just supporting string? Or supporting both string
and ResourcePolicy
in the future? Or maybe there are other solutions which would make sense? One that I can think of which we might actually like is being able to Ref
to the policy - and that Ref could be a configmap with policy json or a custom object which contains a ResourcePolicy
object - just trying to think out of the box on something that could meet everyone's needs.
For now, for us, string is the easiest to support and maintain.
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 understand what you mean. Indeed, ResourcePolicy
is a bit tricky to handle.
However, having a raw JSON strings can also be difficult since its schema is only validated at runtime. It is very easy to have misformatted JSON that causes reconciling to fail.
Do you think JSON
could be an alternative? It is used already in other places as well as https://github.com/crossplane/crossplane.
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.
@MisterMX when I change the type to
// The JSON policy document that is the content for the policy.
Document v1.JSON `json:"document"`
the CRD which is generated is broken
and results in
DEBUG events operation error IAM: PutRolePolicy, https response error StatusCode: 400, RequestID: XXXXXXX, MalformedPolicyDocument: Syntax errors in policy.
I tried using kubebuilder validation instead. Ex. // +kubebuilder:validation:Format:=json
But json is not in the type list that it can validate.
https://github.com/kubernetes/apiextensions-apiserver/blob/d323d00fb3d4c0c1532ac4fdbeec5aef190104ca/pkg/apiserver/validation/formats.go#L26C1-L51
I agree it would be best to be able to reject invalid policies as early in the process as possible and at the k8s API level. Validation Webhook would be ideal.
I wonder if we could use something like this -> crossplane/crossplane#3991
In the same way as validation webhooks can be used to validate compositions, I am wondering if we can use something similar for non-compositions?
There are many other instances where we want more validation than kubegen will give us.
In the least it would be good to fail in the controller instead of passing malformed policies to AWS.
For now - can we start with string and for enhancements:
- Possibly add validation in the controllers (before ANY policies are sent to AWS API)
- Add enhancement to also support
ResourcePolicy
- and prefer that overstring
if defined - Look at webhook validation or any other k8s validation which could be used
This PR is no worse than IAM Policies are handled elsewhere in provider-aws
It would be nice to get this finally merged, and it would be nice to have a solution which could be used across any aws policies included in this provider
@smcavallo looking good to me so far. Can you look at the last remark, fix the tests and squash your commits? Then we can merge this. |
c7e2b3f
to
06d2524
Compare
Signed-off-by: smcavallo <smcavallo@hotmail.com>
06d2524
to
25b5a90
Compare
@MisterMX - I have made the recommended changes, fixed the tests and rebased off master with a single commit. The tests are passing now. Thanks so much for all the help with this one! |
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.
LGTM. Thank you very much @smcavallo for this contribution!
Use Terraform Plugin SDK to Reconcile MRs
Signed-off-by: smcavallo smcavallo@hotmail.com
Description of your changes
Support Inline Policies as part of IAM Role.
provider-aws
currently requiresRole
+Policy
+RolePolicyAttachment
which is 3 objects/api calls - theRolePolicyAttachment
being kind of a strange one and also making order of operations be very important during deletions. Ex. can't delete a policy/role which has aRolePolicyAttachment
-> have to delete the attachment before the policy/role.For any policy which does not need to be shared across roles, this feature is less expensive (less API calls and kubernetes objects) and easier to maintain. It is also more secure as the policy cannot be shared with another role.
This mirrors https://awscli.amazonaws.com/v2/documentation/api/latest/reference/iam/get-role-policy.html
Fixes: #177
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
RolePolicy
RolePolicy
via kubectl and confirmed crossplane updated the inline policy via the consoleRolePolicy
via kubectl and confirmed crossplane deleted the inline policy via the console