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

Enable overriding services for local dev with managed config in Apollo Gateway #4245

Closed
wants to merge 13 commits into from

Conversation

michael-watson
Copy link
Contributor

This PR enables developers to start up an ApolloGateway instance with managed federation and override services in the managed configuration to point a local dev environments.

To enable overriding a single service, the Apollo Gateway would respond to three environment variables being set:

  • APOLLO_SERVICE_OVERRIDE=true
  • APOLLO_SERVICE_OVERRIDE_NAME - name of the service to change url of
  • APOLLO_SERVICE_OVERRIDE_URL - local dev environment url

To enable override a list of services, the user would define a local json file that contains the list of ServiceDefinitions:

  • APOLLO_SERVICE_OVERRIDE=localOverride.json - assumes root directory, but can provide absolute path
    localOverride.json
{
  "servicesToOverride": [
    {
      "name": "accounts",
      "url": "http://localhost:4001"
    }
  ]
}

Pros of current implementation

  • local developers don't need to change code of gateway
  • can clone gateway repo or startup gateway artifact with environment variables set
  • local json file can be put into .gitignore

Cons

  • Another config file to manage, I suppose this could be put into the apollo.config.js as well. It was just quicker/easier for me to do any json file

Other Options

I think there are also other patterns that can be considered here instead of a local configuration file. we could simply do a comma separated list and stick to only two environment variables (APOLLO_SERVICE_OVERRIDE_NAME and APOLLO_SERVICE_OVERRIDE_URL). There could also be the option to put this configuration into the engine configuration options.

I haven't created an entry in the changelog yet because I wanted to wait until we landed on exactly how we want this to look. I like this option, but I'm biased.

@tizmagik
Copy link

This LGTM! Should there be any indication/warning message when an override is specified for a service that doesn't currently exist in managed mode? Or does that enable folks to add additional services locally?

Copy link
Contributor

@jhampton jhampton left a comment

Choose a reason for hiding this comment

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

I like the functionality a LOT, and this is fantastic, thank you @michael-watson .

Environment Vars vs Configuration

My main concern is the addition of specific environment variables. If developers want to use environment variables to add configuration, I think we should enable that without adding new env vars that we'll need to maintain moving forward. I have some principled opinions on why we should prefer explicit configuration over implicit env vars here:

  • Environment vars are essentially global "hidden" state – it's more difficult to know they're there
  • Most IDE's wouldn't surface the env vars at all - there's no autocomplete, type checking, etc
  • We will need to update documentation, and it's easy to break or miss env vars during renames (see engine vs apollo vs studio).
  • Configuration is highly visible, declarative, and explicit. Env vars are invisible, an injected dependency, and implicit
  • If you need to inject information using env vars, that's fine: inject your own ENV vars and then use them in your configuration. But I believe our API here should be an explicit configuration.

I would rather see the addition of serviceOverrides as a config option, and then adding documentation that demonstrates using this (and using ENV vars).

NODE_ENV

We can also separate the conversation about whether this should be allowed in a given node_env, and place that decision completely in the hands of the engineers using this library. If using the serviceOverrides is a configuration concern, we can avoid entire branches of problems that could result from debugging env vars (including incorrect NODE_ENV, MISNAMED VARS, ETC).

I believe that if we are explicit, consistent, and declarative in our approaches, our customers will be confident that they can follow a set of patterns and be rewarded with consistency and stability.

$0.02

Happy to discuss it further. I won't block this PR, but I'd like to have these conversations if possible.

@tizmagik
Copy link

Good points @jhampton ! +1 to explicit configuration over this collection of ENV vars 👍

@michael-watson
Copy link
Contributor Author

@tizmagik There is a console log when the service name provided can't be found in the service list, but now you bring up a great point. I think the developer should be able to add an additional service and we just log details to the console that the service was added to the configuration. I added this functionality in the latest commit.

@jhampton I get what you're saying and I added the hard coded config option. For anyone seeing this that might be wanting to just set environment variables in a docker image of your gateway, or just cloning the repo, here is a code snippet of what you can do:

let shouldOverrideServices = process.env.APOLLO_SERVICE_OVERRIDE;
let serviceOverrides = shouldOverrideServices ? setupServiceOverrides() : [];

function setupServiceOverrides() {
    let overrides = [];
    if (shouldOverrideServices == 'true') {
        overrides.push({ name: process.env.APOLLO_SERVICE_OVERRIDE_NAME, url: process.env.APOLLO_SERVICE_OVERRIDE_URL });
    } else {
        let localOverrideConfigFile = JSON.parse(readFileSync(resolve(__dirname, shouldOverrideServices), { encoding: "utf8" }));
        localOverrideConfigFile.servicesToOverride.map(serviceOverride => overrides.push({ ...serviceOverride }))
    }

    return overrides;
}

const gateway = new ApolloGateway({ debug: true, serviceOverrides });

@michael-watson michael-watson requested a review from jhampton June 19, 2020 03:53
Base automatically changed from master to main June 24, 2020 18:18
@jhampton
Copy link
Contributor

Can I assume this is deprecated in favor of a new approach @michael-watson ?

@abernix
Copy link
Member

abernix commented Sep 16, 2020

Hi @michael-watson!,

This is a semi-automated message. Apologies for the lack of personal detail. I’d encourage reading the rest of this message but, if you're busy, hopefully this is as easy for you — or someone who is willing to own the contribution — as pushing the button later in this message to re-create this PR, from an auto-migrated branch, on the new Federation repository. If you have time, read on!

We’re in the process of moving federation-related concerns out of this repository (where it has historically lived) into a new home specifically for Apollo Federation. This PR is affected by the transition since it touches code which has been moved.

I’ve done some preparations to make this as easy as possible, but we’ll need to close this PR, and I could use your help in re-opening it on the new Apollo Federation repository.

In apollographql/federation#134 (which has a lot more details, if you’re interested), we introduced the same code that was in this repository to the new Federation repository (maintaining its history) and removed the code from this repository in #4561.

While this PR still needs to get reviewed and approved to land, it should no longer live in this repository in its current state since it cannot merge in anymore. To facilitate the movement of this PR to the new home, I’ve automatically generated branches on the new repository using the same commit authorship and messages that you originally included on this PR.

Pull-requests can’t be moved on GitHub in the same way Issues can be. While I could just create the PRs from these new branches too, the contribution belongs to you!


Original PR author only: Click this button to open a new PR from the auto-created apollo-server-import/pr-4245 branch on the new Apollo Federation repository

Original PR author: Click here to re-create this PR on the Federation repository

(The code and your commits should be the same and you will have an opportunity to confirm, but this way you can continue to be the author and track its progress on the new Federation repository!)


There’s no easy way to bring over any existing comments on this PR, so I encourage you to copy and paste those onto the new issue if possible.

Overall, I hope this process is relatively easy for you while maintaining your commit contribution authorship. I apologize for any inconvenience caused by this shuffle, but please ping me if I can help in any way!

🚀

@abernix abernix closed this Sep 16, 2020
@abernix abernix deleted the michael-watson/override-managed-service branch September 21, 2020 11:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants