0xERR0R / blocky

Fast and lightweight DNS proxy as ad-blocker for local network with many features
https://0xERR0R.github.io/blocky/
Apache License 2.0
4.81k stars 210 forks source link

Support Docker env variables based configuration #375

Open agneevX opened 2 years ago

agneevX commented 2 years ago

Traefik supports specifying configuration within the docker-compose file.

pilot:
  token: test

# compose.yml
traefik:
  image: traefik
  labels:
    traefik.pilot.token: test

This would be great for blocky and would eliminate the need for maintaining a separate config file.

EDIT: Setting configuration via environment variables is a better fit rather than Docker labels.

0xERR0R commented 2 years ago

AFAIK it works only if traefik has access to docker daemon (and this is always a security risk). If not, there is no possibility to read labels inside the container. We can introduce environment variables for configuration, but it looks ugly for lists (e.g. for blocking list urls or custom blocking lists)

kwitsch commented 2 years ago

Agreed that the label based solution might be overkill for blocky.

But I would love an environment based configuration. The blocky config is currently the only config file in my DNS stack. Everything else is configured though environment.

For other projects I use koanf to achieve this. There is an option to define conversion functions. With that it might be possible to convert map[int]string to []string. This would enable environment variables like BLOCKY_BLACKLISTS_ADS_1 for example.

0xERR0R commented 2 years ago

koanf looks nice, I'll take a look on it. We need something, which can read yaml and env (env should has precedence over yaml) with custom parsing functionality. Really cool would be a lib which can also set default values with tags and generate documentation.

kwitsch commented 2 years ago

Koanf can read yaml & env(and some more). I personally often use a combination of defaults & koanf.helper lib Haven't seen something for documentation jet.

agneevX commented 2 years ago

AFAIK it works only if traefik has access to docker daemon

Sorry, I had absolutely no idea.

This would enable environment variables like BLOCKY_BLACKLISTS_ADS_1 for example.

This is a good alternative.

0xERR0R commented 2 years ago

@kwitsch I tried koanf and it works well. As far I understand, uses koanf internally a map with string -> string if you parse env variables. There is no support to parse env variables to slices etc. Do you have experience with env variables and complex structures (map for example):

upstream:
  default:
    - 5.9.164.112
    - 1.1.1.1
  laptop*:
    - 123.123.123.123

This could be defines as

Parameter "laptop*" can't be used as variable name (reserved char afaik).

Of course, we can define JSON value for the whole map: "BLOCKY_UPSTREAM={"default": ...}" but this is not really handy.

Any though?

kwitsch commented 2 years ago

Slices in the same variable in a CSV notation should work out of the box. Further are maps supported. I try to prevent using variables with multiple values(slices) by using maps and convert them later. There is probably a better option, didn't investigate much as my focus was on other things. Example usage with maps: TinyMacDns config

kwitsch commented 2 years ago

Using a map[int]string the env config would look like:

agneevX commented 2 years ago

What if this was specified in command?

logLevel: error

upstream:
  default:
    - https://8.8.8.8/dns-query
    - https://8.8.4.4/dns-query
services:
  blocky:
    image: spx01/blocky
    container_name: blocky
    restart: unless-stopped
    command:
      - --loglevel=error
      - --upstream.default=https://8.8.8.8/dns-query
      - --upstream.default=https://8.8.4.4/dns-query
kwitsch commented 2 years ago

Wouldn't really make a difference and env variables are more common.
The effort to rebuild the config stays more or less the same.
I started converting it to koanf(which supports both env & parameter) a while ago but ran out of free time.

0xERR0R commented 2 years ago

I tried to implement env based config with koanf and there were some "challenges":

blocking:
  blackLists:
    ads:
      - |
        # inline definition with YAML literal block scalar style
        # hosts format
        someadsdomain.com
  clientGroupsBlock:
    default:
      - ads
    # use client name (with wildcard support: * - sequence of any characters, [0-9] - range)
    # or single ip address / client subnet as CIDR notation
    laptop*:
      - ads

koanf has no support for nested maps -> this must be (somehow) implemented. Also we need a "special" logic to handle inline list definitions...

agneevX commented 2 years ago

Wouldn't really make a difference and env variables are more common.

I feel that this is not a correct way of providing configuration at runtime and is a potential misuse of environment variables.

kwitsch commented 2 years ago

I feel that this is not a correct way of providing configuration at runtime and is a potential misuse of environment variables.

As far as i know it's the most common way to store configurations inside containers. Config files have to be stored in volumes or distrubted through "docker config". Volumes are locally stored which is quite troublesome in swarm environments and docker configs are read only thus every change has to be a seperate file.

Environment variables(and command arguments) can be updated through the swarm manager and are therefore a lot easier to maintain/update.

kwitsch commented 2 years ago

koanf has no support for nested maps -> this must be (somehow) implemented. Also we need a "special" logic to handle inline list definitions...

Nested maps aren't really a problem but slices are. 😕 My last attempt include a separate koanf config struct(with maps instead of slices) that would have been converted to the old config struct afterwards. This would produce a little overhead but ensure full backward compatibility.

Currently this feature has a low priority for me as there are some other tasks I like to solve first. 😅

0xERR0R commented 2 years ago

As far as i know it's the most common way to store configurations inside containers.

I agree with @kwitsch: env variables are the "recommended" way for containerized applications (see for example 12 factor app

In k8s it is already possible to provide the yaml configuration as a config map -> in this case it is possible to put all descriptors and configurations in one file.

Only with docker-compose there is a need to maintain 2 files. Maybe we can allow to define the whole config as one variable, something like this (didn't try it, but it is a valid docker compose file, I think it will work):

version: "2.1"
services:
  ubuntu:
    image: spx01/blocky
    environment:
      BLOCKY_CONFIG: |-
        upstream:
          default:
            - 5.9.164.112

Alternatively, it also possible to use something like this project: https://github.com/arribada/EnvToFile

kwitsch commented 2 years ago

The solution with one variable containing the whole config would be sufficient for my use case. It should be relatively easy to implement compared to a separated environment variable config.

tommyalatalo commented 2 years ago

@0xERR0R have you looked at viper for configuration? https://github.com/spf13/viper

I also concur with the request to configure using environment variables, that is definitely the common standard when configuring containers. Lists would be parsed as space separated strings or something like that, since there isn't a common way of entering that via env vars, but viper can load env var strings into many different data types, see: https://github.com/spf13/viper#getting-values-from-viper

You can also combine viper with cobra to manage cli flags as well: https://github.com/spf13/cobra

Using only one environment variable to manage the whole config is a very bad idea, it's a nightmare of yaml indentation and I've never seen anyone do that since it makes so much more sense to let every config parameter have its own env var. Environment variables are not made for indentation and formatting like yaml for instance and are not suitable for multi-line formatted configs.

kwitsch commented 2 years ago

Viper does nearly the same as koanf. The usage is a bit different but as far as I get it most of the problems currently considered regarding the implementation with koanf also persist with viper.

The solution with only one environment variable was suggested as quick solution and is definitely not best practice but the current config structure is a bit tricky to convert.

tommyalatalo commented 2 years ago

Viper does nearly the same as koanf. The usage is a bit different but as far as I get it most of the problems currently considered regarding the implementation with koanf also persist with viper.

The solution with only one environment variable was suggested as quick solution and is definitely not best practice but the current config structure is a bit tricky to convert.

Yeah I think the way forward is to just bite the bullet and rework the configuration management either way. Implementing all of the config as env vars + yaml is a project for sure, but it can be done and if you do it in a maintainable way you'll reap the rewards for the rest of the application's lifetime I'm sure.

I wonder if the quick fix with the single env var for all config would actually work at all, at the very least you would need to parse newlines and whitespace to assemble the yaml from the scalar blocked string passed to the variable... personally I would suggest not spending time on this kind of hack and instead focus all efforts on a proper solution instead.

kwitsch commented 2 years ago

Looked into this issue again and figured out a way converting map[string]interface{} to []interface{} using a decode hook function.

This resolves one of two main problems using koanf for the existing config struct.

The second problem would be the custom conversions happening in the UnmarshalYAML methods. It should be possible to also use a decode hook for this by identifying if the decode target implements the method and calling it afterwards with the source data. Still have to figure out how to correctly call the methods though.😅

kwitsch commented 2 years ago

Update: Got it working in a unit test with only tag changes on the config struct.

As koanf uses different tag options it would be reasonable to also load yaml files with koanf and replace the current yaml load with it. This would reduce the tag overhead.

Still need some development and testing before a merge request but I'm still working on it. 😅

kwitsch commented 2 years ago

Update 2: Reading yaml files with koanf is possible but it doesn't auto execute UnmarshalYAML functions. The hook which i wrote shown some problems with values from yaml files(environment source worked well).

There are two solutions to solve this:

  1. use yaml for files & koanf for environment which would result in double tagging mos values
  2. use only koanf for all sources and rewrite the UnmarshalYAML functions to koanf hooks

Option 1 would be way faster to implemtn(actually i just have to revert some commits) but code wise option 2 would be way cleaner. I stick with option 2 as maintainable code should always be a high priority. 😉

May take some time as there are quite some functions to rewrite. I will post updates as seams fit. 😅

ThinkChaos commented 2 years ago

Yeah koanf not supporting UnmarshalYAML is unfortunate but makes sense since values don't always come from YAML (env for instance), though I'm sure you understand that, mostly saying it for others reading this.
I've used koanf at work, and what I did was avoid having values that need to be parsed by structuring the config more: user@domain would be {user: "", domain: ""} for instance; and having a validation pass after Koanf.UnmarshalWithConf that can be (ab)used as a replacement: check user@domain format and set struct fields with the values.

I do think koanf is worth it, and if you'd like help with anything would gladly contribute.
One thing I wrote for work that could be interesting (closed source unfortunately) is a way to generate a pflag.FlagSet from a struct (using mapstructure which koanf already depends on). A FlagSet can be used by koanf, so you get CLI flags generated from the same source as the env and file config options. Supporting a generic flag like --option=abc.xyz=val would also work but you don't get exhaustive documentation in --help.

kwitsch commented 2 years ago

My goal is to implement it without config structure changes as they would break backwards compatibility. I use koanf myself in other projects but it's a bit of a challenge if it's included later an instead of the beginning. 😅

My next move would have been to replace the UnmarshalYAML functions wit mapstructure.DecodeHooks. Figured that this would be quiet some work... So yeah help and fresh ideas are always appreciated. 👍

Could you maybe tell me if my general usage of hook compose is correct? The linked repo contains most of my current development so feel free to look into it. 😅

My current problem is that some unit tests don't pass as a Duration won't be converted correctly. Most likely it is a problem with my unmarshalYAMLHookFunc but I currently don't see it. 😓

tommyalatalo commented 2 years ago

I would say that you would be much better off just starting off clean with koanf instead of jumping through hoops for backwards compatibility. It doesn't make sense to force a previous solution into a new one if the shoe doesn't fit, especially since you can already tell that you're heading into headache territory. Sure there will be some people who pull latest that might have some issues, but to remedy that you can put a big disclaimer in the readme.

kwitsch commented 2 years ago

Completely rewriting the config struct in favor of better koanf compatibility isn't an option I consider worthy. It would require changes in most other packages and unit tests.

I believe that decode hooks should do the trick in an less intrusive way. This would be backwards compatible and a further advantage of this approach would be that all changes are inside the config package as the structs won't change.

It's simply breaks down to more hooks vs more changes in other packages. Time wise it shouldn't be much of a difference.