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

feat(iam): Add observe and update of user permissions boundary #1735

Merged

Conversation

zonybob
Copy link
Contributor

@zonybob zonybob commented Apr 20, 2023

Description of your changes

This update causes the IAM User controller to also observe needed changes on the User permissions boundary and update it as needed. Part of this change also introduces the hopefully desirable side effect that UpdateUser is only called if needed (when the path changes.)

Fixes #1080

I have:

  • [ X ] Read and followed Crossplane's contribution process.
  • [ X ] Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

This code has been tested against a factory environment where a number of IAM users are present and managed. Users were created, and boundaries were updated on the managed resource to observe the correct behavior in AWS.

IAM unit tests were also updated and run.

@zonybob
Copy link
Contributor Author

zonybob commented Apr 21, 2023

I am working on updating this to support IAM Role as well (to fully close #1080 ). I'd appreciate any feedback/testing from the community as I do not use Roles in my setup. I will update unit tests and then test as much as I can.

pkg/clients/iam/user.go Outdated Show resolved Hide resolved
@zonybob zonybob marked this pull request as draft April 22, 2023 12:50
@zonybob zonybob marked this pull request as ready for review April 23, 2023 13:55
Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @zonybob.

This looks pretty close to merge for me. Can you fix the issues I remarked?

pkg/controller/iam/user/controller.go Outdated Show resolved Hide resolved
pkg/controller/iam/user/controller.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
zonybob and others added 2 commits July 7, 2023 14:44
@zonybob
Copy link
Contributor Author

zonybob commented Jul 9, 2023

@MisterMX thank you for the feedback!
I addressed error wrapping and UP_VERSION
I struggle a little bit with placement on IsUpToDate. I tried to follow typical pattern of placing in clients/iam, but I fight an import cycle with existing DiffIAMTags. Tried to stay out of refactoring others' work but can take your guidance.
Thanks!

@zonybob
Copy link
Contributor Author

zonybob commented Jul 9, 2023

Also, @MisterMX you'll notice the failed e2e-tests
The only way I was able to get around that was upgrading up version to 0.16.1

@MisterMX
Copy link
Collaborator

MisterMX commented Jul 10, 2023

notice the failed e2e-tests
The only way I was able to get around that was upgrading up version to 0.16.1

E2E Tests are running successfully for the latest master: https://github.com/crossplane-contrib/provider-aws/actions/runs/5508575184.

@zonybob you might have done some misconfiguration between build and the other make related files. Can you try running the branch without these changes?

@zonybob
Copy link
Contributor Author

zonybob commented Jul 10, 2023

notice the failed e2e-tests
The only way I was able to get around that was upgrading up version to 0.16.1

E2E Tests are running successfully for the latest master: https://github.com/crossplane-contrib/provider-aws/actions/runs/5508575184.

@zonybob you might have done some misconfiguration between build and the other make related files. Can you try running the branch without these changes?

Yes I'm sorry! Forgot all about the build submodule. That is probably it. Will try that out.

@zonybob
Copy link
Contributor Author

zonybob commented Jul 12, 2023

@MisterMX build submodule updated and e2e-tests passed. Let me know how it all looks now. Thanks!

@zonybob zonybob requested a review from MisterMX July 12, 2023 10:17
pkg/controller/iam/user/controller.go Outdated Show resolved Hide resolved
pkg/controller/iam/user/controller.go Outdated Show resolved Hide resolved
@MisterMX
Copy link
Collaborator

MisterMX commented Jul 19, 2023

Looking good for me except for some minor issues: Please encapsulate everything in a separate isUpToDate function and use a distinct error message for remote calls.

better error messages

Signed-off-by: Ben McDonie <[email protected]>
@zonybob
Copy link
Contributor Author

zonybob commented Jul 19, 2023

@MisterMX thanks again for the response. New commits are up. I think I addressed everything this time (hopefully).

@MisterMX
Copy link
Collaborator

@zonybob that looks good now. THank you very much for your contribution!

One last thing: Can you please squash your changes into a single commit in order to keep the history short and concise? Then I can merge it.

@zonybob
Copy link
Contributor Author

zonybob commented Jul 21, 2023

@zonybob that looks good now. THank you very much for your contribution!

One last thing: Can you please squash your changes into a single commit in order to keep the history short and concise? Then I can merge it.

@MisterMX are you able to squash on the merge? If not, could you assist me with this? My git log has intermingled commits from syncing master in periodically, and my rebase powers are minimal :)

@MisterMX
Copy link
Collaborator

@zonybob I could do half of that with Githubs "Squash and Merge" feature. However, it does not - for some reason - create a merge commit which is what we want as well. So we are left with squashing the commits manually.

Squashing in git is comparably easy and can be achieved with an interactive rebase. There are a bunch of tutorials out there that can help understanding it. Essentially it is just git rebase -i origin/master and replacing all feature commits pick (except the first) with either squash (s) or fixup (f).

@MisterMX MisterMX changed the title observe and update IAM user permissions boundary feat(iam): Add observe and update of user permissions boundary Jul 26, 2023
@MisterMX
Copy link
Collaborator

Tested it locally and it seems only a single commit is merged anyways so it should be fine in this case.

@MisterMX MisterMX merged commit d4f4788 into crossplane-contrib:master Jul 28, 2023
8 checks passed
@MisterMX
Copy link
Collaborator

Ok, it wasn't. Seems like Github is doing something differently under the hood when merging.

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.

PermissionsBoundary cannot be modified for IAMRoles or IAMUsers
2 participants