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

add optional policy allowing push access #98

Merged
merged 10 commits into from
May 18, 2023

Conversation

kpankonen
Copy link
Contributor

@kpankonen kpankonen commented Jun 2, 2022

what

  • adds the ability to give push-only access to the repository

why

  • full access was more than we wanted in our situation (CI pushing images to the repo) so we added a principals_push_access to give push-only access.

references

  • policy is based on this AWS doc

@kpankonen kpankonen requested review from a team as code owners June 2, 2022 18:56
@kpankonen kpankonen requested review from r351574nc3 and srhopkins and removed request for a team June 2, 2022 18:56
@ngoyal16
Copy link

#96

main.tf Outdated Show resolved Hide resolved
@nitrocode
Copy link
Member

/test all

@nitrocode
Copy link
Member

Thanks for the contribution. I'm just trying to get a second set of eyes on this to make sure it's what we want in this module.

main.tf Outdated Show resolved Hide resolved
@ngoyal16
Copy link

Use distinct function for override policy document

@ngoyal16
Copy link

Either we should move lambda condition to independent or add in push policy as welll

I prefer it should be independent

@kpankonen
Copy link
Contributor Author

Use distinct function for override policy document

I'm not sure I follow. Where do you think there should be a distinct function?

@ngoyal16
Copy link

Use distinct function for override policy document

I'm not sure I follow. Where do you think there should be a distinct function?

Main.tf line no 250

@kpankonen
Copy link
Contributor Author

Use distinct function for override policy document

I'm not sure I follow. Where do you think there should be a distinct function?

Main.tf line no 250

Maybe I'm missing something but I think that would require restructuring the module so that the all (push, readonly, full)actions are combined/distinct before they get used in a policy document. That would also mean losing the sid on the different policy blocks. That would make it harder to understand the policy when looking at it from the AWS side.

Or I'm looking at the wrong thing...

@nitrocode nitrocode requested review from aknysh and Gowiem June 17, 2022 13:47
main.tf Outdated
override_policy_documents = local.principals_full_access_non_empty ? [data.aws_iam_policy_document.resource_full_access[0].json] : [data.aws_iam_policy_document.empty[0].json]
count = module.this.enabled ? 1 : 0
source_policy_documents = local.principals_readonly_access_non_empty ? [data.aws_iam_policy_document.resource_readonly_access[0].json] : [data.aws_iam_policy_document.empty[0].json]
override_policy_documents = [

Choose a reason for hiding this comment

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

Suggested change
override_policy_documents = [
override_policy_documents = distinct([])

Copy link
Member

Choose a reason for hiding this comment

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

You mean to wrap distinct([ around lines 253 and 254 ?

Choose a reason for hiding this comment

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

Yes

Choose a reason for hiding this comment

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

Also adding lambda principal policy as whole would be great so if someone wants a repo with lambda ran can be done as well without others statements policy

@kpankonen
Copy link
Contributor Author

i see the bridgecrew code analysis is failing but i'm not able to see the output of the job

@nitrocode
Copy link
Member

/test all

main.tf Show resolved Hide resolved
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@kpankonen please see comments

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

please see comments

@kpankonen kpankonen requested a review from aknysh August 17, 2022 19:12
@ngoyal16
Copy link

ngoyal16 commented Aug 27, 2022

@kpankonen currently lambda access is granted using dynamic statements in each group and it may be problematic if someone want to create and ECR with just lambda access as resource policy. then policy might be missing because we are checking if the system has push or pull or full variables then only create not based on lambda too. We should have independent staement for lambda access policy.

@kpankonen
Copy link
Contributor Author

@aknysh @nitrocode is there anything else i can do to help get this merged?

@aknysh
Copy link
Member

aknysh commented Nov 14, 2022

/test all

@aknysh
Copy link
Member

aknysh commented Nov 14, 2022

/rebuild-readme

@aknysh
Copy link
Member

aknysh commented Nov 14, 2022

/test all

@kpankonen
Copy link
Contributor Author

@kpankonen Apologies, but can you fix the out of date README? Do the following:

make init
make github/init
make readme

and commit the changes (if any)

done!

@joe-niland
Copy link
Member

/test all

@kpankonen
Copy link
Contributor Author

anything else needed?

@Gowiem
Copy link
Member

Gowiem commented May 18, 2023

Argh @kpankonen -- we're doing you wrong here with multiple of us trying to move it forward, but then dropping the ball (we get drowned in a sea of GH notification emails as part of being on the maintainer team, so that is our excuse 😅 ).

Can you please address the conflicts or try rebasing again? Once this is ready to go, ping me here or better yet in the SweetOps slack and I will be sure to get this over the line for you. Thanks for the work and patience!

@kpankonen kpankonen force-pushed the principals_push_access branch from 291e7c4 to 7dc3b63 Compare May 18, 2023 22:40
@kpankonen kpankonen requested a review from a team as a code owner May 18, 2023 22:40
@Gowiem
Copy link
Member

Gowiem commented May 18, 2023

/terratest

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@kpankonen kpankonen force-pushed the principals_push_access branch from c5a82a6 to e20bdcd Compare May 18, 2023 23:01
@kpankonen
Copy link
Contributor Author

@Gowiem updated

@Gowiem
Copy link
Member

Gowiem commented May 18, 2023

/terratest

Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

:shipit:

@Gowiem Gowiem added the minor New features that do not break anything label May 18, 2023
@Gowiem
Copy link
Member

Gowiem commented May 18, 2023

@aknysh I believe we need your approval to move this forward.

FYI, I am going to do a fast follow on this PR today or tomorrow to break out the principals_lambda policies into their own policy doc so they don't need to be included with each of these full vs read vs write docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor New features that do not break anything
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants