Refactor permissions functions #2468

mkocher · 2021-08-16T22:27:40Z

Now that space supporter implementation is complete we can collapse a
bunch of functions in permissions.rb and finalize naming conventions
that we're mostly happy with.

include space supporter in ALL_PERMISSIONS
removed space supporter specific mentions in permissions.rb

Co-authored-by: Mona Mohebbi [email protected]
Co-authored-by: Merric de Launey [email protected]
Co-authored-by: Matthew Kocher [email protected]

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

A short explanation of the proposed change:
An explanation of the use cases your change solves
Links to any other associated PRs
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the main branch
I have run all the unit tests using bundle exec rake
I have run CF Acceptance Tests

Now that space supporter implementation is complete we can collapse a bunch of functions in permissions.rb and finalize naming conventions that we're mostly happy with. * include space supporter in ALL_PERMISSIONS * removed space supporter specific mentions in permissions.rb Co-authored-by: Mona Mohebbi <[email protected]> Co-authored-by: Merric de Launey <[email protected]> Co-authored-by: Matthew Kocher <[email protected]>

sweinstein22 · 2021-08-17T00:01:13Z

I'm going to be OOO tomorrow but I'll plan to have a look at this on Wednesday. In the meantime just want to call out it looks like we've got some merge conflicts to resolve

mkocher · 2021-08-17T17:49:59Z

Thank you. We resolved the merge conflicts. /cc @MerricdeLauney

sweinstein22 · 2021-08-18T20:39:11Z

lib/cloud_controller/permissions.rb

@@ -288,15 +262,15 @@ def can_read_system_environment_variables?(space_guid, org_guid)
      membership.has_any_roles?(ROLES_FOR_SPACE_SECRETS_READING, space_guid, org_guid)
  end

-  def untrusted_can_read_route?(space_guid, org_guid)
-    return true if can_read_globally?
+  # def can_read_route?(space_guid, org_guid)


Is there a reason we've left this method commented out rather than removing it?

No, good catch!

sweinstein22

Code Review

I'm noticing that for the most part we were just removing supporter or untrusted from method names, and then making the adjustment to ALL_PERMISSIONS to include space_supporter. The exceptions seem to be:

untrusted_can_write_to_space => can_manage_apps_in_space
untrusted_can_read_from_space => can_download_droplet

Looks like we updated the tests as appropriate for those two.

Changes Requested

I did a quick search through the branch for untrusted and noticed we still seem to have mention of some of these methods in:

service_route_bindings_controller.rb
service_instances_controller.rb

I think addressing those would be part of the associated issue for this PR. I also left a comment on one section, looks like we left in a commented out method, just wondering if we left it for a specific reason.

Needed for #2462. Rubocop 1.19 includes changes to the ConditionalAssignment that cause it to flag some `case` statements as violations. We don't agree with the ConditionalAssignment cop's preferred style, so we're going to disable it.

Bumps [rubocop](https://github.com/rubocop/rubocop) from 1.18.3 to 1.19.0. - [Release notes](https://github.com/rubocop/rubocop/releases) - [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop@v1.18.3...v1.19.0) --- updated-dependencies: - dependency-name: rubocop dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

This commit goes alongside work to TPS and runtimeschema. Together they allow CF-for-VMs users to see when their app instances get evacuated from Diego Cells. As a user running a complex web service on top of Cloud Foundry, it can be confusing when apps seem to restart for no reason. Even once users understand this happens during Diego updates, they still have no automatic insight into when that happened. The audit event is called `audit.app.process.rescheduling` so that is quite generic to whatever container scheduler is used behind the scenes. For instance Eirini could talk to this endpoint and provide a different `reason` string to explain the rescheduling.

Co-authored-by: Matthew Kocher <[email protected]> Co-authored-by: Merric de Launey <[email protected]>

- this is a little odd because route destinations store http2 but don't store tcp as a version. This causes us to have to handle some edge cases so there are a lot of specs - noticed that internal routes are currently reported to be http though they do not logically have a protocol as they're just a DNS entry - not using this field yet. next we will use this to decorate the routing information of the LRPs. - this is a big commit. We regret that we cannot find a good way to make it smaller. Authored-by: Merric de Launey <[email protected]>

* Also refactored the routinginfo class Co-authored-by: Michael Oleske <[email protected]> Co-authored-by: Carson Long <[email protected]>

Co-authored-by: Michael Oleske <[email protected]> Co-authored-by: Carson Long <[email protected]>

cloudfoundry/routing-release#200 Co-authored-by: Michael Oleske <[email protected]> Co-authored-by: Carson Long <[email protected]>

cloudfoundry/routing-release#200 Co-authored-by: Michael Oleske <[email protected]>

- clarify when http1/http2/tcp are valid based on route protocol - mention it could be null Co-authored-by: Michael Oleske <[email protected]>

Co-authored-by: Carson Long <[email protected]>

When adding route destination and it already exists with different protocol

Co-authored-by: Carson Long <[email protected]>

Co-authored-by: Weyman Fung <[email protected]> Co-authored-by: Carson Long <[email protected]>

* applies only to non root and info endpoints * applies only if the space supporter role is the only role assigned * applies only if the flag to enable usage of the space supporter is set to true * addresses #2434 Co-authored-by: Mona Mohebbi <[email protected]> Co-authored-by: Merric de Launey <[email protected]> Co-authored-by: Matthew Kocher <[email protected]> Co-authored-by: Weyman Fung <[email protected]>

Bumps [listen](https://github.com/guard/listen) from 3.6.0 to 3.7.0. - [Release notes](https://github.com/guard/listen/releases) - [Commits](guard/listen@v3.6.0...v3.7.0) --- updated-dependencies: - dependency-name: listen dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [rubocop](https://github.com/rubocop/rubocop) from 1.19.0 to 1.19.1. - [Release notes](https://github.com/rubocop/rubocop/releases) - [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md) - [Commits](rubocop/rubocop@v1.19.0...v1.19.1) --- updated-dependencies: - dependency-name: rubocop dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: Matthew Kocher <[email protected]> Co-authored-by: Merric de Launey <[email protected]>

- this is a little odd because route destinations store http2 but don't store tcp as a version. This causes us to have to handle some edge cases so there are a lot of specs - noticed that internal routes are currently reported to be http though they do not logically have a protocol as they're just a DNS entry - not using this field yet. next we will use this to decorate the routing information of the LRPs. - this is a big commit. We regret that we cannot find a good way to make it smaller. Authored-by: Merric de Launey <[email protected]>

address #2468 Authored-by: Mona Mohebbi <[email protected]>

linux-foundation-easycla · 2021-08-20T21:26:06Z

✅ Mona Mohebbi (05ca7d1, 0671eb3)
✅ Matthew Kocher (60465e8, f7e0161, 9edbfc1, 03d00ce, 399a581)
✅ dependabot[bot] (a3b7b27, 8368a88, 7f257b6)
✅ Miki Mokrysz (135ba1b)
✅ Philipp Thun (bb3f31a)
✅ M. Oleske (6c588b9, 23a67fe)
✅ Carson Long (cdfbca7, 1fcbdd5, e4343bc, 2542e22, 81e2576, 48200ca, 53f6424, 86296f3)
✅ Weyman Fung (7306ceb, c0e4ee0, a630299)

❌ - Tom Viehman The commit (677fd95) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.
❌ - capi-bot The commit (ff7eb31 ,ea60dfcb45fe76cc217a76ee680a36abdd65eadf ,3ff13ecbde92e075b145fc044c0dbaaa31cd7713) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

address #2468 Authored-by: Mona Mohebbi <[email protected]>

monamohebbi · 2021-08-20T22:24:59Z

this branch has gotten a little too messy. Opening a new pr with the right commits cherry picked on top

address #2468 Authored-by: Mona Mohebbi <[email protected]>

* Refactor permissions functions Now that space supporter implementation is complete we can collapse a bunch of functions in permissions.rb and finalize naming conventions that we're mostly happy with. * include space supporter in ALL_PERMISSIONS * removed space supporter specific mentions in permissions.rb address #2468 Co-authored-by: Mona Mohebbi <[email protected]> Co-authored-by: Merric de Launey <[email protected]> Co-authored-by: Matthew Kocher <[email protected]>

cf-gitbot added the unscheduled label Aug 16, 2021

mkocher requested a review from sweinstein22 August 16, 2021 22:28

Merge branch 'main' into space-supporter-refactor-2

60465e8

sweinstein22 reviewed Aug 18, 2021

View reviewed changes

sweinstein22 suggested changes Aug 18, 2021

View reviewed changes

sweinstein22 linked an issue Aug 19, 2021 that may be closed by this pull request

Space Supporter finish line: Refactor #2444

Closed

4 tasks

tjvman and others added 21 commits August 20, 2021 21:12

Disable Style/ConditionalAssignment cop

677fd95

Needed for #2462. Rubocop 1.19 includes changes to the ConditionalAssignment that cause it to flag some `case` statements as violations. We don't agree with the ConditionalAssignment cop's preferred style, so we're going to disable it.

bump to 2.169.0

ff7eb31

bump to 3.104.0

ea60dfc

Bump v2 API docs version 2.169.0

3ff13ec

Wrap all errors in delete_non_transactional_subresources

bb3f31a

Refactor routes_controller to dedupe some logic

f7e0161

Co-authored-by: Matthew Kocher <[email protected]> Co-authored-by: Merric de Launey <[email protected]>

Add protocol to routing_info

6c588b9

* Also refactored the routinginfo class Co-authored-by: Michael Oleske <[email protected]> Co-authored-by: Carson Long <[email protected]>

Refactor to make private functions idempotent

cdfbca7

Co-authored-by: Michael Oleske <[email protected]> Co-authored-by: Carson Long <[email protected]>

Add protocol to the desired LRP

1fcbdd5

Co-authored-by: Michael Oleske <[email protected]> Co-authored-by: Carson Long <[email protected]>

Update route destination object docs with protocol

e4343bc

Co-authored-by: Michael Oleske <[email protected]> Co-authored-by: Carson Long <[email protected]>

Add experimental to protocol field in destination object

2542e22

Co-authored-by: Michael Oleske <[email protected]> Co-authored-by: Carson Long <[email protected]>

Use slice to simplify matching route hashes

81e2576

cloudfoundry/routing-release#200 Co-authored-by: Michael Oleske <[email protected]> Co-authored-by: Carson Long <[email protected]>

Update destination object doc to reflect that protocol can be null

48200ca

cloudfoundry/routing-release#200 Co-authored-by: Michael Oleske <[email protected]>

Update route destination object description

53f6424

- clarify when http1/http2/tcp are valid based on route protocol - mention it could be null Co-authored-by: Michael Oleske <[email protected]>

Fix nil symbol

23a67fe

Co-authored-by: Carson Long <[email protected]>

Improve the error message when protocol does not match

86296f3

When adding route destination and it already exists with different protocol

Refactor UpdateRouteDestinations error message on protocol uniqueness

7306ceb

Co-authored-by: Carson Long <[email protected]>

Remove redundant tcp check from routing info

c0e4ee0

Co-authored-by: Weyman Fung <[email protected]> Co-authored-by: Carson Long <[email protected]>

weymanf and others added 6 commits August 20, 2021 21:16

Refactor routes_controller to dedupe some logic

03d00ce

Co-authored-by: Matthew Kocher <[email protected]> Co-authored-by: Merric de Launey <[email protected]>

Refactor to remove additional untrusted references

0671eb3

address #2468 Authored-by: Mona Mohebbi <[email protected]>

monamohebbi added a commit that referenced this pull request Aug 20, 2021

Refactor to remove additional untrusted references

81e837c

address #2468 Authored-by: Mona Mohebbi <[email protected]>

monamohebbi closed this Aug 20, 2021

cf-gitbot removed the unscheduled label Aug 20, 2021

monamohebbi added a commit that referenced this pull request Aug 23, 2021

Refactor to remove additional untrusted references

1fb9fba

address #2468 Authored-by: Mona Mohebbi <[email protected]>

mkocher added the space-application-supporter https://github.com/cloudfoundry/cfar-proposals/issues/22 label Aug 23, 2021

moleske deleted the space-supporter-refactor-2 branch November 2, 2024 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor permissions functions #2468

Refactor permissions functions #2468

mkocher commented Aug 16, 2021

sweinstein22 commented Aug 17, 2021

mkocher commented Aug 17, 2021

sweinstein22 Aug 18, 2021

monamohebbi Aug 20, 2021

sweinstein22 left a comment

linux-foundation-easycla bot commented Aug 20, 2021

monamohebbi commented Aug 20, 2021

Refactor permissions functions #2468

Refactor permissions functions #2468

Conversation

mkocher commented Aug 16, 2021

sweinstein22 commented Aug 17, 2021

mkocher commented Aug 17, 2021

sweinstein22 Aug 18, 2021

Choose a reason for hiding this comment

monamohebbi Aug 20, 2021

Choose a reason for hiding this comment

sweinstein22 left a comment

Choose a reason for hiding this comment

Code Review

Changes Requested

linux-foundation-easycla bot commented Aug 20, 2021

monamohebbi commented Aug 20, 2021