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

[iam] iam policies should allow passing policydocuments via props to constructor like managed policies do #11236

Closed
1 of 2 tasks
markussiebert opened this issue Oct 31, 2020 · 9 comments · Fixed by #11430
Closed
1 of 2 tasks
Assignees
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on. p2

Comments

@markussiebert
Copy link
Contributor

Use Case

While using eks a lot i often find policy documents for certain tools maintained by the creator of the tool (as an example, aws ingress controller).
I would like to use these policy documents, and don't rewrite them. There is the function, that allows creating policy documents from json - that's fine. Then it is possible to pass the document to the managedpolicy constructor - but not to inline policys.

I see no reason why this shouldn't be allowed for policies to.

Proposed Solution

extend policy props like it's done in managed policy props and extend constructor of policy like it's done in managed policy.

readonly document?: PolicyDocument;

both lines are not included in the normal policy.

Other

If you agree, i will start to implement this and as far as i can see, there should be no negative side effects.

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@markussiebert markussiebert added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Oct 31, 2020
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Oct 31, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 2, 2020

What would the user code look like with your proposed solution in place?

@rix0rrr rix0rrr added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 2, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 2, 2020

(In favor of this functionality in general, btw, but it depends on what the solution looks like)

@markussiebert
Copy link
Contributor Author

markussiebert commented Nov 2, 2020

ok, thats what i do at the moment:

 const albIngressIamPolicyDocument = iam.PolicyDocument.fromJson(
      require(`${__dirname}/addonAwsLoadbalancerControllerFiles/iam_policy.json`)
    );

        const albIngressPolicy = new iam.ManagedPolicy(scope, "AwsLoadbalancerControllerManagedPolicy", {
            document: albIngressIamPolicyDocument
        })

        const albIngressIamPolicyDocument = iam.PolicyDocument.fromJson(
      require(`${__dirname}/addonAwsLoadbalancerControllerFiles/iam_policy.json`)
    );

        albIngressPolicy.attachToRole(albIngressServiceAccount.role)

and i want to use Policy instead of ManagedPolicy - but it's not supported now.

p.s. the iam_policy.json comes from the AwsLoadbalancerController (included via submodules).

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Nov 2, 2020
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 3, 2020
@markussiebert
Copy link
Contributor Author

@rix0rrr what do you think? Can i start creating the PR?

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 9, 2020

I was asking about what the could WOULD look like with your ideal API in place, not about your current workaround.

For example, we could pick either of the following:

const policy = Policy.fromJson(this, 'MyPolicy', '/path/to/bla.json');
albIngressServiceAccount.role.addInlinePolicy(policy);

// OR

const document = PolicyDocument.fromJson('/path/to/bla.json');
const policy = new Policy(this, 'MyPolicy', {
  document: document,
});
albIngressServiceAccount.role.addInlinePolicy(policy);

I feel like my preference would be for the 2nd one.

@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2 labels Nov 9, 2020
@markussiebert
Copy link
Contributor Author

i also prefer the second one (as i wrote, it's like managedpolicy behaves - and i think it should be implemented the same way).

@markussiebert
Copy link
Contributor Author

markussiebert commented Nov 11, 2020

I created a PR with the changes I suggest. I implemented it like your 2nd solution (and as it's implemented in ManagedPolicy).

And sorry, if i did things wrong, it's my first PR for CDK.

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Nov 13, 2020
@mergify mergify bot closed this as completed in #11430 Nov 16, 2020
mergify bot pushed a commit that referenced this issue Nov 16, 2020
allow passing PolicyDocuments to Policys like it could be done right now for ManagedPolicys
fixes #11236


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@sathishpothuri
Copy link

The require option works fine, but the lint fails.
I have the below working example.

managedPolicies: [
          new ManagedPolicy(this, 'myCustomPolicy', {
            document: PolicyDocument.fromJson(JSON.parse(fs.readFileSync(`${__dirname}/../policy/mypolicy.json`, 'utf-8'))),
          }),
        ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants