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

Allow configuration of multiple Uplink URLs #1283

Merged

Conversation

snoopdave
Copy link
Contributor

@snoopdave snoopdave commented Dec 9, 2021

This PR adds two new configuration fields to the existing ManagedGatewayConfig interface:

  uplinkEndpoints?: string[];
  uplinkMaxRetries?: number;

If uplinkEndpoints is not set and there is an existing setting for schemaDeliveryMaxRetries (deprecated) it will be honored.

Also, the existing env variable APOLLO_SCHEMA_CONFIG_DELIVERY_ENDPOINT is now treated as a comma-separated list of Uplink URLS.

uplinkMaxRetries will default to three times the number of entries in the end-points array.

If you specify more than one Uplink URL, the loadSupergraphSdlFromUplinks() logic will round-robin retry and will return the first result it receives.

@snoopdave snoopdave changed the title WIP: support for multi-cloud Uplink Allow configuration of multiple Uplink URLs Dec 13, 2021
Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

I went digging a bit and turned up some issues with our nock usage in loadSupergraphSdlFromStorage.test.ts. I pushed up a fix and landed via #1309, so you'll probably need to rebase.

My nock comments may be affected by the changes I landed, but happy to discuss tomorrow.

gateway-js/src/__tests__/integration/configuration.test.ts Outdated Show resolved Hide resolved
gateway-js/src/__tests__/integration/configuration.test.ts Outdated Show resolved Hide resolved
gateway-js/src/config.ts Outdated Show resolved Hide resolved
gateway-js/src/__tests__/integration/nockMocks.ts Outdated Show resolved Hide resolved
gateway-js/src/__tests__/integration/nockMocks.ts Outdated Show resolved Hide resolved
gateway-js/src/index.ts Outdated Show resolved Hide resolved
Dave Johnson and others added 5 commits December 15, 2021 13:27
# Conflicts:
#	gateway-js/src/__tests__/loadSupergraphSdlFromStorage.test.ts
#	gateway-js/src/index.ts
Use const instead of var

Co-authored-by: Trevor Scheer <[email protected]>
…opdave/federation into snoopdave/multi-cloud-uplink

# Conflicts:
#	gateway-js/src/index.ts
@snoopdave
Copy link
Contributor Author

I downloaded the Gateway package from CircleCI and tested it locally with the original (GCP powered) Uplink URL, with the new AWS URL and with both. I added some console logging and verified that both URLs work and Gateway will alternate between them.

gateway-js/src/index.ts Outdated Show resolved Hide resolved
gateway-js/src/loadSupergraphSdlFromStorage.ts Outdated Show resolved Hide resolved
gateway-js/src/loadSupergraphSdlFromStorage.ts Outdated Show resolved Hide resolved
gateway-js/src/loadSupergraphSdlFromStorage.ts Outdated Show resolved Hide resolved
gateway-js/src/loadSupergraphSdlFromStorage.ts Outdated Show resolved Hide resolved
snoopdave and others added 3 commits December 16, 2021 09:22
Co-authored-by: Trevor Scheer <[email protected]>
…opdave/federation into snoopdave/multi-cloud-uplink

# Conflicts:
#	gateway-js/src/index.ts
#	gateway-js/src/loadSupergraphSdlFromStorage.ts
@snoopdave
Copy link
Contributor Author

thanks for the awesome reviews @trevor-scheer. I think I have hit addressed of your concerns and suggestions.

Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

LGTM! Just one comment about some stray whitespace changes. This needs a changelog entry in the gateway-js/CHANGELOG.md file. It should explain how the API's been updated and the deprecation of the old option.

gateway-js/src/__tests__/integration/configuration.test.ts Outdated Show resolved Hide resolved
@snoopdave
Copy link
Contributor Author

@trevor-scheer OK, we should be good to go on this. I downloaded the latest build from CI, tested it with the Space Camp Federation demo and verified that both Uplink URLs are being used.

@glasser
Copy link
Member

glasser commented Dec 17, 2021

@snoopdave I just want to make sure that updating https://www.apollographql.com/docs/studio/data-privacy/ is on your radar. This is where we list all URLs that any of our software connects to. The source is at https://github.com/apollographql/apollo/blob/main/studio-docs/source/data-privacy.md

@snoopdave
Copy link
Contributor Author

@snoopdave I just want to make sure that updating https://www.apollographql.com/docs/studio/data-privacy/ is on your radar. This is where we list all URLs that any of our software connects to. The source is at https://github.com/apollographql/apollo/blob/main/studio-docs/source/data-privacy.md

That was not on my radar but it is now. Thanks

Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

🎉

@trevor-scheer trevor-scheer merged commit 873b79c into apollographql:main Dec 17, 2021
trevor-scheer pushed a commit that referenced this pull request Dec 17, 2021
This commit adds two new configuration fields to the existing `ManagedGatewayConfig` interface:
* `uplinkEndpoints?: string[];`
* `uplinkMaxRetries?: number;`

If `uplinkEndpoints` is not set and there is an existing setting for `schemaDeliveryMaxRetries`
(deprecated) it will be honored.

Also, the existing env variable `APOLLO_SCHEMA_CONFIG_DELIVERY_ENDPOINT` is now treated
as a comma-separated list of Uplink URLS.

`uplinkMaxRetries` will default to three times the number of entries in the end-points array.

If you specify more than one Uplink URL, the `loadSupergraphSdlFromUplinks()` logic will round-robin
retry and will return the first result it receives.
trevor-scheer added a commit that referenced this pull request Dec 17, 2021
This commit adds two new configuration fields to the existing `ManagedGatewayConfig` interface:
* `uplinkEndpoints?: string[];`
* `uplinkMaxRetries?: number;`

If `uplinkEndpoints` is not set and there is an existing setting for `schemaDeliveryMaxRetries`
(deprecated) it will be honored.

Also, the existing env variable `APOLLO_SCHEMA_CONFIG_DELIVERY_ENDPOINT` is now treated
as a comma-separated list of Uplink URLS.

`uplinkMaxRetries` will default to three times the number of entries in the end-points array.

If you specify more than one Uplink URL, the `loadSupergraphSdlFromUplinks()` logic will round-robin
retry and will return the first result it receives.

Co-authored-by: David M. Johnson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants