Adding enclave restricted flags #1668
Conversation
WalkthroughThe changes across various files reflect a significant update to a Go project, focusing on the introduction of a new flag handling system that supports both restricted and unrestricted modes. This system is integrated into the enclave application's Dockerfile, CLI parsing, and configuration management. The Dockerfile has been modified to allow building the enclave in different modes based on a build argument. The Go code has been refactored to use the new flag package for parsing command-line arguments and environment variables, with tests added to ensure the functionality works as expected. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files ignored due to filter (5)
- go.sum
- go/enclave/container/test.toml
- go/enclave/main/enclave-test.json
- go/enclave/main/enclave.json
- testnet/docker-compose.local.yml
Files selected for processing (5)
- dockerfiles/enclave.Dockerfile (2 hunks)
- go/common/flag/flag.go (1 hunks)
- go/common/flag/flag_test.go (1 hunks)
- go/enclave/container/cli.go (4 hunks)
- testnet/testnet-local-build_images.sh (1 hunks)
Additional comments: 3
dockerfiles/enclave.Dockerfile (1)
- 50-50: Verify that the default setting of
OE_SIMULATION=1aligns with the security requirements and intended use of the enclave, as this enables simulation mode by default.go/common/flag/flag.go (1)
- 143-167: Verify if the environment variable prefix "EDG_" used in
parseMandatoryFlagsis intended to be hardcoded or if it should be configurable.go/enclave/container/cli.go (1)
- 47-83: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [47-114]
The refactoring of the
ParseConfigfunction to use the new flag package and the removal of file-based configuration parsing is a significant change. Ensure that all necessary configuration options are available through the new flag-based mechanism and that the transition does not omit any critical configuration parameters. Additionally, verify that the new restricted flags are indeed restricted as intended and that theflagUsageMapis correctly populated and used.
dockerfiles/enclave.Dockerfile
Outdated
| # /home/obscuro/go-obscuro/go/enclave/main contains the executable for the enclave | ||
| # | ||
|
|
||
| ARG RESTRICTEDMODE |
There was a problem hiding this comment.
Consider providing a default value for the RESTRICTEDMODE argument to ensure the Dockerfile can be built even if the argument is not explicitly provided.
ARG RESTRICTEDMODE=false
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- dockerfiles/enclave.Dockerfile (2 hunks)
- go/common/flag/flag.go (1 hunks)
Additional comments: 4
dockerfiles/enclave.Dockerfile (4)
13-13: The introduction of the
RESTRICTEDMODEargument is a good practice for conditional builds and allows for flexibility in the build process.54-55: The
COPYcommand correctly specifies the destination path with a leading slash, ensuring the binary is copied to the correct location.48-48: The use of variable substitution in the
FROMinstruction is a clever way to switch between build stages based on theRESTRICTEDMODEargument.58-58: Setting the
OE_SIMULATIONenvironment variable to1by default is a clear way to indicate that simulation mode is active.
| @@ -13,5 +13,35 @@ | |||
| "type": "hostfs", | |||
| "readOnly": false | |||
| } | |||
| ], | |||
There was a problem hiding this comment.
I don't quite get it. How are the other values injected?
There was a problem hiding this comment.
Are you referring to the other flags that are not restricted ? Those values are injected via the CLI flags.
|
Also, to replace "Restricted_mode" with "Test_mode" ( and reverse the conditions) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- testnet/docker-compose.local.yml
Files selected for processing (2)
- dockerfiles/enclave.Dockerfile (2 hunks)
- testnet/testnet-local-build_images.sh (1 hunks)
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- go/enclave/main/enclave-test.json
- go/enclave/main/enclave.json
Files selected for processing (6)
- dockerfiles/enclave.Dockerfile (2 hunks)
- go/common/flag/flag.go (1 hunks)
- go/config/enclave_cli_flags.go (1 hunks)
- go/config/enclave_config.go (2 hunks)
- go/config/enclave_config_test.go (1 hunks)
- go/enclave/main/main.go (1 hunks)
Additional comments: 10
go/config/enclave_config.go (6)
15-16: The summary states that the import for "github.com/ethereum/go-ethereum/common" was removed, but it is still present in the code as an alias "gethcommon". Please verify this discrepancy.
108-137: The implementation of
FromFlagsfunction correctly handles errors related to flag retrieval and parsing, and it adheres to the summary description.140-174: The implementation of
retrieveEnvFlagsfunction correctly handles errors related to environment flag parsing, and it adheres to the summary description.176-210: The implementation of
newConfigfunction correctly sets various fields of theEnclaveConfigbased on the provided flags, and it adheres to the summary description.108-137: The
FromFlagsfunction correctly returns anEnclaveConfigand an error as described in the summary.108-137: The error handling in the
FromFlagsfunction for flag retrieval and parsing is implemented as described in the summary.go/config/enclave_config_test.go (2)
12-20: The backup and restoration of the
flag.CommandLineare correctly implemented to ensure test isolation.26-45: The approach of setting flags directly on the
flag.CommandLineand then parsing them is correct for simulating command line flag parsing in a test.go/enclave/main/main.go (2)
21-21: Verify if
tenflag.Parse()returns an error and ensure it is properly handled if it does.29-29: Verify if
container.Serve(enclaveContainer)returns an error and ensure it is properly handled if it does.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/config/enclave_config.go (2 hunks)
- go/config/enclave_config_test.go (1 hunks)
Additional comments: 3
go/config/enclave_config.go (2)
3-8: The summary mentions the addition of the "strings" import, but it is not visible in the provided hunk. Please verify if the "strings" import is indeed used and included in the file.
108-137: The
FromFlagsfunction introduces a new environment variable "EDG_TESTMODE". Ensure that this variable is well-documented and that its usage is clear to the users or operators of the system.go/config/enclave_config_test.go (1)
- 13-63: The test
TestCLIFlagTypescorrectly sets up and asserts the values of various flags, ensuring that the flag parsing and configuration creation are functioning as expected.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- go/common/flag/flag.go (1 hunks)
- go/config/enclave_config.go (2 hunks)
- go/config/enclave_config_test.go (1 hunks)
Additional comments: 6
go/common/flag/flag.go (2)
- 49-49: The
Valuefield inNewInt64Flagshould be initialized to0, notfalse.- Value: false, + Value: int64(0),
- 59-59: The
Valuefield inNewUint64Flagshould be initialized to0, notfalse.- Value: false, + Value: uint64(0),go/config/enclave_config.go (1)
- 108-144: The implementation of
FromFlagscorrectly handles the distinction between test mode and normal operation, and the overriding of CLI flags with environment flags is consistent with the intended functionality.go/config/enclave_config_test.go (3)
13-63: The test
TestCLIFlagTypesis well-structured and tests the functionality of setting and retrieving different types of flags.65-102:
104-137: The test
TestRestrictedModeNoCLIDuplicationcorrectly asserts that restricted flags should not be set via the command line and should raise an error if attempted.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- go/config/enclave_config.go (2 hunks)
- go/config/enclave_config_test.go (1 hunks)
- go/enclave/main/main.go (1 hunks)
Additional comments: 7
go/config/enclave_config.go (3)
108-145: The implementation of
NewConfigFromFlagscorrectly checks for theEDG_TESTMODEenvironment variable to determine if the application is running in test mode and then retrieves environment flags accordingly. It also ensures that restricted flags are not set via the CLI when not in test mode, which aligns with the PR objectives.147-187: The
retrieveEnvFlagsfunction enforces that all restricted environment variables must be set, which could lead to errors if some flags are optional. This behavior should be reviewed to ensure it aligns with the intended use of restricted flags, as it may not be desirable for all restricted flags to be mandatory.189-223: The
newConfigfunction uses string literals for flag names when accessing the flags map. It would be more maintainable to define constants for these flag names to reduce the risk of typos and improve code readability.go/config/enclave_config_test.go (3)
3-4: The standard
flagpackage is still being used, which might be intentional for testing compatibility with the newflagpackage. Confirm that this usage aligns with the intended testing strategy.65-102: The tests for restricted mode appear to correctly set environment variables for restricted flags and assert their values in the configuration, aligning with the PR objectives.
104-137: The
TestRestrictedModeNoCLIDuplicationtest correctly asserts that an error is thrown when a restricted flag is set via the command line, which is in line with the PR objectives.go/enclave/main/main.go (1)
- 23-29: The code changes align with the PR objectives and the summary provided. The new flag handling and configuration logic are implemented as described.
go/enclave/main/main.go
Outdated
| config, err := enclavecontainer.ParseConfig() | ||
| // fetch and parse flags | ||
| flags := config.EnclaveFlags() | ||
| err := tenflag.CreateCLIFlags(flags) | ||
| if err != nil { | ||
| panic(fmt.Errorf("could not parse config. Cause: %w", err)) |
There was a problem hiding this comment.
am I misunderstanding, or is parsing happening below?
There was a problem hiding this comment.
You're reading it correctly, creation and parsing of flags are 2 separate actions. The main reason for this is how the go flags package works. Flags must be defined before they can be assigned value.
In the tests:
this errors with no such flag -logPath
// Set the flags as needed for the test.
err := flag.CommandLine.Set(LogPathFlag, "string-value")
require.NoError(t, err)
flags := EnclaveFlags()
err = tenflag.CreateCLIFlags(flags)
require.NoError(t, err)
flag.Parse()This passes because the flag is defined.
flags := EnclaveFlags()
err = tenflag.CreateCLIFlags(flags)
require.NoError(t, err)
// Set the flags as needed for the test.
err := flag.CommandLine.Set(LogPathFlag, "string-value")
require.NoError(t, err)
flag.Parse()wdyt ?
go/config/enclave_config.go
Outdated
| // check if it's in test mode or not | ||
| val := os.Getenv("EDG_TESTMODE") | ||
| if val == "true" { | ||
| flagsTestMode = true |
There was a problem hiding this comment.
let's log and print a warning here.
There was a problem hiding this comment.
Added the prints! When you say to log, you mean to use the logger right ? Is it ok if we don't use the logger though? Asking because in the flow of the main application, the logger is only created at the container and I think probably this is ok with just the stdou print ? wdyt ?
There was a problem hiding this comment.
yeah, print should be fine
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/config/enclave_cli_flags.go (1 hunks)
- go/config/enclave_config.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- go/config/enclave_config.go
Additional comments: 2
go/config/enclave_cli_flags.go (2)
78-80: The flags
L2BaseFeeFlag,L2CoinbaseFlag, andL2GasLimitFlagare missing descriptions. If this is not intentional, descriptions should be added for consistency and clarity.91-98: The
enclaveRestrictedFlagsare defined, but it's not clear from this code how these flags are restricted to only be loaded from environment variables or within an enclave when prefixed withEDG_. Please ensure that the implementation elsewhere in the codebase enforces this restriction as per the PR objectives.
go/config/enclave_cli_flags.go
Outdated
| return map[string]*flag.TenFlag{ | ||
| HostIDFlag: flag.NewStringFlag(HostIDFlag, "", FlagDescriptionMap[HostIDFlag]), | ||
| HostAddressFlag: flag.NewStringFlag(HostAddressFlag, "", FlagDescriptionMap[HostAddressFlag]), | ||
| AddressFlag: flag.NewStringFlag(AddressFlag, "", FlagDescriptionMap[HostAddressFlag]), |
There was a problem hiding this comment.
There appears to be a copy-paste error in the flag description mapping for AddressFlag. The description is incorrectly using HostAddressFlag's description.
- flag.NewStringFlag(AddressFlag, "", FlagDescriptionMap[HostAddressFlag]),
+ flag.NewStringFlag(AddressFlag, "", FlagDescriptionMap[AddressFlag]),Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| AddressFlag: flag.NewStringFlag(AddressFlag, "", FlagDescriptionMap[HostAddressFlag]), | |
| AddressFlag: flag.NewStringFlag(AddressFlag, "", FlagDescriptionMap[AddressFlag]), |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/config/enclave_cli_flags.go (1 hunks)
- go/config/enclave_config.go (2 hunks)
Files skipped from review due to trivial changes (1)
- go/config/enclave_config.go
Additional comments: 1
go/config/enclave_cli_flags.go (1)
- 3-73: The code correctly defines a set of flags and a list of restricted flags, aligning with the PR objective to differentiate between standard and restricted flags. However, the implementation details for ensuring restricted flags are only loaded from environment variables or prefixed with
EDG_are not visible in this hunk. Ensure that theflagpackage'sParsefunction and other related logic adhere to these requirements.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (7)
- go/config/enclave_cli_flags.go (1 hunks)
- go/config/enclave_config.go (2 hunks)
- go/config/enclave_config_test.go (1 hunks)
- go/enclave/main/main.go (1 hunks)
- go/node/config.go (2 hunks)
- integration/common/constants.go (2 hunks)
- integration/simulation/devnetwork/node.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- go/config/enclave_cli_flags.go
- go/config/enclave_config_test.go
Additional comments: 15
go/config/enclave_config.go (3)
74-83: The PR objectives mention a
RESTRICTEDMODEenvironment variable, but the code checks forEDG_TESTMODE. Please verify if this is correct or if it should beRESTRICTEDMODE.113-153: The implementation of
retrieveEnvFlagsaligns with the PR objectives of loading restricted flags from environment variables, prefixed withEDG_.155-190: The
newConfigfunction correctly reflects the logic changes mentioned in the summary, creating a configuration object from the provided flags.go/enclave/main/main.go (3)
15-16: The code fetches flags using
config.EnclaveFlagsand then attempts to create CLI flags withtenflag.CreateCLIFlags(config.EnclaveFlags). Ensure thattenflag.CreateCLIFlagsis designed to handle both normal and restricted flags according to the PR objectives, especially sinceconfig.EnclaveFlagsmay contain both types of flags.23-23: The introduction of
config.NewConfigFromFlags(flags)aligns with the PR objectives and the summary provided, indicating a change in how the enclave configuration is initialized.28-29: The initialization of
enclaveContainerwithenclavecontainer.NewEnclaveContainerFromConfig(enclaveConfig)and serving it withcontainer.Serve(enclaveContainer)is consistent with the PR objectives, which aim to impact the initialization process of theenclaveContainer.go/node/config.go (3)
9-12: The addition of the
integrationCommonimport is consistent with the PR objectives and the summary provided.77-77: The change from
config.DefaultEnclaveConfig()tointegrationCommon.DefaultEnclaveConfig()is consistent with the PR objectives and the summary provided.76-80: The rest of the changes in this hunk are consistent with the PR objectives and the summary provided.
integration/common/constants.go (3)
3-15: The changes in imports reflect the PR's objective to introduce new configurations and handling within the Ten Protocol's enclave component.
56-58: The addition of the
ERC20Mappingstruct aligns with the summary and PR objectives, indicating an expansion of the system's capabilities to map ERC20 tokens across different layers.60-88: The introduction of
DefaultEnclaveConfigfunction is consistent with the PR's objectives to enhance the enclave's configuration system, providing a clear default state for the enclave's settings.integration/simulation/devnetwork/node.go (3)
31-35: The addition of the
integrationCommonimport and the use ofintegrationCommon.DefaultEnclaveConfig()instead ofconfig.DefaultEnclaveConfig()is consistent with the PR objectives to refactor flag handling and configuration.163-163: The use of
integrationCommon.DefaultEnclaveConfig()to initialize default configuration values is a good practice for maintainability and consistency across different parts of the application.160-166: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [165-167]
The assignment of
BaseFee,GasLimit, andGasPaymentAddressfromdefaultCfgensures that the enclave configuration uses sensible defaults, which is important for consistency and reliability.
Successfully ran at https://github.com/ten-protocol/ten-test/actions/runs/7090754526 |
Why this change is needed
https://github.com/ten-protocol/ten-internal/issues/2247
container/cli.goflags now are eitherflag.TYPEorflag.RestrictedTYPEEDG_-
RESTRICTEDMODEis read from the env vars and defines if restricted flag can be read from CLIenclave.json( RESTRICTEDMODE = true) at build time ( default )enclave-test.json( RESTRICTEDMODE = false) at build timeenclave.jsonhas the default values for enclave to be run in testnetRESTRICTEDMODE = false