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

[doc] Add docs of required IAM actions for Piped #4925

Merged
merged 3 commits into from
May 28, 2024
Merged

Conversation

t-kikuc
Copy link
Member

@t-kikuc t-kikuc commented May 27, 2024

What this PR does / why we need it:

as title.

Currently, it's difficult to specify which actions are necessary for deployments.

Which issue(s) this PR fixes:

Fixes #4793

Does this PR introduce a user-facing change?: N/A

  • How are users affected by this change: N/A
  • Is this breaking change:N/A
  • How to migrate (if breaking change):N/A

How I listed

by the following commands.
List API calls -> Extract API names -> Remove duplicated

## Lambda
cat pkg/app/piped/platformprovider/lambda/client.go| grep '= c.client.' |  awk -F'.' '{print $3}' | awk -F'(' '{print $1}' | sort | uniq
## ECS>ECS
cat pkg/app/piped/platformprovider/ecs/client.go| grep '= c.ecsClient.' |  awk -F'.' '{print $3}' | awk -F'(' '{print $1}' | sort | uniq
## ECS>ELB
cat pkg/app/piped/platformprovider/ecs/client.go| grep '= c.elbClient.' |  awk -F'.' '{print $3}' | awk -F'(' '{print $1}' | sort | uniq

t-kikuc added 2 commits May 27, 2024 09:39
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

@t-kikuc Thank you for the improvement. I left some comments.

Comment on lines 15 to 31
- `ecs:CreateService`
- `ecs:CreateTaskSet`
- `ecs:DeleteTaskSet`
- `ecs:DeregisterTaskDefinition`
- `ecs:DescribeServices`
- `ecs:DescribeTaskSets`
- `ecs:RegisterTaskDefinition`
- `ecs:RunTask`
- `ecs:TagResource`
- `ecs:UpdateService`
- `ecs:UpdateServicePrimaryTaskSet`

- `elasticloadbalancing:DescribeListeners`
- `elasticloadbalancing:DescribeRules`
- `elasticloadbalancing:DescribeTargetGroups`
- `elasticloadbalancing:ModifyListener`
- `elasticloadbalancing:ModifyRule`
Copy link
Member

Choose a reason for hiding this comment

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

[ask] I am not familiar with AWS IAM, so I have a question. Is the IAM in AWS based on the API call? I'm concerned about whether other required permission exists. 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I forgot to list permissions that are required along with ECS/ELB/Lambda APIs.
I'll add them

Copy link
Member Author

Choose a reason for hiding this comment

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

@ffjlabo
I fixed and added the following action.

         {
            "Effect": "Allow",
            "Action": [
                "iam:PassRole"
            ],
            "Resource": [
                "arn:aws:iam::<account-id>:role/<task-execution-role>",
                "arn:aws:iam::<account-id>:role/<task-role>"
            ]
        }

@@ -0,0 +1,47 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

[imo] How about moving to the installation section? It might be nice for users to know the requirements when installing.
https://pipecd.dev/docs-v0.47.x/installation/install-piped/

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds great, thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

i fixed

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
@t-kikuc t-kikuc requested a review from ffjlabo May 28, 2024 05:06
Copy link

codecov bot commented May 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.30%. Comparing base (2c067bc) to head (1cac68f).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4925      +/-   ##
==========================================
- Coverage   29.32%   29.30%   -0.02%     
==========================================
  Files         321      321              
  Lines       40835    40835              
==========================================
- Hits        11974    11968       -6     
- Misses      27904    27910       +6     
  Partials      957      957              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks 👍

Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

🚀

@t-kikuc t-kikuc merged commit 89fba60 into master May 28, 2024
13 of 14 checks passed
@t-kikuc t-kikuc deleted the doc-iam-policy branch May 28, 2024 23:10
@github-actions github-actions bot mentioned this pull request Jun 12, 2024
@github-actions github-actions bot mentioned this pull request Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[doc] Add docs of referential AWS IAM policies with least privilege for Piped
3 participants