Isolation segments <-> Organizations #685

ghost · 2016-09-23T01:34:57Z

Created a "shared" Isolation Segment that gets seeded in the database.

Organizations have a list of "allowed Isolation Segments"

Only Admins can manage the allowed list
Space's Isolation Segments must belong to an Organization's allowed list
Org managers can manage Space's Isolation Segment

Organizations can set a 'default' from the allowed list.

A number of endpoints to manage Isolation Segments:

post '/isolation_segments/:guid/relationships/organizations'
delete '/isolation_segments/:guid/relationships/organizations'
get '/isolation_segments/:guid/relationships/organizations'
get '/isolation_segments/:guid/relationships/spaces'
put '/v2/organizations/:guid' -d '{"default_isolation_segment_guid":"guid"}'
get /v2/organizations/:guid/isolation_segments

cfdreddbot · 2016-09-23T01:34:58Z

Hey DanLavine!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

cf-gitbot · 2016-09-23T01:34:59Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/130958055

The labels on this github issue will be updated when the story is started.

Gerg · 2016-10-11T21:43:42Z

app/controllers/runtime/organizations_controller.rb

+      dataset = IsolationSegmentModel.dataset.where(organizations: org)
+      message = IsolationSegmentsListMessage.from_params(@params)
+
+      [HTTP::OK, MultiJson.dump(Presenters::V3::PaginatedListPresenter.new(


Using the v3 presenter will mean that this will look like a v3 endpoint, even though it is in the v2 api.

My understanding from @zrob was that using the v3 presenter was okay, since these are v3-only objects - for example, GET /v3/isolation_segments will use this same presenter, so we'd like to keep the same look/feel. There is no v2 isolation segment API. Zach, did we misunderstand the outcome of our last conversation on this?

I've talked some with @SocalNick about this.

By exposing this resource on the v2 API, it is not longer a v3-only object. Using V3:PaginatedListPresenter means that you will get v3 pagination even though you are hitting a URL that begins with "v2." If I'm building an API client, this means that I can no longer key off of the api version number to determine what pagination scheme will be returned.

Given that, the two options are:

Don't expose isolation segments through the v2 api

Build v2 presenters for isolation segments

Since these are still experimental endpoints, this change doesn't have to block merging this PR, but we need to change it before going GA.

@Gerg Thanks for checking with Nick on this. If you can let me know when you've finished merging, I'll see about a follow-on PR to implement a v2 presenter for Isolation Segments.

-Sandy

Gerg · 2016-10-11T22:19:53Z

spec/api/documentation/organizations_api_spec.rb

+                'guid'        => isolation_segment_model.guid,
+                'created_at'  => iso8601,
+                'updated_at'  => nil,
+                'links'       => {


Related to my other comment: the pagination and resource links are v3-style here.

Yep, please see my response above. Need @zrob or @utako to weigh in with they remember us having agreed on.

@Gerg @ScarletTanager I remember that we considered the body the same, but we never expected to see links or v3 pagination on the v2 api

- has a code-constant guid - name can be changed via manifest - cannot be deleted or updated via API Signed-off-by: Dan Lavine <[email protected]>

Signed-off-by: Dan Lavine <[email protected]>

- IS<->org is many:many - Allows for creation and reading of IS<->org assignments - Organization has a default_isolation_segment which is the first isolation segment assigned to the org - Setup logic for deletion of orgs with isolation segments. This also respects space associations with Isolation Segments - Organizations list the isolation segments associated with them - Isolation Segments have relationship endpoints to list Organizations and spaces - Isolation Segments can add organizations through a bulk api - Note: There are no query parameters for isolation segmetns on Organizations [finishes #129611053] Signed-off-by: Dan Lavine <[email protected]>

…rganizations' Signed-off-by: Sandy Cash <[email protected]>

…:"guid"}' - This is an OrgManager/Admin only endpoint Signed-off-by: Sandy Cash <[email protected]>

[finishes #129612885] Signed-off-by: Dan Lavine <[email protected]>

[finishes #129612925] Signed-off-by: Dan Lavine <[email protected]>

[#130698053] Signed-off-by: Dan Lavine <[email protected]>

Signed-off-by: Dan Lavine <[email protected]>

- Improve error messages - Add 'relationships' for get urls - Move isolation segments <-> organizations to end of migration list Signed-off-by: Dan Lavine <[email protected]>

Gerg · 2016-10-12T17:32:25Z

Since we have been so slow reviewing/merging this, there were a bunch of merge issues. I rebased and pushed to the isolation_segments branch

Gerg

I didn't have time to go through most of the specs yet.

Gerg · 2016-10-12T18:23:10Z

app/controllers/runtime/spaces_controller.rb

-        isolation_segment = IsolationSegmentModel[guid: request_attrs['isolation_segment_guid']]
-        raise CloudController::Errors::ApiError.new_from_details('InvalidRelation',
-                "Could not find Isolation Segment with guid: #{request_attrs['isolation_segment_guid']}") unless isolation_segment
+        isolation_segment_guids = obj.organization.isolation_segment_models.map(&:guid)


Law of Demeter. Maybe organizations should have an accessor for isolation segments?

Not sure what you mean - organization.isolation_segment_models is the accessor for ISes from orgs...? The space needs to navigate to the ISes to which the owning org is entitled, I don't really see a shorter path to get there?

Sorry about the confusion. This was a note to myself that I forgot to edit.

I'm talking about obj.organization.isolation_segment_models.map(&:guid) vs something like obj.organization.get_isolation_segment_guids. That way you define a public interface for organizations instead of reaching into their internal state. We do the former a lot in CC anyway, so this isn't blocking the PR.

Okay, thanks for the clarification.

Gerg · 2016-10-12T18:27:11Z

app/controllers/v3/isolation_segments_controller.rb

+      roles.admin? ||
+      isolation_segment_model.spaces.any? { |space| can_read?(space.guid, space.organization.guid) } ||
+      isolation_segment_model.organizations.any? { |org| can_read_from_org?(org.guid) }
+    )


This logic is starting to get pretty hairy, might be nice to break out helper methods.

Sure, I can do that.

Fixed. (Coming soon to a commit near you)

Gerg · 2016-10-12T18:34:49Z

app/controllers/v3/isolation_segments_controller.rb

@@ -56,6 +60,9 @@ def destroy
    isolation_segment_model = IsolationSegmentModel.where(guid: params[:guid]).first
    resource_not_found!(:isolation_segment) unless isolation_segment_model

+    method_not_allowed!('DELETE', "the #{isolation_segment_model.name} Isolation Segment") if


This is the first time we are returning a 405 from the CC API, and I don't think this is the correct usage. DELETE is a supported action for IsolationSegments, just not this particular isolation segment. I'm not sure what the current idiom is for these (maybe just 400?) @SocalNick

Relevant docs: https://httpstatuses.com/405

I think a 405 is exactly the correct response here - from the spec: "The method specified in the Request-Line is not allowed for the resource identified by the Request-URI." A 405 doesn't mean you can't DELETE any IS, just this one IS.

Does 409 seem more appropriate? https://httpstatuses.com/409

@SocalNick I don't think so - a 409 implies that you could tweak the request - but same method, same resource - and have it succeed. That isn't the case here, there is literally no way to delete (or update) this particular segment.

Gerg · 2016-10-12T18:35:24Z

app/controllers/v3/isolation_segments_controller.rb

    isolation_segment_model = IsolationSegmentModel.where(guid: params[:guid]).first
    resource_not_found!(:isolation_segment) unless isolation_segment_model

+    method_not_allowed!('PUT', "the #{isolation_segment_model.name} Isolation Segment") if


Same 405 issue as above

Same 405 response as above

Gerg · 2016-10-12T18:36:24Z

app/controllers/v3/application_controller.rb

@@ -16,6 +16,14 @@ def unprocessable!(message)
  def unauthorized!
    raise CloudController::Errors::ApiError.new_from_details('NotAuthorized')
  end
+
+  def method_not_allowed!(method, obj)


See my other comments about if we should ever be returning 405s

lIRC we had at least one legit case for a 405 - but I'll look at each of the cases you pointed out and be sure we're staying on the reservation.

I think both cases where we return 405s are legit, so I think this should stay.

Gerg · 2016-10-13T00:32:19Z

db/migrations/20160922213611_organization_isolation_segments.rb

+  end
+
+  down do
+    alter_table :organizations do


Do you also need to remove the default_isolation_segment_guid column? Or does Sequel magically do that when dropping the foreign key?

I'll check.

Gerg · 2016-10-13T00:42:37Z

spec/api/documentation/organizations_api_spec.rb

@@ -19,6 +19,7 @@
      field :status, 'Status of the organization'
      field :quota_definition_guid, 'The guid of quota to associate with this organization', example_values: ['org-quota-def-guid']
      field :billing_enabled, 'If billing is enabled for this organization', deprecated: true
+      field :default_isolation_segment_guid, 'The guid for the default isolation segment', experimental: true


We are no longer automatically updating the v2 api docs via these tests. In order for this change to be reflected in the docs, you will have to manually edit them.

Gerg · 2016-10-13T00:52:49Z

spec/request/isolation_segments_spec.rb


+    it 'returns the seeded isolation segment' do


This seems more appropriate as a controller spec

I think I disagree. We want to show that the API will always return the seeded segment, even if no others have been created - this seems like a basic happy path behavior to me and appropriate for a request spec.

Gerg · 2016-10-13T00:56:17Z

spec/request/isolation_segments_spec.rb

+        expect(parsed_response['pagination']).to eq(expected_pagination)
+      end
+
+      context 'when the user is not an admin' do


This would also be more appropriate as a controller spec. Generally, we want the request specs to only cover super happy path cases since they are relatively slow.

Correct me if I'm wrong @utako

Gerg · 2016-10-13T00:56:52Z

spec/spec_helper.rb

@@ -12,6 +12,8 @@
 require 'timecop'
 require 'awesome_print'

+require 'pry'


Unrelated to PR.

Whoops, this leaked in. I'll remove it. As a side note, is there any good reason not to include this in the spec_helper?

Signed-off-by: Dan Lavine <[email protected]>

aashah · 2016-10-13T18:20:58Z

spec/unit/controllers/runtime/organizations_controller_spec.rb

            post '/v2/organizations', MultiJson.dump({ name: 'my-org-name' })

            expect(last_response.status).to eq(201)
            org = Organization.find(name: 'my-org-name')
            expect(org.managers.count).to eq(0)
          end
+
+          it 'does not allow creation with a default isolation segment set' do


Reads weird (as it's not preventing creation), when the contents indicate a 201 - Created.

Fixed. Spec description was incorrect, code is correct.

aashah · 2016-10-13T18:28:17Z

spec/unit/controllers/runtime/spaces_controller_spec.rb

+            organization.add_manager(user)
+          end
+
+          # it 'fails with a 403' do


Extra comment.

Fixed, also later in the same file.

aashah · 2016-10-13T18:32:44Z

spec/unit/lib/cloud_controller/membership_spec.rb

@@ -5,6 +5,7 @@ module VCAP::CloudController
    let(:user) { User.make }
    let!(:space) { Space.make(organization: organization) }
    let(:organization) { Organization.make }
+    let(:organization2) { Organization.make }


Unused variable.

aashah · 2016-10-13T18:55:43Z

spec/unit/models/runtime/v3/persistence/isolation_segment_model_spec.rb

+        end
+
+        context 'when setting the default isolation segment' do
+          it 'must be in the allowed list' do


Empty test.

Ugh. This behavior is partially handled (where it should be) in the organization model spec, but there are empty tests there as well. Fixing.

aashah · 2016-10-13T18:55:51Z

spec/unit/models/runtime/v3/persistence/isolation_segment_model_spec.rb

+          it 'must be in the allowed list' do
+          end
+
+          it 'can be updated' do


Empty test.

See previous.

Signed-off-by: Dan Lavine <[email protected]>

ScarletTanager · 2016-10-14T15:08:21Z

@Gerg @SocalNick @zrob @aashah I fixed most of the issues, but I think the following remain:

Moving the IS model hooks into v3 action(s)
Adding a v2 presenter for ISes (does not block PR, but needs to happen before GA)
Moving one negative spec from IS request spec into controller spec
Need to come to some agreement on response codes - for the most part, I think the 405s are correct. The one that still sticks is the 501 case, which may need to become something else (405 or 409, possibly?). Need a verdict on that. Does this block the PR?

Gerg · 2016-10-17T21:55:57Z

Regarding all the discussion around HTTP status codes: One of our objectives for v3 was to avoid the status code pedantry that I've been gleefully engaging in.

Zach reminded me that in our v3 style guide we have a small list of response codes: https://github.com/cloudfoundry/cc-api-v3-style-guide#response-codes. The idea is to use the response codes somewhat generically, but have descriptive error messages, since those are what most people care about anyway.

Given that, it seems like 422 is appropriate for the 501/405 cases. Thoughts?

ScarletTanager · 2016-10-18T13:15:55Z

@Gerg - I don't think so - I actually think that a 405 is correct for the two cases where I'm defending it (there is an entity identifiable by the URI path for which the verb used will not be accepted ever). It's not because there is some semantic error (I realize that the scope of what could qualify for a 422 is pretty broad, which is one of the reasons to be careful about using it). In the case where we had a 501, but you argued for a 405, I think you are probably correct - I think that should be a 405. Since I'm X-team pairing with CAPI today, we can probably work this out in real time.

ScarletTanager · 2016-10-24T13:08:32Z

@Gerg @SocalNick any news on when we might get this merged? It's blocking a bunch of stuff for us.

Gerg · 2016-10-25T18:54:04Z

Our pipeline went green Friday EOD, so we should be able to push today.

On Monday, October 24, 2016, Sandy Cash [email protected] wrote:

@Gerg https://github.com/gerg @SocalNick https://github.com/SocalNick
any news on when we might get this merged? It's blocking a bunch of stuff
for us.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#685 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA1ddM-ZyIbV5Mgu7h-oEaabP1nH6GWSks5q3K3XgaJpZM4KEhoS
.

ScarletTanager · 2016-10-26T20:54:35Z

@Gerg @SocalNick @zrob - it appears that all the commits from this PR are now in master. Can we close this?

cf-gitbot added the unscheduled label Sep 23, 2016

Gerg reviewed Oct 11, 2016

View reviewed changes

ScarletTanager and others added 10 commits October 12, 2016 10:29

Shared Isolation Segment is seeded with the database

c51d0a0

- has a code-constant guid - name can be changed via manifest - cannot be deleted or updated via API Signed-off-by: Dan Lavine <[email protected]>

fix up specs that now show the isolation segment seed

60cc662

Signed-off-by: Dan Lavine <[email protected]>

Implement 'DELETE /v3/isolation_segment/:guid/relationships/allowed_o…

5f685e8

…rganizations' Signed-off-by: Sandy Cash <[email protected]>

Implement 'PUT /v2/organizations/:guid -d '{"isolation_segments_guid"…

b9c7c69

…:"guid"}' - This is an OrgManager/Admin only endpoint Signed-off-by: Sandy Cash <[email protected]>

OrgManager can set space isolation segment

88bd5cb

[finishes #129612885] Signed-off-by: Dan Lavine <[email protected]>

OrgManager can delete a space's associated IS

0a8dc80

[finishes #129612925] Signed-off-by: Dan Lavine <[email protected]>

isolation segmetn relationships respect user read scope

0133cb1

[#130698053] Signed-off-by: Dan Lavine <[email protected]>

revert changes to hide links for v3 presenters

10a112c

Signed-off-by: Dan Lavine <[email protected]>

Cleanup code quality

801db0e

- Improve error messages - Add 'relationships' for get urls - Move isolation segments <-> organizations to end of migration list Signed-off-by: Dan Lavine <[email protected]>

Gerg reviewed Oct 13, 2016

View reviewed changes

ScarletTanager added 3 commits October 13, 2016 12:33

Move IS access logic into helper methods

420d911

Signed-off-by: Dan Lavine <[email protected]>

isolation_segment_assign_org_message only uses strings for keys

25d3cc6

Signed-off-by: Dan Lavine <[email protected]>

Add tests for error messages

05219e8

Signed-off-by: Dan Lavine <[email protected]>

aashah reviewed Oct 13, 2016

View reviewed changes

ScarletTanager added 2 commits October 14, 2016 09:18

Make default_isolation_segment setter more clearly a setter

48515fb

Signed-off-by: Dan Lavine <[email protected]>

General code cleanup according to PR comments

d3450b0

Signed-off-by: Dan Lavine <[email protected]>

ScarletTanager force-pushed the isolation_segments branch from 2173fb1 to d3450b0 Compare October 14, 2016 15:02

SocalNick closed this Oct 26, 2016

ghost mentioned this pull request Nov 15, 2016

Change how to present Isolation Segments from Orgs. #719

Merged

Isolation segments <-> Organizations #685

Isolation segments <-> Organizations #685

Uh oh!

Conversation

ghost commented Sep 23, 2016 • edited by Gerg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfdreddbot commented Sep 23, 2016

Uh oh!

cf-gitbot commented Sep 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gerg commented Oct 12, 2016

Uh oh!

Gerg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScarletTanager Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScarletTanager Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 23, 2016 •

edited by Gerg

Loading

ScarletTanager Oct 13, 2016 •

edited

Loading

ScarletTanager Oct 13, 2016 •

edited

Loading