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

Variable substitution in DataSource JSON files #2556

Closed
8 tasks
bajtos opened this issue Mar 8, 2019 · 12 comments
Closed
8 tasks

Variable substitution in DataSource JSON files #2556

bajtos opened this issue Mar 8, 2019 · 12 comments

Comments

@bajtos
Copy link
Member

bajtos commented Mar 8, 2019

To support https://12factor.net/ application loading their database connection strings from ENV variables, we should enhance our DataSource booter to provide variable substitution similar to what https://github.com/strongloop/loopback-boot already provides for LB3 applications.

Example LB3 config file:

// server/datasources.json
{
  "db": {
    "connector": "mysql",
    "url": "${MYSQL_URL}",
  }
}

Proposed format for LB4:

// src/datasources/db.json
{
  "name": "db",
  "connector": "mysql",
  "url": "${ENV.MYSQL_URL}",
}

By using ENV. prefix, we are keeping doors open to add other variable sources in the future, e.g. APP. prefix to access application-level bindings.

To support numeric values, allow the following format (notice the + sign after the opening bracket):

{
  "port": "${+ENV.PORT}",
}

Acceptance criteria

  • A new helper function accepting configuration object loaded from the JSON file, an object with values to use for substitutions (conceptually {ENV: process.env}), and returning modified configuration object with substitutions resolved according to rules described above. Include comprehensive test coverage.
  • Substitution should be applied to values at all levels, including deeply nested properties.
  • Support conversion to numeric values as described above.
  • @loopback/boot recognizes and replaces variables in datasource configuration. Leverage the already implemented helper function.
  • process.env must not be accessed directly from individual booters. Instead, the environment object should be provided via app.boot() arguments. For example, we can add a new property to BootExecutionOptions.
  • Modify examples and CLI templates to pass process.env value to app.boot(), so that variable substitution can be performed.
  • Documentation
  • Blog post

Out of scope

@marioestradarosa
Copy link
Contributor

marioestradarosa commented Mar 12, 2019

@bajtos , I was analyzing the datasource booter and found that this can be done before it calls the repositoryMixin.dataSource easily. I am suggesting that we use a convention based on the datasource name, without specifying any variable in the .json file.

// src/datasources/db.json
{
  "name": "myDBName",
  "connector": "mysql",
  "host": "xxxx",
  "port": "yyyy",
  "user": "xxxxxkkd",
...
}

Then in the booter it will evaluate ENV.MYDBNAME_USER , if null then it will use the "xxxxxkkd" property value already there, otherwise it will use the value specified by ENV.MYDBNAME_USER.

thoughts?

@emonddr
Copy link
Contributor

emonddr commented Apr 11, 2019

we need a JSON utility that can perform dependency injection post boot.

@emonddr
Copy link
Contributor

emonddr commented Apr 11, 2019

The team brought up a few questions:

  1. How to declare a variable for non-string values. for example PORT NUMBER.? (Can't use string)
  2. How do we specify a default value?
  3. Impossible to add a comment in the file.

We figure we may need a file format different from .json. Perhaps .config.

Please clarify the Acceptance Criteria in light of our comments. Thank you.

@emonddr
Copy link
Contributor

emonddr commented Apr 11, 2019

@bajtos ^

@bajtos
Copy link
Member Author

bajtos commented Apr 18, 2019

@marioestradarosa Thank you for chiming in!

I was analyzing the datasource booter and found that this can be done before it calls the repositoryMixin.dataSource easily.

Yes, that was my roughly my idea too.

I am suggesting that we use a convention based on the datasource name, without specifying any variable in the .json file.

Then in the booter it will evaluate ENV.MYDBNAME_USER , if null then it will use the "xxxxxkkd" property value already there, otherwise it will use the value specified by ENV.MYDBNAME_USER.

I see this as an alternative/complementary solution to what I have originally proposed.

In many environments, the application developer does not control the environment variables. For example, Heroku uses DATABASE_URL environment variable - see https://devcenter.heroku.com/articles/heroku-postgresql#connecting-in-node-js.

How to declare a variable for non-string values. for example PORT NUMBER.? (Can't use string)

Good question! I am proposing to use the following syntax for numeric values (notice the + sign after the opening bracket):

// a number
"port": "${+ENV.PORT}",
// a string
"host": "${ENV.HOST}",

Compare this to regular javascript:

const port = +'0'; 
const host = 'localhost';

How do we specify a default value?

That's out of scope of this story. When the environment value is not set, then the config property is set to undefined.

Impossible to add a comment in the file.
We figure we may need a file format different from .json. Perhaps .config.

That's out of scope of this story. Please open a new issue to discuss this idea.

@bajtos
Copy link
Member Author

bajtos commented Apr 18, 2019

Updated the acceptance criteria:

  • A new helper function accepting configuration object loaded from the JSON file, an object with values to use for substitutions (conceptually {ENV: process.env}), and returning modified configuration object with substitutions resolved according to rules described above. Include comprehensive test coverage.
  • Substitution should be applied to values at all levels, including deeply nested properties.
  • Support conversion to numeric values as described above.
  • @loopback/boot recognizes and replaces variables in datasource configuration. Leverage the already implemented helper function.
  • process.env must not be accessed directly from individual booters. Instead, the environment object should be provided via app.boot() arguments. For example, we can add a new property to BootExecutionOptions.
  • Modify examples and CLI templates to pass process.env value to app.boot(), so that variable substitution can be performed.
  • Documentation
  • Blog post

@bajtos
Copy link
Member Author

bajtos commented Apr 18, 2019

Hmm, I was looking at the current implementation of datasource booter and AFAICT, we are not processing datasource JSON files at boot time right now. We rely on the TypeScript source to load the configuration from the .json file via import/require statement.

As I was envisioning this story, I wanted only a small tweak in the datasource booter to perform variable substitution on the configuration data loaded from JSON before the config is bound to context. That's obviously not possible in the current architecture.

In that light, I think this story is not ready to be worked on and I'll need to think a bit more about how to approach the problem of modifying datasource config in production.

@bajtos bajtos self-assigned this Apr 18, 2019
@dhmlau dhmlau added 2019Q4 and removed 2019Q3 labels Jun 25, 2019
@amites
Copy link

amites commented Jul 23, 2019

Getting into the LB4 ecosystem

Wanted to point out that loading everything from the environment is also not ideal -- lot's of ways that environment values can be leaked -- ideal would also support loading from some kind of vault into an object or similar holder and then used async -- working on a hacked out approach to doing this now

happy to share some snippets once I get a working solution

@bajtos
Copy link
Member Author

bajtos commented Jul 30, 2019

Wanted to point out that loading everything from the environment is also not ideal -- lot's of ways that environment values can be leaked -- ideal would also support loading from some kind of vault into an object or similar holder and then used async -- working on a hacked out approach to doing this now

Good point.

I am not very familiar with Kubernetes, Istio and similar tools, so pardon me if my question is stupid. But isn't it the responsibility of the container orchestration tool to fetch the configuration from a valt into environment variables before starting the process?

Having said that, if it makes sense for the app to load the config from the vault directly, and we can find an elegant solution, then I am fine with implementing such feature ✌️

@amites
Copy link

amites commented Jul 30, 2019

Pulling directly from a vault is also likely not ideal since there are so many to choose from, this would be more appropriate to hand off to a plugin or easy hook method.

My recommendation would be to using a cascade method with an easy method of providing an callback

ie:

  • load defaults
  • merge with values from a known file ex: .env
  • merge with values from system environment (yeah, not ideal but some systems only give you this method)
  • merge with values from an optional async callback to load a config object

make sure the config structure is well documented for the different locations it will look for

make sense?

@bajtos
Copy link
Member Author

bajtos commented Mar 9, 2020

Recently, I am leaning towards a different approach. I think we should remove .datasource.json files, because all other artifact types (models, repositories, controllers) are already using TypeScript instead of JSON.

The idea is to embed the default datasource configuration directly inside .datasource.ts files, for example:

import {inject} from '@loopback/core';
import {juggler} from '@loopback/repository';

export const CONFIG = {
  name: 'db',
  connector: 'memory',
  localStorage: '',
  file: './data/db.json',
};

export class DbDataSource extends juggler.DataSource {
  static dataSourceName = 'db';

  constructor(
    @inject('datasources.config.db', {optional: true})
    dsConfig: object = CONFIG,
  ) {
    super(dsConfig);
  }
}

With this new design in place, variable substitution does not require any special support from LoopBack.

export const CONFIG = {
  name: 'db',
  connector: 'mysql',
  // in production
  url: process.env.MYSQL_CONNECTION_STRING,
  // local dev & test
  database: 'myapp',
};

Obviously, this does not solve the requirement that process.env should not be accessed so deeply from inside the app, but it allows us to quickly enable 12factor deployments while buying us more time to research a better design where environment-specific config can be injected from outside the application.

@bajtos bajtos added the CloudNative Cloud native enablement label Mar 9, 2020
@bajtos
Copy link
Member Author

bajtos commented Mar 31, 2020

The idea is to embed the default datasource configuration directly inside .datasource.ts files

See #5000.

I am closing this issue in favor of #5000 and #1464.

The current solution for 12factor env-based configuration is described in #1464 (comment):

If you are using @loopback/boot to load your datasources (as is the default in LB4 applications scaffolded using lb4 CLI tool), then you can bind just the custom datasource configuration.

this.bind('datasources.config.db').to({
  name: 'db',    
  connector: 'mysql',
  hostname: process.env.DB_HOST,
  port: process.env.DB_PORT,
  user: process.env.DB_USER,
  password: process.env.DB_PASSWORD,
  database: process.env.DB_DATABASE
});

@bajtos bajtos closed this as completed Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants