Define APIs for scoped connector definitions #11244
Conversation
- listSourceDefinitions takes workspaceId - listDestinationDefinitions takes workspaceId
github-actions
bot
added
area/api
- SourceDefinitionCreate takes workspaceId - DestinationDefinitionCreate takes workspaceId
git-phu
force-pushed
the
peter/define-opt-in-apis
branch
from
6464d53
to
4c97d37
Compare
|
@timroes would you be able to assist with making the frontend changes to support passing Happy to learn how to do it myself as well though I'm not familiar with react or typescript. |
| @@ -296,6 +296,11 @@ paths: | |||
| - source_definition | |||
| summary: List all the sourceDefinitions the current Airbyte deployment is configured to use | |||
There was a problem hiding this comment.
This comment should probably be tweaked slightly, maybe 'List all the sourceDefinitions the indicated workspace is configured to use'
There was a problem hiding this comment.
Since we decided not to modify the params of the existing endpoints, when implementing the new endpoint behaviors I will also update the summaries of the existing actor definition endpoints to reflect their new behavior.
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/WorkspaceIdRequestBody" |
There was a problem hiding this comment.
I do wonder if it would make sense to support a listSourceDefinitions route that doesn't require a workspaceId in the request body. Semantically, such a route would mean 'list all the sourceDefinitions that are available by default to the current Airbyte deployment' whereas your modified route semantically means 'list all the sourceDefinitions that are available to the indicated workspace within the current Airbyte deployment'.
If we don't have a use-case for the workspace-agnostic version of the route, maybe we don't worry about it. @cgardens what do you think? Are there situations where we think we'd want to list all of the default, public definitions for the current Airbyte instance without requiring a workspace ID?
There was a problem hiding this comment.
good call. i think it's valuable to have that functionality for us as administrators of cloud. so yeah i think i would keep both, but change the permission so it was only usable by instance admins.
also worth noting that this might make our lives easier for the sake of migration. right now if we merge this PR, the app will break because the UI isn't ready for it. ideally we could merge this PR without having to coordinate with the UI team. i.e.
- make change in API that is backwards compatible
- tell the UI team that the new endpoint exists
- once the UI team has updated the UI, we adjust the old endpoint to have the behavior i mentioned (instance-wide view only for instance admins)
There was a problem hiding this comment.
I like the idea of having this PR only make non-breaking changes.
And similarly to the list apis, I think we could also add new endpoints for creating custom definitions for a specific workspace and leave the existing create endpoints for admins only to create private non-custom definitions (i.e. the definitions that are grantable to workspaces).
Also a slight tangent, but my initial thought for defining these new endpoints with more restful routes would be something like
/v1/workspaces/{workspace_id}/source_definitions/list
/v1/workspaces/{workspace_id}/source_definitions/create
/v1/workspaces/{workspace_id}/source_definitions/{source_definitions}/grant
/v1/workspaces/{workspace_id}/source_definitions/{source_definitions}/revoke
but it looks like we don't actually use restful routes in this app, which is fine but I'm just curious about how we decided on the naming convention we use for routes?
There was a problem hiding this comment.
The description at the top explains WHAT the convention is a bit. In terms of the WHY... it was inspired by how slack does it. If I were to start again, I would just go with REST because the fact that this approach is unfamiliar to most engineers is enough to make it not really worth it. It does iron over some parts of REST that are annoying and I liked the idea of modeling things in a more RPC style.
There was a problem hiding this comment.
Gotcha, I do actually kinda like the current approach and I can see how it's convenient to always have all args come from the post body too. Just takes me a bit more effort to come up with sensible names for the routes 😄
| post: | ||
| tags: | ||
| - source_definition | ||
| summary: Opt in to a non-public, non-custom sourceDefinition |
There was a problem hiding this comment.
I'm unsure on whether or not we actually want to specify 'non-custom' for this route. I could see a scenario where, for instance, an admin logs into somebody's workspace and adds a custom connector definition, verifies that it works, and then wants to add the same custom connector to another workspace. In that case, we might want it to be possible for the instance admin to hit this API endpoint to grant access to the 2nd workspace, instead of forcing them to add it as a duplicate custom connector, or needing to add it to the catalog as a public: false "opt-in" definition.
What do you think? @cgardens also curious about your opinion on this. I think there's potentially some risk in allowing instance admins to create grants for custom connectors that aren't in our catalog, but maybe it's worth it if there's enough utility?
There was a problem hiding this comment.
Yeah i think either could work. I believe @cgardens had mentioned a scenario to me before that we'd want to avoid, which is: we don't want a custom connector that is specifically built for one workspace (possibly even with workspace specific secrets baked into the connector definition) to accidentally appear in other workspaces.
Maybe if we included some indicator of which private connectors were custom or not, that could help alleviate this concern.
There was a problem hiding this comment.
interesting point. i think since we don't have a notion of an "owner" for a connector definition, what peter said about not allowing this for custom connectors makes sense. otherwise if someone got ahold of the definition id for a custom connector that wasn't theirs they could add it to their own workspace. so either for custom connectors we need to introduce some concept of owners (it makes my head hurt to think about how we would actually model it) or we don't allow granting access to custom connectors in the api. i think i'm inclined to stick with the ladder for now and we can build more features into the schema in the future if we need it. lmk if i'm overestimating the definition ownership piece.
There was a problem hiding this comment.
Cool, that makes sense to me! I think we're good to leave this as-is then
- remove use of opt-in - update description of definition list apis
git-phu
force-pushed
the
peter/define-opt-in-apis
branch
from
13ab83c
to
3df0873
Compare
- create new routes instead of making breaking changes to existing ones - existing create/list routes will be repurposed as admin only routes
- create new routes instead of making breaking changes to existing ones - existing create/list routes will be repurposed as admin only routes
- create new routes instead of making breaking changes to existing ones - existing create/list routes will be repurposed as admin only routes
| type: object | ||
| required: | ||
| - workspaceId | ||
| - sourceDefinition |
There was a problem hiding this comment.
not sure if it's preferable to have the post body as specified here, which looks like
{
"workspaceId": "<workspaceId>",
"sourceDefinition": {
"name": "<name>",
"dockerRepository": "<dockerRepository>",
"dockerImageTag": "<dockerImageTag>",
// etc...
}
}
or whether we should flatten it to
{
"workspaceId": "<workspaceId>",
"name": "<name>",
"dockerRepository": "<dockerRepository>",
"dockerImageTag": "<dockerImageTag>",
// etc...
}
thoughts?
There was a problem hiding this comment.
I think I slightly prefer the first, because otherwise it implies that the workspaceId is a first-class property of the resource being created. The separation maps onto the underlying data model, a sourceDefinition that is created and then granted to a particular workspace
|
I've updated this PR to be purely additive, so no existing endpoints have been modified. When implementing the new endpoint behaviors I can also update the summaries of the existing actor definition endpoints to reflect their new behavior. @pmossman should be ready for re-review, thanks! |
| type: object | ||
| required: | ||
| - workspaceId | ||
| - sourceDefinition |
There was a problem hiding this comment.
I think I slightly prefer the first, because otherwise it implies that the workspaceId is a first-class property of the resource being created. The separation maps onto the underlying data model, a sourceDefinition that is created and then granted to a particular workspace
- create new routes instead of making breaking changes to existing ones - existing create/list routes will be repurposed as admin only routes
What
Part of #9652
Tech Spec
This PR is only for defining the new and modified APIs required for supporting scoped connector definitions.
A subsequent PR will actually implement these endpoints.
Recommended reading order
Everything interesting happens in
config.yaml. The rest is mostly auto generated code.Might also be easier to review by commit.
## 🚨 Breaking Changes 🚨This PR in its current state will cause some breaking changes.
This PR now only adds new endpoints.