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

chore: Add dynamodb restriction #251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

carlyjiang
Copy link
Contributor

@carlyjiang carlyjiang commented Feb 11, 2025

close #

Could you share the solution in high level?

  • Reject new dynamoDB policy which is against iks-compute account. (existing inline policy doesn't have access to the same IKS account dynamodb)
  • For backward compatible, if there's existing policy having access to the same IKS account dynamodb, the new policy still can have access to the same account dynamodb policy.
  • For all dynamodb policies, which targets to other accounts should be respected.
  • Update gomock to uber.org/mock, add mocks to git repo

Could you share the test results?

  • If existing policy has access to the same account dynamodb table, new policy allows to access the same account dynamodb table.
  • If existing policy has access to the other account dynamodb table, new policy doesn't allow to access the same account dynamodb table.
  • if existing policy doesn't have access to the dynamodb, new policy doesn't allow to access the same account dynamodb table.
  • if existing policy doesn't have access to dynamodb, new policy doesn't have dynamodb access, will pass validation.
  • Add all unit tests:
    • TestValidateAllowedDynamoDBAccessExistingPolicyNoAccessAndNewRequestHasAccess
    • TestValidateAllowedDynamoDBAccessExistingPolicyOtherAccountAndNewRequestSameAccount
    • TestValidateAllowedDynamoDBAccessExistingPolicyAndNewRequestHaveAccess
    • TestValidateAllowedDynamoDBAccessGetRolePolicyFailure
    • TestValidateAllowedDynamoDBAccessGetRoleFailure
    • TestValidateAllowedDynamoDBAccessNewPolicyHasAccess
    • TestValidateAllowedDynamoDBAccessExistingPolicyHasAccess
    • TestValidateAllowedDynamoDBAccessExistingPolicyNoAccessAndNewRequestHasAccessWithArrayFields
    • TestValidateAllowedDynamoDBAccessInvalidRoleArn

@carlyjiang carlyjiang requested a review from a team as a code owner February 11, 2025 05:42
@carlyjiang carlyjiang force-pushed the dynamodb-restriction branch 6 times, most recently from 63f99be to 1db9a1d Compare February 11, 2025 23:52
@carlyjiang carlyjiang self-assigned this Feb 11, 2025
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 86.79245% with 14 lines in your changes missing coverage. Please review.

Project coverage is 50.70%. Comparing base (43d3f06) to head (f5114dd).

Files with missing lines Patch % Lines
pkg/awsapi/iam.go 86.79% 11 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
- Coverage   58.52%   50.70%   -7.82%     
==========================================
  Files          13       17       +4     
  Lines        1408     2191     +783     
==========================================
+ Hits          824     1111     +287     
- Misses        548     1040     +492     
- Partials       36       40       +4     

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

@carlyjiang carlyjiang force-pushed the dynamodb-restriction branch 3 times, most recently from 121a547 to aa1b790 Compare February 12, 2025 00:28
Makefile Outdated
@@ -38,11 +38,9 @@ endif
all: manager

mock:
go install github.com/golang/mock/mockgen@v1.6.0
go install go.uber.org/mock/mockgen@v0.4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated the lib to v0.5.0, and bump go version to 1.22.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants