Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connector-builder server stub #18410

Merged
merged 87 commits into from
Nov 9, 2022
Merged

Connector-builder server stub #18410

merged 87 commits into from
Nov 9, 2022

Conversation

girarda
Copy link
Contributor

@girarda girarda commented Oct 25, 2022

What

Add the skeleton for the connector-builder server without including it the the deploys.

The connector-builder server will server as a bridge between the connector builder UI and the CDK.

The updates to the deploy files were commented out so we can minimize the drift between master and our dev branch without impacting the users.
The module is added to gradle so we still build it.

The server is a FastAPI server running on port 8003 exposing the following endpoints:

  • "/"
  • "/items/{item_id}"

A follow up PR includes server generation for the connector-builder server.

How

  • Create airbyte-connector-builder module which contains a Dockerfile running a FastAPI dummy server
  • Move the openAPI spec file to the airbyte-connector-builder module (and update the webapp to point to it)
  • Add airbyte-connector-builder to the docker-compose and kube charts (commented out)
  • Install python on the CI runners

Recommended reading order

connector-builder

  1. airbyte-connector-builder/connector_builder/entrypoint.py
  2. airbyte-connector-builder/build.gradle
  3. settings.gradle
  4. airbyte-connector-builder/Dockerfile
  5. airbyte-connector-builder/setup.py
  6. airbyte-connector-builder/src/main/openapi/openapi.yaml
  7. airbyte-webapp/orval.config.ts
  8. settings.gradle
  9. .github/workflows/gradle.yml
  10. .bumpversion.cfg
  11. . tools/bin/publish_docker.sh

deployment

  1. .env
  2. .env.dev
  3. docker-compose.yaml
  4. docker-compose.build.yaml
  5. kube/resources/connector-builder.yaml
  6. kube/overlays/dev-integration-test/kustomization.yaml
  7. kube/overlays/dev/kustomization.yaml
  8. kube/overlays/stable-with-resource-limits/kustomization.yaml
  9. kube/overlays/stable-with-resource-limits/set-resource-limits.yaml
  10. kube/overlays/stable/kustomization.yaml
  11. kube/resources/kustomization.yaml

webapp

  1. airbyte-webapp/.env
  2. airbyte-webapp/nginx/default.conf.template
  3. airbyte-webapp/src/config/configProviders.ts
  4. airbyte-webapp/src/config/defaultConfig.ts

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the connector is published, connector added to connector index as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here
Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here
Connector Generator
  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

@girarda girarda temporarily deployed to more-secrets November 4, 2022 04:25 Inactive
.bumpversion.cfg Outdated
@@ -61,3 +61,11 @@ serialize =
[bumpversion:file:octavia-cli/install.sh]

[bumpversion:file:octavia-cli/setup.py]

[bumpversion:file:connector-builder/Dockerfile]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add the module to so its version gets bumped with the others

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean that to get a connector builder update, I have to upgrade my whole airbyte instance? in that case we should probably recommend that people develop lowcode connectors in a separate instance from their prod one. Just a note for the docs in the future



task generateApiServer(type: GenerateTask) {
inputSpec = "$rootDir.absolutePath/airbyte-connector-builder/src/main/openapi/openapi.yaml"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the new path to the openAPI spec



@app.get("/")
def read_root():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a dummy server. the openAPI template will come in a follow up PR

@@ -23,7 +23,7 @@ export default defineConfig({
},
},
connectorBuilder: {
input: "../connector-builder-server/src/main/openapi/openapi.yaml",
input: "../airbyte-connector-builder/src/main/openapi/openapi.yaml",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved the openapi spec to the airbyte-connector-builder module

@@ -5,7 +5,7 @@ const defaultConfig: Config = {
healthCheckInterval: 20000,
version: "dev",
apiUrl: `${window.location.protocol}//${window.location.hostname}:8001/api`,
connectorBuilderApiUrl: `${window.location.protocol}//${window.location.hostname}:8080/`,
connectorBuilderApiUrl: `${window.location.protocol}//${window.location.hostname}:8003/`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed the port to 8003 as discussed

@@ -29,6 +29,13 @@ services:
context: airbyte-workers
labels:
io.airbyte.git-revision: ${GIT_REVISION}
#connector-builder:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(commented out)
this adds the connector-builder server to the docker-compose deploy

# container_name: airbyte-connector-builder
# restart: unless-stopped
# ports:
# - 8003
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

port is not exposed to localhost

airbyte-proxy:
image: airbyte/proxy:${VERSION}
container_name: airbyte-proxy
ports:
- 8000:8000
- 8001:8001
#- 8003:8003 FIXME: Uncomment this when enabling airbyte-connector-builder
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only expose the server through airbyte-proxy

Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will add back the commented out parts in #18886


RUN pip install --no-cache-dir .

ENTRYPOINT ["uvicorn", "connector_builder.entrypoint:app", "--host", "0.0.0.0", "--port", "80"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@girarda do you remember why this is explicitly specified as 0.0.0.0? 127.0.0.1 didn't work for me, was curious if you understood why

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure why 127.0.0.1 does not work. the doc says

Use --host 0.0.0.0 to make the application available on your local network

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

127.0.0.1 is the loopback address which will only take traffic from the same host, if you need any to listen to traffic coming from outside the container use 0.0.0.0 (means effectively "from anywhere")

.bumpversion.cfg Outdated
@@ -61,3 +61,11 @@ serialize =
[bumpversion:file:octavia-cli/install.sh]

[bumpversion:file:octavia-cli/setup.py]

[bumpversion:file:connector-builder/Dockerfile]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean that to get a connector builder update, I have to upgrade my whole airbyte instance? in that case we should probably recommend that people develop lowcode connectors in a separate instance from their prod one. Just a note for the docs in the future

@girarda
Copy link
Contributor Author

girarda commented Nov 4, 2022

@sherifnada

to get a connector builder update, I have to upgrade my whole airbyte instance?
Yes, this is true for the FE too

Comment on lines 24 to 25
#- name: airbyte/connector-builder #FIXME: Uncomment this block when enabling airbyte-connector-builder
# newTag: 0.40.18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is not a huge lift to also add this server to our airbyte-cloud deployment, it would be pretty nice to do that so people could just access the Connector Builder through a URL instead of having to spin up Airbyte themselves.

Alternatively, I believe the demo.airbyte.io instance is actually an OSS deployment that is just made publicly accessible. So people could access the Connector Builder there too, which has the added benefit of not requiring any authentication to use

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are a few open questions to answer like whether we need to enforce rate limits to protect the backend against a bad user, but using the demo instance is a good idea for making the connector public without putting prod at risk

.env.dev Outdated
@@ -23,6 +23,7 @@ HACK_LOCAL_ROOT_PARENT=/tmp
WEBAPP_URL=http://localhost:8000/
API_URL=/api/v1/
INTERNAL_API_HOST=airbyte-server:8001
#CONNECTOR_BUILDER_API_HOST=airbyte-connector-builder:8003 #FIXME: Uncomment this when enabling the connector-builder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is commented out for now because this is stubbed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. I doubt there would be any harm to exposing the env variable, but we can just turn this on at the same time as we enable the server

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a short conversation with Sherif offline and although I understand the reasons for choosing to host the Connector Builder UI in the Webapp, I think we now have leaked this complexity into the rest of the deploy tooling.

We should properly decide how to handle this sooner rather than later to reduce both product and technical complexity.

@davinchia
Copy link
Contributor

@airbytehq/production-engineering can someone take a quick look here?

I'm tagging someone from the team because I think it's useful for the PE to understand bits of the OSS deploy stack for future work.

Here, what we are doing is:

  1. setting the stage so the connector builder server can be part of the general Airbyte deployment, OSS and Kube.
  2. the connector builder server is the backend for the low-code cdk UI - this is the low-code framework that allows devs to build apis without writing code.
  3. ideally this is decoupled from airbyte - i.e. a low-code dev doesn't have to deploy Airbyte to use the low-code cdk. However, the FE doesn't really support this today, and we've made the choice to embed the low-code UI with the current Webapp for velocity sake.
  4. In the long term, we will remove this. In the short term, the server will be deployed with Airbyte to unblock us for now.

@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2020 Airbyte, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/2020/2022/

@girarda girarda temporarily deployed to more-secrets November 9, 2022 06:50 Inactive
@girarda girarda temporarily deployed to more-secrets November 9, 2022 15:29 Inactive
@@ -209,12 +210,24 @@ services:
- workspace:${WORKSPACE_ROOT}
networks:
- airbyte_internal
#airbyte-connector-builder: #FIXME: Uncomment this block when enabling airbyte-connector-builder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@girarda I wonder if we should call this airbyte-connector-builder-server, to make it clear that this is only the backing server for the connector builder, and not the connector builder webapp?

Copy link
Contributor Author

@girarda girarda Nov 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lmossman do you think we should rename the module too?

update: I renamed the module too for consistency

@girarda girarda temporarily deployed to more-secrets November 9, 2022 19:29 Inactive
@girarda girarda temporarily deployed to more-secrets November 9, 2022 19:38 Inactive
@girarda girarda temporarily deployed to more-secrets November 9, 2022 20:22 Inactive
@girarda girarda merged commit 64b50ad into master Nov 9, 2022
@girarda girarda deleted the alex/connector_builder_fastapi branch November 9, 2022 21:07
akashkulk pushed a commit that referenced this pull request Dec 2, 2022
* init

* bad copy/paste

* move to top level

* Revert "move to top level"

This reverts commit aca3534.

* attempt to wire up connector builder frontend to server

* copy from octaviacli

* fix connection to builder server

* update

* delete

* Update

* delete python-version

* Revert "delete python-version"

This reverts commit f9258a7.

* setup python

* install python

* rename

* kube stuff

* Install python

* missing kube file

* rename

* Update files

* Update bumpversion

* install python

* try with different entrypoint

* rename container

* point to docker-compose.yaml file

* derp

* copy acceptance_test.sh

* copy from acceptance tests

* delete cruft

* update

* remove application env

* reset

* reset to master

* update

* skip comprehensive incremental tests

* Revert "skip comprehensive incremental tests"

This reverts commit 9cee657.

* reset to master

* remove cruft

* delete superfluous steps

* update port to 8003

* reset to master

* Update publish docker

* move openapi spec to airbyte-connector-builder

* point to openapi spec

* dont expose the connector builder to localhost

* reset FE components to master

* Don't deploy the connector-builder

* Revert "Don't deploy the connector-builder"

This reverts commit 3d15749.

* Revert "Revert "Don't deploy the connector-builder""

This reverts commit beac3d4.

* comment out more things related to connector builder server

* more attempts at removing the connector builder

* comment out more things

* Apply suggestions from code review

Co-authored-by: Lake Mossman <lake@airbyte.io>

* Update airbyte-webapp/src/config/configProviders.ts

Co-authored-by: Lake Mossman <lake@airbyte.io>

* update

* rename

* indent

* Revert "move openapi spec to airbyte-connector-builder"

This reverts commit 57dda04.

* Revert "rename"

This reverts commit b2d802b.

* Revert "Revert "rename""

This reverts commit 91db24f.

* point to wrong file in case it fixes the build

* point to right openapi file

* Revert "Revert "move openapi spec to airbyte-connector-builder""

This reverts commit e46a837.

* point to moved file

* fix path

* Update from master

* newline

* Add failing test

* Revert "Add failing test"

This reverts commit ed9fea0.

* comment

* update commented requires

* Add a comment

* 2022

* rename to connector-builder-server

* typo

Co-authored-by: lmossman <lake@airbyte.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants