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

feat(core): ARC-1906 fetch env from ssm #52

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/arc-auth/.env.schema
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
NODE_ENV=
LOG_LEVEL=
DB_HOST=
DB_PORT=
DB_USER=
DB_PASSWORD=
DB_DATABASE=
DB_SCHEMA=
REDIS_HOST=
REDIS_PORT=
REDIS_URL=
REDIS_PASSWORD=
REDIS_DATABASE=
JWT_SECRET=
JWT_ISSUER=
28 changes: 26 additions & 2 deletions packages/arc-auth/cdk/src/common/stacks/lambda.stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ import * as random from '@cdktf/provider-random';
import {ILambdaWithApiGateway, LambdaWithApiGateway} from 'arc-cdk';
import {TerraformStack} from 'cdktf';
import {Construct} from 'constructs';
import {getEnv, getSecurityGroup, getSubnetIds} from '../../env';
import {AwsProvider} from '../constructs/awsProvider';
import path = require('path');

export class LambdaStack extends TerraformStack {
constructor(
scope: Construct,
id: string,
config: Omit<ILambdaWithApiGateway, 'name'>,
config: Omit<ILambdaWithApiGateway, 'name' | 'envVars' | 'namespace' | 'environment'>,
) {
super(scope, id);

Expand All @@ -20,7 +21,7 @@ export class LambdaStack extends TerraformStack {
const pet = new random.pet.Pet(this, 'random-name', {
length: 2,
});

const env = getEnv(this);

Choose a reason for hiding this comment

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

This implementation is still incorrect.

// overwrite codePath based on useImage as deploy via docker needs different codePath
config.codePath = path.resolve(
config.codePath,
Expand All @@ -31,6 +32,29 @@ export class LambdaStack extends TerraformStack {
// NOSONAR
...config,
name: pet.id,
vpcConfig: {
securityGroupIds: getSecurityGroup(this),
subnetIds: getSubnetIds(this),
},
envVars: {
DB_HOST: env.DB_HOST || '',
DB_PORT: env.DB_PORT || '',
DB_USER: env.DB_USER || '',
DB_PASSWORD: env.DB_PASSWORD || '',
DB_DATABASE: env.DB_DATABASE || '',
DB_SCHEMA: env.DB_SCHEMA || '',
JWT_SECRET: env.JWT_SECRET || '',
JWT_ISSUER: 'sourcefuse',
PORT: '3005',
LOG_LEVEL: 'info',
DB_CONNECTOR: 'postgresql',
},
customDomainName: {
domainName: env.DOMAIN_NAME || '',
hostedZoneId: env.HOSTED_ZONE_ID || '',
},
namespace: env.NAMESPACE || '',
environment: env.ENV || '',
});
}
}
19 changes: 17 additions & 2 deletions packages/arc-auth/cdk/src/common/stacks/migration.stack.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import * as random from '@cdktf/provider-random';
import {ILambda, Lambda} from 'arc-cdk';
import {TerraformStack} from 'cdktf';
import {Construct} from 'constructs';
import {ILambda, Lambda} from 'arc-cdk';
import {getEnv, getSecurityGroup, getSubnetIds} from '../../env';
import {AwsProvider} from '../constructs/awsProvider';

export class MigrationStack extends TerraformStack {
constructor(scope: Construct, id: string, config: Omit<ILambda, 'name'>) {
constructor(scope: Construct, id: string, config: Omit<ILambda, 'name' | 'envVars' | 'namespace' | 'environment'>) {
super(scope, id);

new AwsProvider(this, 'aws'); // NOSONAR
Expand All @@ -15,11 +16,25 @@ export class MigrationStack extends TerraformStack {
const pet = new random.pet.Pet(this, 'random-name', {
length: 2,
});
const env = getEnv(this);

new Lambda(this, 'lambda', {
// NOSONAR
...config,
name: pet.id,
vpcConfig: {
securityGroupIds: getSecurityGroup(this),
subnetIds: getSubnetIds(this),
},
envVars: {
DB_HOST: env.DB_HOST || '',
DB_PORT: env.DB_PORT || '',
DB_USER: env.DB_USER || '',
DB_PASSWORD: env.DB_PASSWORD || '',
DB_DATABASE: env.DB_DATABASE || '',
},
namespace: env.NAMESPACE || '',
environment: env.ENV || '',
});
}
}
13 changes: 10 additions & 3 deletions packages/arc-auth/cdk/src/common/stacks/redis.stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as aws from '@cdktf/provider-aws';
import {Fn, TerraformIterator, TerraformStack} from 'cdktf';
import {Construct} from 'constructs';
import {Redis} from '../../.gen/modules/redis';
import {getEnv} from '../../env';
import {AwsProvider} from '../constructs/awsProvider';
import {getResourceName} from '../utils/helper';

Expand All @@ -11,10 +12,16 @@ type Config = {
};

export class RedisStack extends TerraformStack {
constructor(scope: Construct, id: string, config: Config) {
constructor(scope: Construct, id: string) {
super(scope, id);

new AwsProvider(this, 'aws'); // NOSONAR
const env = getEnv(this);
const config: Config = {
// NOSONAR
namespace: env.NAMESPACE || '',
environment: env.ENV || '',
}

const name = getResourceName({
namespace: config.namespace,
Expand Down Expand Up @@ -57,8 +64,8 @@ export class RedisStack extends TerraformStack {
{
name: 'tag:Name',
values: [
`${config.namespace}-${config.environment}-privatesubnet-private-${process.env.AWS_REGION}a`,
`${config.namespace}-${config.environment}-privatesubnet-private-${process.env.AWS_REGION}b`,
`${config.namespace}-${config.environment}-privatesubnet-private-${env.AWS_REGION}a`,
`${config.namespace}-${config.environment}-privatesubnet-private-${env.AWS_REGION}b`,
],
},
{
Expand Down
81 changes: 81 additions & 0 deletions packages/arc-auth/cdk/src/env.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import {DataAwsSecurityGroup} from '@cdktf/provider-aws/lib/data-aws-security-group';
import {DataAwsSsmParameter} from '@cdktf/provider-aws/lib/data-aws-ssm-parameter';
import {DataAwsSubnets} from '@cdktf/provider-aws/lib/data-aws-subnets';
import {TerraformStack} from 'cdktf';
import {readFileSync} from 'fs';

export const env = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to add properties here also?
This would be duplication of effort for the user whenever user adds a new property. User will have to add that property here also.
We can just pick the properties from the .env. Whatever properties are there we can fetch from ssm and feed it into lambda env variables.
cc: @jamescrowley321

Choose a reason for hiding this comment

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

These are all params that should be loaded from SSM.

The idea is that the env file is a list of SSM param keys, we query SSM for those keys at runtime, and then we pass the values from SSM to the CDK where needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will be maintainig all our env variables here only. not in .env.
we can bring our values from .env , but in thatcase we can not use process.env for that. either we have to use fs methods or something else. because process.env brings in many other variables too. And using fs methods with .env, it does not seems usual, i also went through some workaround but didn't get the funcitonal way. that's why i followed this approach.

storing all our env inside an object makes them accessible easily, we don't have to add any further logic to separate our required env from the system env variables.

this is one approcah, and may be its possible to do in some better way

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's discuss it tomorrow @Tyagi-Sunny

Copy link
Contributor

@Yogi-Vishwas Yogi-Vishwas Feb 1, 2024

Choose a reason for hiding this comment

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

@jamescrowley321 so we can easily assume that the properties that user wants to fetch from SSM. User will have to specify those beforehand.
So we can create an array propertiesToFetch and user have to add properties there
whatever properties user will add we'll fetch it from ssm.
and all other non-sensitive values can be picked from .env

Choose a reason for hiding this comment

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

Here is what I recommend

  • ENV file is a list of values like DB_HOST_SSM_PARAM_KEY=whatever/the/key/value/is
  • Create a configuration type, i.e. AuthServiceConfiguration
  • Create a type such as AuthServiceConfigurationLoader (or w/e you want to name it) and a concrete implementation that encapsulates reading the ENV variables and looking up the values from SSM to return an instance of AuthServiceConfiguration.
  • Only reference AuthServiceConfiguration
  • Inject AuthServiceConfigurationLoader where you can
  • Then the devs can also use that interface to create a Mock/Stub/Local version of the AuthServiceConfigurationLoader to read from other param stores
  • We may not always use SSM to load the variables, so decouple things where you can.

Choose a reason for hiding this comment

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

Why cant we do all of this in Code Pipeline ? Ideally we do these things in code pipeline or jenkins or whatever tool we use for CI/CD. So that application code remains unaware of tool to use. It just accesses everything from env. CI/CD pipeline scripts read values from SSM or Vault and put into the env while deploying the service.

AWS_REGION: "",
DB_HOST: "",
DB_PORT: 5432,
DB_USER: "",
DB_PASSWORD: "",
DB_DATABASE: "",
DB_SCHEMA: "",
JWT_SECRET: "",
ACM_CERTIFICATE_ARN: "",
HOSTED_ZONE_ID: "",
DOMAIN_NAME: "",
NAMESPACE: "",
ENV: "",
S3_BUCKET: ""
};

interface EnvVar {
[key: string]: string;
}

export const getSubnetIds = (scope: TerraformStack) => {
const subnets = new DataAwsSubnets(scope, "private_subnets", {
filter: [
{
name: "tag:Name",
values: ['demoTagName'], //Replace demoTagName by Name Tag of subnet id
},
],
});
return subnets.ids;
}

export const getSecurityGroup = (scope: TerraformStack) => {
const sgroup = new DataAwsSecurityGroup(scope, "security_group", {
filter: [
{
name: "tag:Name",
values: ['demoTagName'], //Replace demoTagName by Name Tag of security group
},
],
});
return [sgroup.id];
};


export const getEnv = (scope: TerraformStack) => {
let envVar: EnvVar = {};
checkEnv();

for (const key in process.env) {
// Check if the property is directly defined on the object (not inherited)
if (process.env.hasOwnProperty(key)) {
//read value from ssm
const ssm = new DataAwsSsmParameter(scope, "db_admin_username_ssm_param", {
name: process.env[key] ?? '',
withDecryption: true
});
// Copy the value from process.env to envVar
envVar[key] = ssm.value;
}
}

return envVar;
}


export const checkEnv = () => {
let envToCheck = readFileSync('../.env.schema', "utf8").split(/[\n =]/).filter(Boolean);
Copy link
Contributor

@Yogi-Vishwas Yogi-Vishwas Feb 1, 2024

Choose a reason for hiding this comment

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

This misses the case when there are comments in the env.
There can be both inline and a whole line comments.
I think the regex is already splitting the string on space, new line and = so you just need to filter values that begin with #.
readFileSync(path, "utf8").split(/[\n =]/).filter(Boolean).filter((elem) => !elem.startsWith("#"));

also I think it would be better to split it into lines by splitting it first into "\n" and then take the left side of "=" (i.e. line[0]), because this code assumes that the values will not be there and the values will be filtered using filter(Boolean) so in case the user has a value with a property in the .env.schema this code will treat that (value) as property also. You don't need to deal with values anyway, you just need properties.

you can use this
const lines = readFileSync(path, "utf-8").split("\n").filter((line) => line.length);
const properties = lines.map((line) => line.trim().split("=")[0].trim()).filter((line) => !line.startsWith("#"));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm ,
better

Choose a reason for hiding this comment

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

Why would you always assume a .env file? We should just be checking for environment variables. Assuming an ENV assumes a CI/CD process creates a file when it does

Choose a reason for hiding this comment

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

We can use dotenv. Let's not hand write anything to read an env file or environment variables.

envToCheck.forEach(key => {
if (!env.hasOwnProperty(key)) {
throw new Error(`env is missing- ${key}`);
}
})
}
62 changes: 3 additions & 59 deletions packages/arc-auth/cdk/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,47 +13,16 @@ dotenvExt.load({

const app = new App();

const getSubnetIds = () => {
try {
const subnetIds = process.env?.SUBNET_IDS || '';
return JSON.parse(subnetIds);
} catch (e) {
console.error(e); // NOSONAR
}
return [];
};

const getSecurityGroup = () => {
try {
const securityGroup = process.env?.SECURITY_GROUPS || '';
return JSON.parse(securityGroup);
} catch (e) {
console.error(e); // NOSONAR
}
return [];
};

new MigrationStack(app, 'migration', {
// NOSONAR
codePath: resolve(__dirname, '../../migration'),
handler: 'lambda.handler',
runtime: 'nodejs18.x',
vpcConfig: {
securityGroupIds: getSecurityGroup(),
subnetIds: getSubnetIds(),
},
memorySize: 256,
invocationData: '{}',
timeout: 60,
envVars: {
DB_HOST: process.env.DB_HOST || '',
DB_PORT: process.env.DB_PORT || '',
DB_USER: process.env.DB_USER || '',
DB_PASSWORD: process.env.DB_PASSWORD || '',
DB_DATABASE: process.env.DB_DATABASE || '',
},
namespace: process.env.NAMESPACE || '',
environment: process.env.ENV || '',
});

new LambdaStack(app, 'lambda', {
Expand All @@ -63,38 +32,13 @@ new LambdaStack(app, 'lambda', {
handler: 'lambda.handler',
runtime: 'nodejs18.x',
layerPath: resolve(__dirname, '../../layers'),
vpcConfig: {
securityGroupIds: getSecurityGroup(),
subnetIds: getSubnetIds(),
},

memorySize: 256,
timeout: 30,
envVars: {
DB_HOST: process.env.DB_HOST || '',
DB_PORT: process.env.DB_PORT || '',
DB_USER: process.env.DB_USER || '',
DB_PASSWORD: process.env.DB_PASSWORD || '',
DB_DATABASE: process.env.DB_DATABASE || '',
DB_SCHEMA: process.env.DB_SCHEMA || '',
JWT_SECRET: process.env.JWT_SECRET || '',
JWT_ISSUER: 'sourcefuse',
PORT: '3005',
LOG_LEVEL: 'info',
DB_CONNECTOR: 'postgresql',
},
customDomainName: {
domainName: process.env.DOMAIN_NAME || '',
hostedZoneId: process.env.HOSTED_ZONE_ID || '',
},
namespace: process.env.NAMESPACE || '',
environment: process.env.ENV || '',

useImage: true,
});

new RedisStack(app, 'redis', {
// NOSONAR
namespace: process.env.NAMESPACE || '',
environment: process.env.ENV || '',
});
new RedisStack(app, 'redis');

app.synth();
Loading