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: [CP-548] Adding config files for EKS stage and dev #5009

Merged
merged 11 commits into from
Oct 4, 2023

Conversation

kathrynlovett
Copy link
Contributor

Ticket: https://kiva.atlassian.net/browse/CP-548

Added new config files for EKS dev and stage so that we can reference them when starting up the UI. Let me know if you have concerns about this approach or if there's another approach that would be better!

This should take the place of our current config map for the UI and work better as we deploy to different environments.

@kathrynlovett kathrynlovett requested review from a team October 3, 2023 22:38
Copy link
Contributor

@JoeTravisKiva JoeTravisKiva left a comment

Choose a reason for hiding this comment

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

This might be a good time to consider how we can read these values from env vars and populate them similarly to apolloBatching and memcachedServers. In an ideal world, we would be able to change these without needing to create a new build.

config/eks.js Outdated
cNTV7eN5sBKgv9nQOxDpAz1pPfJGlBI5: `https://admin.${baseUrl}/admin/login?force=1`,
e6wSaTBDpKRkV5SV5cWw6zD6eJjd2DEk: `https://partners.${baseUrl}/pa2/login/login?authLevel=recent`,
xOXldYg02WsLnlnn0D5xoPWI2i3aNsFD: `https://www.${baseUrl}/authenticate?authLevel=recent`,
KIzjUBQjKZwMRgYSn6NvMxsUwNppwnLH: `https://www.$baseUrl}/ui-login?force=true`,
Copy link
Contributor

Choose a reason for hiding this comment

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

missing a { around baseUrl

@kathrynlovett
Copy link
Contributor Author

@JoeTravisKiva I took a stab at making all the settings that differ between stage and dev configurable. Let me know what you think (also not married to the env/config file name). If this looks good, I can remove the staging file and we can move forward with this. It's set to default to dev values that can then be overriden for stage. Not sure if we want to make it work for prod as well. The current app uses the prod config (index.js) as a base, so we may want a plan for EKS rollout but perhaps after that we'd simply want to continue using that as a base?

config/eks.js Outdated
domain: `login.${env}.kiva.org`,
},
paypal : {
url: 'www.sandbox.paypal.com',
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect we'll want to make a few more of these values configurable (like the paypal values), but I think this approach is great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I figured I'd start small with just the values we'd have to override for stage so as not to bloat the file with variables we would never set. Let me know if you think it would be helpful to add any others in at this point, though!

] Removing unnecessary staging file
config/eks.js Outdated
@@ -0,0 +1,78 @@
const { merge } = require('webpack-merge');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a better name we can come up with other than eks? I'm not a huge fan of putting that in our configs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe dynamic?

@kathrynlovett kathrynlovett requested a review from hobbsh October 4, 2023 16:25
e6wSaTBDpKRkV5SV5cWw6zD6eJjd2DEk: `https://partners.${baseUrl}/pa2/login/login?authLevel=recent`,
xOXldYg02WsLnlnn0D5xoPWI2i3aNsFD: `https://www.${baseUrl}/authenticate?authLevel=recent`,
KIzjUBQjKZwMRgYSn6NvMxsUwNppwnLH: `https://www.${baseUrl}/ui-login?force=true`,
ouGKxT4mE4wQEKqpfsHSE96c9rHXQqZF: `https://www.${baseUrl}/ui-login?force=true`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The keys in this object are environment specific, matching the application id in each Auth0 env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should they be dynamic as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, they will have to be if you want to use this config file across environments.

These are app names in order:
Kiva Admin
partners.dev.kiva.org
Kiva.org php
Ui Server
UI Client

oneTrust: {
enable: true,
key: 'db9dcf94-1c32-40fb-9a57-cefafea1088d',
domainSuffix: '-test',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would need to be an empty string for use in prod.

@kathrynlovett kathrynlovett requested a review from mcstover October 4, 2023 17:03
[browserClientID]: `https://www.${baseUrl}/ui-login?force=true`,
},
enable: true,
checkFakeAuth: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We would not want this to propagate to prod so it should be false by default and be setup to pass in as true only in lower envs...

Copy link
Collaborator

Choose a reason for hiding this comment

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

The way this is currently handled is that the key isn't defined in the index.js config file so it's false by default. Then the key is only added to lower env config files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'd mostly been setting this up mimicking the other lower envs, so it was importing from the prod (index.js) file and overriding that. I was picturing that we may do something similar to the current setup where prod is the default (perhaps create a duplicate of index.js for EKS configuration) that is then inherited from and overridden by the lower environments. Because of that, I hadn't really set this up for prod configuration yet. Do you think we should focus on using this dynamic config file for all EKS environments (and thus configure it with prod in mind) or continue with the existing configuration file inheritance pattern in this repo (and possibly spin up the base configuration file for EKS that contains prod values and is inherited by dynamic)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we want to tackle that problem in this PR (I would personally like it) but I think the end goal should definitely be allowing UI services to be configured entirely via secrets and environment variables, so a dynamic config seems attractive to me. I'll let @JoeTravisKiva chime in in terms of priority.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seemed to be trending toward something that might get used for all eks deployments so I was just trying to safe guard that incase it was fast tracked into use for prod...

If you want to skip the prod setup for now, I think we should name this file in a way that signifies it's only for lower envs, maybe dynamic-dev.js. It could also be good to startup a dynamic-prod.js for the prod base config which has a few values not present in the files that import it as a base.

Copy link
Contributor

@hobbsh hobbsh Oct 4, 2023

Choose a reason for hiding this comment

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

I think in terms of fast-tracking to prod, it would still just be EKS prod which is basically a pre-prod environment at the moment. We should still have time to vet it. Totally get what you're saying though. I'd personally prefer to have a global dynamic config (I like the 12-factor philosophy: apps should have no awareness of their environment in their code, or "strict separation of config from code").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcstover and @hobbsh I hear you both. I've attempted to update the dynamic config to stand alone and work for prod. I pulled in all the properties that were only in the index file, so we no longer need to merge that in. At this point, pre-prod envs will have to set CHECK_FAKE_AUTH to true to enable that. Let me know if you have any thoughts on the current setup/default values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Also, I alphabetized for ease of finding properties. I tried to put that in a separate commit for readability. Let me know if the previous grouping was preferable).

@kathrynlovett kathrynlovett requested a review from mcstover October 4, 2023 18:05
const formattedUrlEnv = env === 'prod' ? '' : `${env}.`;
const enableOptimizely = process.env.ENABLE_OPTIMIZELY !== 'false';
const enablePerimeterx = process.env.ENABLE_PERIMETERX !== 'false';
const disableCluster = process.env.SERVER_DISABLE_CLUSTER === 'true';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one can just be set to true now. We always disable cluster most in k8s infra.

@kathrynlovett kathrynlovett merged commit d95eee9 into main Oct 4, 2023
1 check passed
@kathrynlovett kathrynlovett deleted the feat-CP-548-eks-configs branch October 4, 2023 19:05
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.

4 participants