-
Notifications
You must be signed in to change notification settings - Fork 276
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
Update the Helm Chart to set an AIRBYTE_API_HOST in OSS and Enterprise #310
Conversation
…ken for OSS, so this needs some review and suggestions.
|
@@ -46,7 +46,7 @@ data: | |||
KEYCLOAK_INTERNAL_HOST: localhost # just a placeholder so that nginx template is valid - shouldn't be used when edition isn't "pro" | |||
{{- end }} | |||
CONNECTOR_BUILDER_API_HOST: {{ .Release.Name }}-airbyte-connector-builder-server-svc:{{ index .Values "connector-builder-server" "service" "port" }} | |||
AIRBYTE_API_HOST: {{ .Release.Name }}-airbyte-api-server-svc:{{ index .Values "airbyte-api-server" "service" "port" }} | |||
AIRBYTE_API_HOST: {{ printf "%s/api/public/v1" (index $airbyteYmlDict "webapp-url") | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what happens here is the airbyteYmlDict is not set. This will happen in OSS. Not sure what we should set for this value in OSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok after looking at this more here's the rundown.
When we try to build say a pagination next URL, we take the AIRBYTE_API_HOST
and grab i.e. the connections path (a constant with value /api/public/v1/connections
), remove the /api/public
prefix, and then smash them together to get the URL that appears in the next
param of the response.
I think if we set the AIRBYTE_API_HOST
to http://airbyte-server-svc.cluster.local/api/public
or whatever the equivalent is, we'll still remove the /api/public
off the path, but the API Host will still contain it and we'll be able to correctly direct folks. Should definitely test before we merge, but I think that will work for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to include the v1 at the end of the path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Enterprise, the majority of the traffic will be coming in through an ingress, so we will want the external host name. I am good with defaulting this to the {{ Release.name }}-airbyte-server/api/public though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me. To summarize:
- In Cloud/Enterprise we'll expect an ingress to be present where we'll rewrite the path from something like
api.cloud-customer.com
->airbyte-server/api/public
and have anAIRBYTE_API_HOST
value ofapi.cloud-customer.com
- In OSS (and some Enterprise cases) we'll expect no ingress to be present which will mean all calls will be made directly to
airbyte-server/api/public
and theAIRBYTE_API_HOST
value will beairbyte-server/api/public
.
Does that seem accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good enough for now?
Airbyte Code Coverage
|
closed in favor of https://github.com/airbytehq/airbyte-platform-internal/pull/11729 |
Your branch is not currently up-to-date with |
Use the webapp-url when deploying as Enterprise and the internal service name in OSS.
Can this PR be safely reverted / rolled back?