Setup ten config library to replace our flags approach #2115

BedrockSquirrel · 2024-10-29T11:43:46Z

Why this change is needed

We are moving away from flags based config to a systematic yaml/environment variable based config that will be used across all our go processes. This will make our config play more nicely with standard orchestration tooling (kubernetes etc.) and reduce duplication and magic in our config.

What changes were made as part of this PR

This first PR adds the config structure, the loading mechanism and sets it up for used by the enclave and host processes.

All other Go processes (including the node and testnet launcher orchestration processes) are still configured with flags. They will be migrated shortly.

Short term while both systems are in place this may seem to be making things worse but when the node and testnet launchers are replaced with the infra tooling that is being worked on then a lot of code and magic will be removed.

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

PR checks reviewed and performed

tudor-malene

This is great!
Makes everything much clearer.

I left a few comments.

go/config/defaults/0-base-config.yaml

go/config/loader.go

go/enclave/main/enclave-test.json

tudor-malene · 2024-10-29T17:13:49Z

go/enclave/main/main.go

-	if err != nil {
-		panic(fmt.Errorf("unable to create config from flags - %w", err))
-	}
+	enclaveConfig := enclaveconfig.EnclaveConfigFromTenConfig(tenCfg)


I don't follow here how the config is read from enclave.json

Enclave config is read the same as the host and other processes: base yaml file(s) + environment variables

In enclave.json the "fromHost" flag allows you to whitelist env variable keys so their values can be passed in from the host machine.

E.g. if this was in the enclave.json then NETWORK_CHAINID=1337 would be set and ENCLAVE_DB_EDGELESSDBHOST value would be passed through if the runner set it as an environment variable.

"env": [ { "name": "ENCLAVE_DB_EDGELESSDBHOST", "fromHost": true }, { "name": "NETWORK_CHAINID", value: 1337 }, ]

So for now I'm just letting it pass through all the env variables. For tagged public releases we can hardcode values.

what confuses me is that I don't see anywhere in the code how "fromHost" is resolved.
I don't follow how the right values end up in the enclave.json file that is passed to ego

testnet/launcher/docker.go

StefanIliev545

A lot of changes that seem like grunt work I glanced over, but this is my first (prolly final) pass

P.S. Ask copilot for any improvements and see if anything is reasonable; obviously as you've written he code you're best placed to know if it's bullshitting or legit suggestion

StefanIliev545 · 2024-10-30T22:44:23Z

go/config/README.md

+Ten config has a hierarchical structure, so every field has a unique field name. Some fields are used by multiple processes, 
+while others are only used by a single process.
+
+For example, `network.chainId` is a field that might be used by multiple processes, but `host.rpc.httpPort` is only used by the host processes.


StefanIliev545 · 2024-10-30T22:45:35Z

go/config/README.md

+the mapstructure annotation that gives their yaml field name and any comments about their usage.
+
+This library provides a loading mechanism powered by viper, it reads 0-base-config.yaml to initialise the config and then
+overrides fields with any other yaml files provided and finally overrides with environment variables.


Is this ok for production? ENV variable overrides should be disabled in enclave IMO? Or at least disabled for anything baked into the image

StefanIliev545 · 2024-10-30T22:46:43Z

go/config/README.md

+  we want to make sure that all fields are defined in the base config file even if just with trivial or empty values.
+
+- The ego enclave restricts the environment variables that can be accessed by the enclave process. This means that the enclave
+  process will not be able to access environment variables that are not whitelisted. This is a security feature of the ego enclave.


go/config/decoding.go

StefanIliev545 · 2024-10-30T22:49:24Z

go/config/defaults/0-base-config.yaml

+
+network:
+  chainId: 443
+  genesis: "{}" # json string of genesis block, used for configuring prefunded accounts etc.


Do we still rely on genesis override in sim tests? Perhaps a good opportunity to have test/1-sim.yaml that showcases overrides

StefanIliev545 · 2024-10-30T22:49:51Z

go/config/defaults/0-base-config.yaml

+    maxSize: 56320 # 55kb
+  rollup:
+    interval: 5s
+    maxInterval: 10m # rollups will be produced after this time even if the data blob is not full


Is this identical for local testnet or is it the sepolia time?

StefanIliev545 · 2024-10-30T22:51:25Z

go/config/defaults/0-base-config.yaml

+    rpcTimeout: 10s
+  l1:
+    wsURL: ws://localhost:8546 # websocket URL for L1 RPC service
+    beaconURL: eth2network:12600 # websocket URL for L1 beacon service


Okay this cfg looks to be a mix of what we have right now (unless I'm wrong, which is quite possible).
Maybe we should have one for each environment (basic matches local testnet, overrides only for environments)

So I started doing one for each environment but I think it's a waste of time because we'll be throwing it away in a couple of weeks. We don't want environmental config in our repo and it will live in Krish's kubernetes config stores.

So for now all these config values are still coming from github in the testnet environments. For sims and local networks we use yaml file overrides (or set the values dynamically in the code where that makes sense).

Happy to be convinced otherwise though, I have gone around in circles on this. Originally I had the environment passed to the main as a flag, like -env=uat and then it would auto-load 1-env-uat.yaml.

But anyway, if we get the core process in we can iterate on how it's used, for now I just want it in without breaking anything so Krish can start testing against it.

StefanIliev545 · 2024-10-30T22:52:28Z

go/config/defaults/testnet-launcher/2-validator.yaml

+   nodeType: validator
+   isGenesis: false
+   id: 0x2f7fCaA34b38871560DaAD6Db4596860744e1e8A
+   privateKey: ebca545772d6438bbbe1a16afbed455733eccf96157b52384f1722ea65ccfa89


Are we going to leave this as is? Perhaps this field should either be a key or somewhere to load it from ( or off chain signing )
Maybe a todo with a ticket

tudor-malene

follow up question

tudor-malene · 2024-10-31T16:39:29Z

go/enclave/main/main.go

-	if err != nil {
-		panic(fmt.Errorf("unable to create config from flags - %w", err))
-	}
+	enclaveConfig := enclaveconfig.EnclaveConfigFromTenConfig(tenCfg)


what confuses me is that I don't see anywhere in the code how "fromHost" is resolved.
I don't follow how the right values end up in the enclave.json file that is passed to ego

BedrockSquirrel · 2024-11-04T12:42:11Z

@tudor-malene The values don't get set into the enclave.json (unless they're hardcoded values which we're not doing at this stage). The fromHost flag is part of the enclave.json spec from ego to whitelist those env var keys: https://docs.edgeless.systems/ego/reference/config#environment-variables

So we pass the values to the enclave process with env variables in docker like normal. So when you start the docker container you set the env variables (that's what @pkrishnath 's kubernetes set up will do, and temporarily that's what the docker_node launcher thing is doing).

tudor-malene

LGTM!

BedrockSquirrel added 11 commits October 23, 2024 23:05

Initial spec for ten config

158c828

WIP fixing after rebase

73ccf36

WIP

c3511ac

WIP local network still buggered:

7194eab

WIP some fixes

000c78f

fix local testnet launcher

648b8b4

tidying up

07623b1

Tidying and fixes

ec2ec5e

Fix type cfg

eab6f52

Merge remote-tracking branch 'origin/main' into matt/config-viper

f1f61c9

add the host ID back in for now

8f5c248

tudor-malene reviewed Oct 29, 2024

View reviewed changes

BedrockSquirrel added 5 commits October 30, 2024 10:02

Merge remote-tracking branch 'origin/main' into matt/config-viper

dc06319

review comments

39b6544

linting

4511e92

fix dockerfile

bb9ac7f

fix tests

552ef00

BedrockSquirrel temporarily deployed to dev-testnet October 30, 2024 22:07 — with GitHub Actions Inactive

BedrockSquirrel temporarily deployed to dev-testnet October 30, 2024 22:21 — with GitHub Actions Inactive

BedrockSquirrel had a problem deploying to dev-testnet October 30, 2024 22:27 — with GitHub Actions Failure

StefanIliev545 reviewed Oct 30, 2024

View reviewed changes

WIP fixes for node CLI

bf4d073

BedrockSquirrel temporarily deployed to dev-testnet October 31, 2024 16:11 — with GitHub Actions Inactive

BedrockSquirrel temporarily deployed to dev-testnet October 31, 2024 16:23 — with GitHub Actions Inactive

BedrockSquirrel had a problem deploying to dev-testnet October 31, 2024 16:30 — with GitHub Actions Failure

tudor-malene reviewed Oct 31, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into matt/config-viper

987467f

BedrockSquirrel temporarily deployed to dev-testnet November 1, 2024 10:54 — with GitHub Actions Inactive

BedrockSquirrel temporarily deployed to dev-testnet November 1, 2024 11:08 — with GitHub Actions Inactive

BedrockSquirrel temporarily deployed to dev-testnet November 1, 2024 11:15 — with GitHub Actions Inactive

BedrockSquirrel temporarily deployed to dev-testnet November 1, 2024 11:16 — with GitHub Actions Inactive

BedrockSquirrel temporarily deployed to dev-testnet November 1, 2024 11:20 — with GitHub Actions Inactive

BedrockSquirrel added 3 commits November 1, 2024 13:36

testnet launcher: turn on debug namespace by default

4cd32e5

fix noderunner test

276451d

Merge remote-tracking branch 'origin/main' into matt/config-viper

2d92ad9

BedrockSquirrel added 4 commits November 5, 2024 11:31

Merge remote-tracking branch 'origin/main' into matt/config-viper

ca7da4d

Merge remote-tracking branch 'origin/main' into matt/config-viper

4a3362b

review: add comment

fd0a45e

Merge remote-tracking branch 'origin/main' into matt/config-viper

a046221

BedrockSquirrel temporarily deployed to dev-testnet November 6, 2024 15:28 — with GitHub Actions Inactive

BedrockSquirrel temporarily deployed to dev-testnet November 6, 2024 15:41 — with GitHub Actions Inactive

BedrockSquirrel temporarily deployed to dev-testnet November 6, 2024 15:48 — with GitHub Actions Inactive

BedrockSquirrel deployed to dev-testnet November 6, 2024 15:49 — with GitHub Actions Active

BedrockSquirrel temporarily deployed to dev-testnet November 6, 2024 15:49 — with GitHub Actions Inactive

BedrockSquirrel temporarily deployed to dev-testnet November 6, 2024 15:53 — with GitHub Actions Inactive

tudor-malene approved these changes Nov 6, 2024

View reviewed changes

BedrockSquirrel merged commit 9618a79 into main Nov 6, 2024
11 checks passed

BedrockSquirrel deleted the matt/config-viper branch November 6, 2024 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup ten config library to replace our flags approach #2115

Setup ten config library to replace our flags approach #2115

BedrockSquirrel commented Oct 29, 2024

tudor-malene left a comment

tudor-malene Oct 29, 2024

BedrockSquirrel Oct 30, 2024

tudor-malene Oct 31, 2024

StefanIliev545 left a comment •

edited

Loading

StefanIliev545 Oct 30, 2024

StefanIliev545 Oct 30, 2024

StefanIliev545 Oct 30, 2024

StefanIliev545 Oct 30, 2024

StefanIliev545 Oct 30, 2024

StefanIliev545 Oct 30, 2024

BedrockSquirrel Oct 31, 2024

StefanIliev545 Oct 30, 2024

tudor-malene left a comment

tudor-malene Oct 31, 2024

BedrockSquirrel commented Nov 4, 2024

tudor-malene left a comment

Setup ten config library to replace our flags approach #2115

Setup ten config library to replace our flags approach #2115

Conversation

BedrockSquirrel commented Oct 29, 2024

Why this change is needed

What changes were made as part of this PR

PR checks pre-merging

tudor-malene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanIliev545 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tudor-malene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BedrockSquirrel commented Nov 4, 2024

tudor-malene left a comment

Choose a reason for hiding this comment

StefanIliev545 left a comment •

edited

Loading