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

Added config-file flag to accept file based input params #164

Merged
merged 9 commits into from
Jun 21, 2023

Conversation

Ganeshrockz
Copy link
Contributor

@Ganeshrockz Ganeshrockz commented Jun 20, 2023

Why is the change required?

As part of the work needed to migrate consul-ecs to use consul-dataplane and remove consul clients from the ECS tasks, we would need to make the consul ecs control plane configure the dataplane config. Since ECS tasks are immutable by nature, we can't call the dataplane CLI command with the appropriate parameters similar to how it is being done in consul-k8s.

According to the RFC, it was decided that the Consul ECS control plane would take of care of writing the configuration needed to start dataplane to a shared volume and pass the filename as an input parameter to the consul-dataplane command. Dataplane would then configure itself based on the config values present in the file.

What has changed?

This PR introduces an additional string input parameter -config-file which accepts a json file for now. We can later extend it to accepting hcl files if needed.

How is the change implemented?

CLI flag inputs (if provided) still holds the highest precedence even if the same config input field is present the config file. Upon startup, dataplane prepares a default config based on the default values present already for various fields. Following that, dataplane reads the configuration present in the json file, unmarshals the input and prepares a config struct based on the flags present in the file. It then merges the second config onto the first config if the values present in the second config aren't the default ones. Finally dataplane constructs a third struct based on the CLI flag inputs and merges it to the previously formed config. In this way, we can make sure that the right order of precedence is maintained when accepting and processing inputs

How was the change tested?

  • Unit tests (covering various input generation flows
  • CI

@Ganeshrockz Ganeshrockz requested a review from a team as a code owner June 20, 2023 02:59
cmd/consul-dataplane/config.go Outdated Show resolved Hide resolved
cmd/consul-dataplane/merge.go Outdated Show resolved Hide resolved
cmd/consul-dataplane/merge.go Outdated Show resolved Hide resolved
@Ganeshrockz Ganeshrockz requested a review from pglass June 20, 2023 06:51
.changelog/164.txt Outdated Show resolved Hide resolved
cmd/consul-dataplane/merge.go Outdated Show resolved Hide resolved
cmd/consul-dataplane/duration.go Outdated Show resolved Hide resolved
cmd/consul-dataplane/flags.go Show resolved Hide resolved
@Ganeshrockz Ganeshrockz requested a review from pglass June 20, 2023 18:25
Copy link

@pglass pglass left a comment

Choose a reason for hiding this comment

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

Few other minor comments, but this looks good to me


// Constructs a config with the default values
func buildDefaultConsulDPFlags() (*DataplaneConfigFlags, error) {
data := `
Copy link

Choose a reason for hiding this comment

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

Why define this as a JSON string that is parsed, instead of defining a DataplaneConfigFlags struct directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the fields of DataplaneConfigFlags are mostly pointers, it kind of looks ugly referencing the const variables inside the struct. We can't define const variables for these default flags and assign them as &DefineConsulFlag to those fields. We would have to fallback to primitive types for these variables which doesn't look that good as well. So I went ahead with a hardcoded json that is easier for people to make changes. I can reiterate on this if people see further concerns.

cmd/consul-dataplane/config_test.go Outdated Show resolved Hide resolved
cmd/consul-dataplane/config_test.go Show resolved Hide resolved
@Ganeshrockz Ganeshrockz merged commit e8a9eb1 into main Jun 21, 2023
@Ganeshrockz Ganeshrockz deleted the net-4580/add-config-file-flag branch June 21, 2023 16:26
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.

3 participants