Conversation
9e6d5ca to
72a49b2
Compare
lfundaro-do
left a comment
There was a problem hiding this comment.
- Aggregator already has
GET /v2/vpcs/{vpc_uuid}/subnets/{subnet_uuid}/members(resource_type, pagination). VPC-level members are already in the public spec (vpcs_list_members). If members ships with subnets, this PR is incomplete.
| curl -X POST \ | ||
| -H "Content-Type: application/json" \ | ||
| -H "Authorization: Bearer $DIGITALOCEAN_TOKEN" \ | ||
| -d '{"destination_cidr":"0.0.0.0/0","target_urns":["do:droplet:14aa1d1b-e6ab-4ccb-bb10-dade56fcb8ec"]}' \ |
There was a problem hiding this comment.
we will accept only numerical droplets, no uuids allowed
lfundaro-do
left a comment
There was a problem hiding this comment.
- Aggregator already has
GET /v2/vpcs/{vpc_uuid}/subnets/{subnet_uuid}/members(resource_type, pagination). VPC-level members are already in the public spec (vpcs_list_members). If members ships with subnets, this PR is incomplete.
72a49b2 to
dce264c
Compare
dce264c to
a98d63c
Compare
| @@ -0,0 +1,41 @@ | |||
| list_vpc: | |||
| lang: cURL | |||
There was a problem hiding this comment.
tested this against a live account. Please address the following:
-
Pagination bug on list_members
The spec says per_page and page are optional (defaulting to 20 and 1, like the rest of our API). But if you actually hit the GET endpoint without those query params, it throws a 422 unprocessable_entity: "Pagination is invalid". If you explicitly add ?page=1&per_page=20, it returns a 200.
We either need to fix the backend to respect the defaults, or mark these as required: true in the spec. (Though making them required breaks our usual API pagination convention, so I'd vote for fixing the backend). -
Missing resource_type enum value
The resource_type enum in parameters.yml only lists droplet, loadbalancer, dbaas, and kubernetes. However, I saw a real subnet member return "urn":"do:nfs_share:...". We should add nfs_share (and double-check if there are any others) so the filter actually works for everything. -
name shouldn't be readOnly
In vpc_subnet.yml, name is marked as readOnly: true, but this PR also adds a PATCH endpoint to rename subnets. We should drop the readOnly flag here since the client can clearly change it, similar to how it's handled in the base vpc.yml model. -
404 on empty VPC routes
I hit GET /v2/vpcs/{vpc_uuid}/routes on a VPC that didn't have any static routes configured and got a 404. Is that expected behavior? Normally, I'd expect a 200 with an empty list []. Might just be a feature flag/rollout quirk on my end, but wanted to flag it just in case. -
Dual security scopes on create/delete
The specs for creating and deleting subnets accept either vpc:create/vpc:delete OR vpc:update. That means a token scoped strictly to vpc:update can completely create or wipe out subnets.
If the idea is that subnets just inherit permissions from their parent VPC, that's a totally fair product call. I just wanted to point it out to confirm it's intentional, since the rest of the PR sticks pretty strictly to one scope per verb.
There was a problem hiding this comment.
404 on empty VPC routes
I hit GET /v2/vpcs/{vpc_uuid}/routes on a VPC that didn't have any static routes configured and got a 404. Is that expected behavior? Normally, I'd expect a 200 with an empty list []. Might just be a feature flag/rollout quirk on my end, but wanted to flag it just in case.
you would need to be added to https://flipperui.internal.digitalocean.com/features/vpc_route_enabled
in stage2: https://flipperui.s2r1.internal.digitalocean.com/features/vpc_route_enabled
There was a problem hiding this comment.
Dual security scopes on create/delete
The specs for creating and deleting subnets accept either vpc:create/vpc:delete OR vpc:update. That means a token scoped strictly to vpc:update can completely create or wipe out subnets.
If the idea is that subnets just inherit permissions from their parent VPC, that's a totally fair product call. I just wanted to point it out to confirm it's intentional, since the rest of the PR sticks pretty strictly to one scope per verb.
that's intended and analog to what we do on dns api with records permissions.
0282946 to
3011993
Compare
3011993 to
3b27506
Compare
add api documentations for vpc subnets and routes