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

hardcode data provider config into prod enclave #2291

Merged

Conversation

kziemianek
Copy link
Member

@kziemianek kziemianek commented Nov 28, 2023

  • Removes running-mode CLI arg
  • Hardcodes data provider config for prod env
  • Adds reading secrets from env variables
  • Adds possibility for overriding data provider config by env variables on non prod env
  • Decouples data providers from global config variable (configs are provided as constructor parameters)
  • StfTaskContext holds reference to ShieldingKeyRepository because shielding key can be changed after mu-ra key provisioning.

Copy link

linear bot commented Nov 28, 2023

P-233 Hardcode the data provider URL/constants into prod enclave

As topic - currently these configurations (e.g. "official_twitter_url", achainable endpoint etc) are read from the file. This is risky as the enclave operator could use a wrong config file intentionally or unintentionally, it also means they are free to change the endpoint.

Basically we need to:

  • for prod: hardcode these constants (including the discord role ids maybe?) to enclave logic
  • for dev: allow to override the constants by reading from file

@kziemianek kziemianek added the C0-breaking Breaking change that will cause existing client to break label Nov 28, 2023
@BillyWooo
Copy link
Collaborator

In general, the refactor looks good for me. Let's get the CI tests pass.
Two concerns:

  1. how does this refactor affect the mock server?
  2. I would suggest to wait for PRs from @zhouhuitian and @higherordertech and merge after theirs PRs merged. Because this refactor will block their deployment to current staging env.

@kziemianek
Copy link
Member Author

@BillyWooo this PR doesn't affect mock server. Regarding waiting for other PRs im fine with it. We can wait even longer - untill we drop staging branch.

Copy link
Collaborator

@Kailai-Wang Kailai-Wang left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@BillyWooo
Copy link
Collaborator

untill we drop staging branch.

I would say so. Thanks

@0xverin
Copy link
Contributor

0xverin commented Dec 7, 2023

@BillyWooo this PR doesn't affect mock server. Regarding waiting for other PRs im fine with it. We can wait even longer - untill we drop staging branch.

Will we affect the mock server in the future?

@kziemianek
Copy link
Member Author

@BillyWooo this PR doesn't affect mock server. Regarding waiting for other PRs im fine with it. We can wait even longer - untill we drop staging branch.

Will we affect the mock server in the future?

I think we don't have any plans for it.
We can adjust mocks logic to fit our testing needs.

@kziemianek
Copy link
Member Author

I've made few changes, there very plenty of conflicts and problem with shielding key so I'm requesting a review again 🙃

@kziemianek kziemianek requested a review from a team January 16, 2024 15:01
Copy link
Collaborator

@BillyWooo BillyWooo left a comment

Choose a reason for hiding this comment

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

in general, LGTM. Some points to talk with you privately.

Copy link
Collaborator

@BillyWooo BillyWooo left a comment

Choose a reason for hiding this comment

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

Please resolve the conflict and get CI jobs pass. Thank you.

@kziemianek kziemianek enabled auto-merge (squash) January 22, 2024 11:34
@kziemianek kziemianek merged commit ff22b32 into dev Jan 22, 2024
31 of 32 checks passed
@Kailai-Wang Kailai-Wang deleted the p-233-hardcode-the-data-provider-urlconstants-into-prod-enclave branch January 22, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C0-breaking Breaking change that will cause existing client to break
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants