Transfer route ownership #2887

dalvarado · 2022-07-26T21:29:26Z

Co-authored-by: Merric de Launey mdelauney@pivotal.io
Co-authored-by: Alex Rocha alexr1@vmware.com
Co-authored-by: David Alvarado dalvarado@pivotal.io

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

linux-foundation-easycla · 2022-07-26T21:29:29Z

✅ login: moleske / name: M. Oleske (2e481ce, b0e0573, a0293e2, 227ba7d, e6fa5f6, d722cf6, 2cf0cfc, bb6266a)
✅ login: MerricdeLauney (351b1f4, da84f01)
✅ login: xandroc / name: Alex (831c728, 50527bb, 9875032)
❌ - login: @dalvarado / name: david . The commit (8b858b9) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

Gerg · 2022-07-26T21:33:33Z

Note: This is probably note the right endpoint defintion for this action

current frontrunners are:

PATCH v3/routes/:guid/relationships/space -d { "data": {"guid": "space-2-guid" }}
PATCH /v3/routes/:guid/actioins/transfer_owner -d {"guid": "space-2-guid" }

Source: f387129

What is y'all's current thinking on this?

philippthun · 2022-07-28T15:45:38Z

@dalvarado - Rebasing to the current main branch fixes mysql:8.0 tests.

- permissions for roles based on initial space done - next is permissions based on the target_space Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: Alex Rocha <alexr1@vmware.com>

- return 422 if the target space either doesn't exist or you don't have write permissions for target space - next step is to start actually transferring ownership or do request body validation Authored-by: Michael Oleske <moleske@pivotal.io>

Note: This is probably note the right endpoint defintion for this action current frontrunners are: PATCH v3/routes/:guid/relationships/space -d { "data": {"guid": "space-2-guid" }} PATCH /v3/routes/:guid/actioins/transfer_owner -d {"guid": "space-2-guid" } Co-authored-by: Merric de Launey <mdelauney@pivotal.io> Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: David Alvarado <alvaradoda@vmware.com> Co-authored-by: Alex Rocha <alexr1@vmware.com>

Co-authored-by: Alex Rocha <alexr1@vmware.com> Co-authored-by: David Alvarado <alvaradoda@vmware.com> Co-authored-by: Merric de Launey <mdelauney@pivotal.io>

Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: Alex Rocha <alexr1@vmware.com>

…mmit. Our tests include checks that ensure `record_route_transfer_owner` doesn't run on exceptions that result in a commit being rolled back. Unfortunately the DB.transaction block does not bubble up `Sequel::RollBack` exceptions themselves--although it does bubble up all other types of exceptions that would result in a failed commit Co-authored-by: Alex Rocha <alexr1@vmware.com> Co-authored-by: Michael Oleske <moleske@pivotal.io>

Co-authored-by: Alex Rocha <alexr1@vmware.com> Co-authored-by: Michael Oleske <moleske@pivotal.io>

MerricdeLauney · 2022-08-01T17:56:44Z

Note: This is probably note the right endpoint defintion for this action
current frontrunners are:
PATCH v3/routes/:guid/relationships/space -d { "data": {"guid": "space-2-guid" }}
PATCH /v3/routes/:guid/actions/transfer_owner -d {"guid": "space-2-guid" }

Source: f387129

What is y'all's current thinking on this?

I am still unsure if either of these options (or something else?) are more in align with capi's endpoint design. Would love to hear anyone else's opinion.

It's worth noting that the current endpoint is PATCH /v3/routes/:guid/transfer_owner -d {"guid": "space-2-guid" } which is almost certainly not correct.

@Gerg @philippthun

Gerg · 2022-08-01T19:09:03Z

Without thinking about it too deeply, PATCH v3/routes/:guid/relationships/space sounds the most intuitive to me, since you are changing that relationship.

philippthun · 2022-08-02T08:39:32Z

I would also go for the PATCH ... /relationships/ ... endpoint.

philippthun · 2022-08-02T08:47:54Z

docs/v3/source/includes/resources/routes/_transfer_route_ownership.md.erb

+--- |
+Admin |
+Space Developer |
+Space Supporter |


Should a Space Supporter be allowed to perform this action?

Hmm good question. I think you are right, this falls outside the domain of space supporters

philippthun · 2022-08-02T08:50:15Z

app/controllers/v3/routes_controller.rb

+    message = RouteTransferOwnerMessage.new(hashed_params[:body])
+    unprocessable!(message.errors.full_messages) unless message.valid?
+
+    unauthorized! unless permission_queryer.can_manage_apps_in_active_space?(route.space.guid)


The check for a suspended org needs to be added as well:

suspended! unless permission_queryer.is_space_active?(route.space.guid)

philippthun · 2022-08-02T08:58:49Z

app/controllers/v3/routes_controller.rb

+    target_space = Space.first(guid: message.guid)
+    target_space_error = if target_space.nil? || !can_read_space?(target_space)
+                           'Ensure the space exists and that you have access to it.'
+                         elsif !permission_queryer.can_manage_apps_in_active_space?(target_space.guid)
+                           'Ensure that you have write permission for the target space.'
+                         elsif !permission_queryer.is_space_active?(target_space.guid)
+                           'The target organization is suspended.'
+                         end


This is very similar with the checks in unshare_route - the only difference I see is that if the target space is not readable, there is a resource_not_found error instead. Can this be changed to be identical and then extracted into a dedicated method?

started looking at this, still playing around with what making them same looks like. Such as which parts to change in unshare to match transfer or the opposite way. Leaning toward making unshare match transfer rather than transfer match unshare

- pr feedback Authored-by: Michael Oleske <moleske@pivotal.io>

- pr feedback - next commit should extract these to a function - consider test organzation as well Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: David Alvarado <alvaradoda@vmware.com>

- pr feedback Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: David Alvarado <alvaradoda@vmware.com>

…ips/space This change ensures the endpoint adheres to the v3 api style guide https://github.com/cloudfoundry/cloud_controller_ng/blob/main/docs/v3_style_guide.md [#182668597] Authored-by: Merric de Launey <mdelauney@pivotal.io>

tcdowney · 2022-08-05T22:51:03Z

Following along with this to see if/when we might want to support this change in Korifi.

Without thinking about it too deeply, PATCH v3/routes/:guid/relationships/space sounds the most intuitive to me, since you are changing that relationship.

Agreed, goes well with the existing route sharing endpoints.
https://v3-apidocs.cloudfoundry.org/version/3.122.0/index.html#share-a-route-with-other-spaces-experimental

url "https://api.example.org/v3/routes/[guid]/relationships/shared_spaces" \
  -X POST \
  -H "Authorization: bearer [token]" \
  -H "Content-type: application/json" \
  -d '{
    "data": [
      { "guid": "space-one-guid" },
      { "guid": "space-two-guid" }
    ]
  }'

cf-gitbot added the unscheduled label Jul 26, 2022

moleske and others added 7 commits July 28, 2022 09:54

Initial transfer_owner commit

2e481ce

- permissions for roles based on initial space done - next is permissions based on the target_space Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: Alex Rocha <alexr1@vmware.com>

Finishes up the action for transferring route to another space

831c728

Co-authored-by: Alex Rocha <alexr1@vmware.com> Co-authored-by: David Alvarado <alvaradoda@vmware.com> Co-authored-by: Merric de Launey <mdelauney@pivotal.io>

Add documentation for the new transfer route ownership endpoint

8b858b9

Remove unused custom error

a0293e2

Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: Alex Rocha <alexr1@vmware.com>

moleske force-pushed the transfer-route-ownership branch from bfd38ca to 50527bb Compare July 28, 2022 17:10

Fix incorrect href link in transfer-route-ownership

9875032

Co-authored-by: Alex Rocha <alexr1@vmware.com> Co-authored-by: Michael Oleske <moleske@pivotal.io>

philippthun reviewed Aug 2, 2022

View reviewed changes

moleske and others added 6 commits August 3, 2022 22:59

Remove space supporter from being allowed to transfer route ownership

227ba7d

- pr feedback Authored-by: Michael Oleske <moleske@pivotal.io>

Return org suspended error if org is suspended

e6fa5f6

- pr feedback Authored-by: Michael Oleske <moleske@pivotal.io>

Make unshare route and transfer owner have the same checks

d722cf6

- pr feedback - next commit should extract these to a function - consider test organzation as well Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: David Alvarado <alvaradoda@vmware.com>

Extract duplicate code for space accessibility

2cf0cfc

- pr feedback Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: David Alvarado <alvaradoda@vmware.com>

Switch route to /routes/:guid/relationships/space

bb6266a

- pr feedback Co-authored-by: Michael Oleske <moleske@pivotal.io> Co-authored-by: David Alvarado <alvaradoda@vmware.com>

MerricdeLauney merged commit 6dbd10a into main Aug 8, 2022

cf-gitbot removed the unscheduled label Aug 8, 2022

moleske deleted the transfer-route-ownership branch August 8, 2022 19:02

beyhan mentioned this pull request May 3, 2024

Add David Alvarado as CLI approver cloudfoundry/community#835

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transfer route ownership #2887

Transfer route ownership #2887

dalvarado commented Jul 26, 2022 •

edited by xandroc

Loading

linux-foundation-easycla bot commented Jul 26, 2022 •

edited

Loading

Gerg commented Jul 26, 2022 •

edited

Loading

philippthun commented Jul 28, 2022

MerricdeLauney commented Aug 1, 2022

Gerg commented Aug 1, 2022 •

edited

Loading

philippthun commented Aug 2, 2022

philippthun Aug 2, 2022

MerricdeLauney Aug 2, 2022

philippthun Aug 2, 2022

philippthun Aug 2, 2022

moleske Aug 3, 2022 •

edited

Loading

tcdowney commented Aug 5, 2022

Transfer route ownership #2887

Transfer route ownership #2887

Conversation

dalvarado commented Jul 26, 2022 • edited by xandroc Loading

linux-foundation-easycla bot commented Jul 26, 2022 • edited Loading

Gerg commented Jul 26, 2022 • edited Loading

philippthun commented Jul 28, 2022

MerricdeLauney commented Aug 1, 2022

Gerg commented Aug 1, 2022 • edited Loading

philippthun commented Aug 2, 2022

philippthun Aug 2, 2022

Choose a reason for hiding this comment

MerricdeLauney Aug 2, 2022

Choose a reason for hiding this comment

philippthun Aug 2, 2022

Choose a reason for hiding this comment

philippthun Aug 2, 2022

Choose a reason for hiding this comment

moleske Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

tcdowney commented Aug 5, 2022

dalvarado commented Jul 26, 2022 •

edited by xandroc

Loading

linux-foundation-easycla bot commented Jul 26, 2022 •

edited

Loading

Gerg commented Jul 26, 2022 •

edited

Loading

Gerg commented Aug 1, 2022 •

edited

Loading

moleske Aug 3, 2022 •

edited

Loading