CLI arguments affecting lists: values are appended instead of overriding the list

[hhromic]

Describe the bug

I noticed that the -frontend.instance-interface-names argument actually appends the interface name to the existing default list instead of overriding it fully from the value given in the CLI. For example:

$ docker run --rm grafana/loki:2.4.2 -config.file=/etc/loki/local-config.yaml -frontend.instance-interface-names=myiface -print-config-stderr
(...)
frontend:
  log_queries_longer_than: 0s
  max_body_size: 10485760
  query_stats_enabled: false
  max_outstanding_per_tenant: 100
  querier_forget_delay: 0s
  scheduler_address: ""
  scheduler_dns_lookup_period: 10s
  scheduler_worker_concurrency: 5
  grpc_client_config:
    max_recv_msg_size: 104857600
    max_send_msg_size: 16777216
    grpc_compression: ""
    rate_limit: 0
    rate_limit_burst: 0
    backoff_on_ratelimits: false
    backoff_config:
      min_period: 100ms
      max_period: 10s
      max_retries: 10
    tls_enabled: false
    tls_cert_path: ""
    tls_key_path: ""
    tls_ca_path: ""
    tls_server_name: ""
    tls_insecure_skip_verify: false
  instance_interface_names:   # this should be a list with 'myiface' alone, but 'myiface' got appended instead
  - eth0
  - en0
  - lo
  - myiface
  address: ""
  port: 0
  compress_responses: false
  downstream_url: ""
  tail_proxy_url: ""
(...)

I don't think I'm using the CLI incorrectly here, and I also noticed that it happens with any CLI argument that refers to interface names. Maybe it affects all options that are lists?

Moreover, if you want to set multiple values, this also doesn't work and instead single-appends anything passed:
EDIT: It was clarified that to append multiple values, you have to provide the CLI argument multiple times.

If this is the intended behaviour for -frontend.instance-interface-names (and other similar options) then maybe the CLI argument is a bit useless because interface names are tried in top-down order in the list, thus any option passed has little effect if you want to override the default values.

To Reproduce
Steps to reproduce the behavior:

1. Start Loki with the -frontend.instance-interface-names=myiface argument and the -print-config-stderr to see the applied configuration:

   docker run --rm grafana/loki:2.4.2 -config.file=/etc/loki/local-config.yaml -frontend.instance-interface-names=myiface -print-config-stderr
   

2. Observe in the printed configuration the frontend.instance_interface_names list.

3. Observe that the list has myiface appended to the bottom of the existing defaults instead of being set exclusively.

Expected behavior
I expected that the -frontend.instance-interface-names CLI argument (and any other argument affecting lists) sets the relevant list completely from the value given instead of appending to any existing list of values.
Moreover, if I pass multiple values, i.e. comma-separated, I expect each of them to be set (in order) to the relevant list as individual items.

For example:

$ docker run --rm grafana/loki:2.4.2 -config.file=/etc/loki/local-config.yaml -frontend.instance-interface-names=myiface -print-config-stderr
(...)
frontend:
  (...)
  instance_interface_names:
  - myiface
(...)

Environment:

• Infrastructure: plain Loki binary in bare-metal or Docker container
• Deployment tool: N/A

[DylanGuedes]

I did some investigation and:

• To add multiple interface names through CLI, repeat the flag usage. Ex: instead of -frontend.instance-interface-names=myiface,otheriface, try -frontend.instance-interface-names=myiface -frontend.instance-interface-names=otheriface
• If we modify the flagset to completely override the list (to avoid appending to it), there won't be a way to append to it. In the other hand, if you want to completely override the list you only have to have them in your configuration file. However, I agree with you: this is inconsistent.

[hhromic]

Hi @DylanGuedes, thanks for your answer and investigation.

• To add multiple interface names through CLI, repeat the flag usage. Ex: instead of -frontend.instance-interface-names=myiface,otheriface, try -frontend.instance-interface-names=myiface -frontend.instance-interface-names=otheriface

Ah interesting, forgot to test the case of repeating CLI arguments. Good to know!

• If we modify the flagset to completely override the list (to avoid appending to it), there won't be a way to append to it. In the other hand, if you want to completely override the list you only have to have them in your configuration file. However, I agree with you: this is inconsistent.

Yes, being able to override the list is what is missing then. While it is true that one can just set the list in the configuration file, the use case is situations where you need to modify the configuration on the fly with the CLI. For example, to just override simple options of the default configuration in the official Loki Docker image.

For a simple option change (such as the interface names), it becomes a big burden to have to create a copy of the local stock configuration in the Docker image, modify it just for such option, and then provide it to the container either by baking it into a new custom image or bind-mounting it when running.

How about having a set of two flags for list-options then? For example (CLI argument names not final!):

• -frontend.instance-interface-names which overrides the list with a single value.
• -frontend.instance-interface-names-append which appends a single value to the list (current behaviour).

In this way, we can cover all use-cases for populating option-lists:

# create a list of interface names from scratch (doesn't matter what is in the configuration file)
loki -frontend.instance-interface-names=myiface -frontend.instance-interface-names-append=otheriface

# append interface names to whatever is already in the configuration file
loki -frontend.instance-interface-names-append=myiface -frontend.instance-interface-names-append=otheriface

[DylanGuedes]

For a simple option change (such as the interface names), it becomes a big burden to have to create a copy of the local stock configuration in the Docker image, modify it just for such option, and then provide it to the container either by baking it into a new custom image or bind-mounting it when running.

That's a fair point.

I'll bring these changes to the discussion. I'm inclined to change CLI flags to override values, but my only concern is that it would be a breaking change.

[hhromic]

Depending on how the CLI arguments are re-organised/named, maybe it doesn't have to be a breaking change.
For example, there could be a CLI argument to fully override the list, and then use the existing arguments to populate it by appending.

For example:

• frontend.instance-interface-names-override which clears the option list and assigns a single value.
• frontend.instance-interface-names which appends a single value to the list (current behaviour).

Then, to create a new list with two options, you could do:

# create a list of interface names from scratch (doesn't matter what is in the configuration file)
loki -frontend.instance-interface-names-override=myiface -frontend.instance-interface-names=otheriface

# or, existing behaviour: append interface names to whatever is already in the configuration file
loki -frontend.instance-interface-names=myiface -frontend.instance-interface-names=otheriface

In any case, thanks for considering my issue. It is much appreciated :)

[DylanGuedes]

Hey, thanks for your patience!

After some discussion with some maintainers:

• Adding another CLI flag just to override a configuration seems a little too much (no other project follows this approach). Also, it would be inconsistent with other configs unless we do the same to them, but that would be hard to scale ( you might be scared by the number of flags listed in Loki -help)
• Although having to specify through the config file is annoying when you want to quickly override it, it is still a way of getting the desired behavior. Because of this, we won't change the current behavior immediately
• However, as the current behavior is misleading, we'll work to modify this for the 3.x release. This way, we'll have some time to work on this and being a breaking change won't be a problem anymore

[hhromic]

However, as the current behavior is misleading, we'll work to modify this for the 3.x release. This way, we'll have some time to work on this and being a breaking change won't be a problem anymore

Sounds good to me!
Thanks for the consideration and looking forward for Loki to keep improving!

[DylanGuedes]

I did some investigation and:

• To add multiple interface names through CLI, repeat the flag usage. Ex: instead of -frontend.instance-interface-names=myiface,otheriface, try -frontend.instance-interface-names=myiface -frontend.instance-interface-names=otheriface
• If we modify the flagset to completely override the list (to avoid appending to it), there won't be a way to append to it. In the other hand, if you want to completely override the list you only have to have them in your configuration file. However, I agree with you: this is inconsistent.

[hhromic]

Hi @DylanGuedes, thanks for your answer and investigation.

• To add multiple interface names through CLI, repeat the flag usage. Ex: instead of -frontend.instance-interface-names=myiface,otheriface, try -frontend.instance-interface-names=myiface -frontend.instance-interface-names=otheriface

Ah interesting, forgot to test the case of repeating CLI arguments. Good to know!

• If we modify the flagset to completely override the list (to avoid appending to it), there won't be a way to append to it. In the other hand, if you want to completely override the list you only have to have them in your configuration file. However, I agree with you: this is inconsistent.

Yes, being able to override the list is what is missing then. While it is true that one can just set the list in the configuration file, the use case is situations where you need to modify the configuration on the fly with the CLI. For example, to just override simple options of the default configuration in the official Loki Docker image.

For a simple option change (such as the interface names), it becomes a big burden to have to create a copy of the local stock configuration in the Docker image, modify it just for such option, and then provide it to the container either by baking it into a new custom image or bind-mounting it when running.

How about having a set of two flags for list-options then? For example (CLI argument names not final!):

• -frontend.instance-interface-names which overrides the list with a single value.
• -frontend.instance-interface-names-append which appends a single value to the list (current behaviour).

In this way, we can cover all use-cases for populating option-lists:

# create a list of interface names from scratch (doesn't matter what is in the configuration file)
loki -frontend.instance-interface-names=myiface -frontend.instance-interface-names-append=otheriface

# append interface names to whatever is already in the configuration file
loki -frontend.instance-interface-names-append=myiface -frontend.instance-interface-names-append=otheriface

[DylanGuedes]

For a simple option change (such as the interface names), it becomes a big burden to have to create a copy of the local stock configuration in the Docker image, modify it just for such option, and then provide it to the container either by baking it into a new custom image or bind-mounting it when running.

That's a fair point.

I'll bring these changes to the discussion. I'm inclined to change CLI flags to override values, but my only concern is that it would be a breaking change.

[hhromic]

Depending on how the CLI arguments are re-organised/named, maybe it doesn't have to be a breaking change.
For example, there could be a CLI argument to fully override the list, and then use the existing arguments to populate it by appending.

For example:

• frontend.instance-interface-names-override which clears the option list and assigns a single value.
• frontend.instance-interface-names which appends a single value to the list (current behaviour).

Then, to create a new list with two options, you could do:

# create a list of interface names from scratch (doesn't matter what is in the configuration file)
loki -frontend.instance-interface-names-override=myiface -frontend.instance-interface-names=otheriface

# or, existing behaviour: append interface names to whatever is already in the configuration file
loki -frontend.instance-interface-names=myiface -frontend.instance-interface-names=otheriface

In any case, thanks for considering my issue. It is much appreciated :)

[DylanGuedes]

Hey, thanks for your patience!

After some discussion with some maintainers:

• Adding another CLI flag just to override a configuration seems a little too much (no other project follows this approach). Also, it would be inconsistent with other configs unless we do the same to them, but that would be hard to scale ( you might be scared by the number of flags listed in Loki -help)
• Although having to specify through the config file is annoying when you want to quickly override it, it is still a way of getting the desired behavior. Because of this, we won't change the current behavior immediately
• However, as the current behavior is misleading, we'll work to modify this for the 3.x release. This way, we'll have some time to work on this and being a breaking change won't be a problem anymore

[hhromic]

However, as the current behavior is misleading, we'll work to modify this for the 3.x release. This way, we'll have some time to work on this and being a breaking change won't be a problem anymore

Sounds good to me!
Thanks for the consideration and looking forward for Loki to keep improving!