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

feat: support override in config file #1565

Open
wants to merge 3 commits into
base: Alpha
Choose a base branch
from

Conversation

Benyamin-Tehrani
Copy link

@Benyamin-Tehrani Benyamin-Tehrani commented Oct 4, 2024

Intro
Support override section in config file. It contains the configurations that will be merged into the original one, and determine whether to merge or not based on the given conditions.

Solved issue #1476

Format

override:
  - os: windows         # optional
    arch: amd64         # optional
    hostname: xxxx      # optional
    username: xxxx      # optional
    list-strategy: xxx  # optional, insert-front/append/override  (list merge strategy)
    content:
      # configs that will be merged into the original one
      mixed-port: 1234
      allow-lan: true
      rules:
         - DOMAIN-SUFFIX, my-pc-specific.com, Proxy
      ...

Available Conditions

  • os
  • arch
  • hostname
  • username
  • ... (can add more conditions)

Example

mixed-port: 7890
ipv6: true
log-level: debug
allow-lan: false
unified-delay: false
tcp-concurrent: true
external-controller: 127.0.0.1:9090
nameserver:
  - "1.1.1.1"
tun:
  enabled: false

......

override:
  # My home network has bad IPv6 condition
  - os: windows
    hostname: LAPTOP-XXXXX
    content:
      ipv6: false
  # Allow LAN & enable external controller with ui on my AWS cloud server
  - os: linux
    hostname: aws-xxxx
    content:
      allow-lan: true
      external-controller: :9090
      external-ui: ui
      external-ui-url: "https://github.com/MetaCubeX/metacubexd/archive/refs/heads/gh-pages.zip"
  # Some modify for my Android phone
  - os: android
    list-strategy: override
    content:
      find-process-mode: always
      tun:
        enabled: true
      nameserver:
        - 8.8.8.8
  # Add custom rules and hosts for the PC in my lab
  - os: windows
    username: lab-user-xxxx
    list-strategy: insert-front
    content:
      hosts:
        server.xxlab.lan: 192.168.3.12
        calc.xxlab.lan: 192.168.3.14
      rules:
        - DOMAIN, storage.xxlab.lan, Socks5-LAB

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Oct 4, 2024

@Benyamin-Tehrani Thanks, but you need run go mod tidy before submit PR

@Benyamin-Tehrani
Copy link
Author

Thanks, I forgot that. Force-pushed

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Oct 4, 2024

According to the mergo documentation, zero values ​​will not be overwritten. This will lead to a defect that a string cannot be overwritten to empty, and an int cannot be overwritten to 0. It may be difficult for users to understand and accept this behavior?

@Benyamin-Tehrani
Copy link
Author

Sure, I have thought about this problem. Maybe some workaround like "key: nil" means delete & "key: 0" means override is needed.

BTW, it's not easy to find out an scenario that need to overwrite an existing value with 0 or ""?
Perhaps this issue will have little impact, although it does exist.

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Oct 4, 2024

At the same time, golang will also treat false as zero value. These restrictions will greatly increase the mental burden of users when using it. For example, in certain circumstances, set allow-lan to false, or in certain circumstances, set external-controller to empty to turn off this function. The above are just two examples to prove that such scenarios are not difficult to find.

@Benyamin-Tehrani
Copy link
Author

Oh, I have found another params of mergo about EmptyValue overwrite policy. I'll look into it.

@Benyamin-Tehrani
Copy link
Author

This is a bit of a tricky one. In essence the problem is that golang doesn't use nil as a common null representation, while string & numbers still use zero as their nil. So it's hard for libraries to distinguish these types of nulls from zeroes. omitempty cannot work here either.

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Oct 4, 2024

This is why hub/route/configs.go repeatedly defines a large number of configuration types and changes the attributes to pointers to overwrite them one by one.

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Oct 4, 2024

Perhaps patching the original yaml directly can avoid falling into golang's zero value problem, but it should be noted that the existing order needs to be kept unchanged when serializing and deserializing the map in yaml.

@Benyamin-Tehrani
Copy link
Author

This is why hub/route/configs.go repeatedly defines a large number of configuration types and changes the attributes to pointers to overwrite them one by one.

I can also do this for RawConfig if you guys don't mind 🤣 since it's not used at anywhere else but only in config.go.

This should be the easiest way, I think, values that don't exist in yaml will be set to nil pointers. Then just merge with mergo normally, and those not modified will be ignored as they're nil pointers. Is it right?

I have not read the code in depth. Maybe patch in hub/route/config could be simplified with the help of mergo in this way too.

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Oct 4, 2024

In fact, the value of RawConfig is widely used in other GUI client projects. At the same time, the DefaultRawConfig function defines many special initial values ​​for RawConfig. The maintenance cost of significantly modifying its definition is unacceptable.

@Benyamin-Tehrani
Copy link
Author

Benyamin-Tehrani commented Oct 6, 2024

Finally figured out a way to solve this problem. mergo is not needed any more, but make a little changes to go-yaml to support custom list merge strategy.

Idea:

Process config override in yaml.Unmarshal, thus to avoid the zero-value problem about structs.

  1. Normally yaml.Unmarshal the whole *RawConfig from yaml
  2. In ApplyOverride, use yaml.Marshal to marshal each override.Content back to yaml-text
  3. Call yaml.Unmarshal again, unmarshal the override yaml-text into *RawConfig, with custom list merge strategy options passed.

All tests are passed.

Modified go-yaml:

I have forked go-yaml from another fork in its pending PRs, then add custom list decode options to support custom list merge strategy, while fix all errors about import and tests.

The repo is https://github.com/Benyamin-Tehrani/yaml. Maybe it's better to move this repo into MetacubeX for easier maintenance in the future.

Considering that the original go-yaml repo has not been maintained for two years, this should not be a problem, I think :-P

All tests in this go-yaml repo is passed too.

@Skyxim
Copy link
Collaborator

Skyxim commented Oct 6, 2024

It might be more reasonable to separate the library responsible for merging and generating configurations. After all, the Core does not handle multiple configuration files, and relying on an overwrite mechanism within a single configuration seems less practical than directly managing the original values.

Sorry, I misunderstood the purpose of this issue.

@Benyamin-Tehrani
Copy link
Author

Benyamin-Tehrani commented Oct 6, 2024

It might be more reasonable to separate the library responsible for merging and generating configurations. After all, the Core does not handle multiple configuration files, and relying on an overwrite mechanism within a single configuration seems less practical than directly managing the original values.

As written in the example, override is mainly used to fine-tune the configuration file on specific devices. With this mechanism, it is possible to simply host one copy of config on public (like: secret gist) for subscription, and then directly uses the subscription link on all devices. (like PCs/Android Phone/Mac/Lab workstations)

It eliminates the need to save the config file on each device individually, and meanwhile ease the synchronization of the configs across multiple devices.

In addition, this feature gives support to configuration overriding for all clients, no longer need to set override settings seperately on all devices :-P

@Benyamin-Tehrani
Copy link
Author

Benyamin-Tehrani commented Oct 9, 2024

Any process on this PR? ;-) @wwqgtxx
Sorry for the unintentional interrupt.

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Oct 9, 2024

This implementation looks good, but I think we can refer to the syntax of mihomo-party and simplify the insert-front/append/override syntax to: https://mihomo.party/docs/guide/override/yaml#%E8%A6%86%E5%86%99%E8%BF%90%E8%A1%8C%E9%80%BB%E8%BE%91

@Benyamin-Tehrani
Copy link
Author

Benyamin-Tehrani commented Oct 9, 2024

This implementation looks good, but I think we can refer to the syntax of mihomo-party and simplify the insert-front/append/override syntax to: https://mihomo.party/docs/guide/override/yaml#%E8%A6%86%E5%86%99%E8%BF%90%E8%A1%8C%E9%80%BB%E8%BE%91

The mihomo party's design is indeed clearer, but it would require a complete reimplementation of the deep merge logic I think ;-) . It looks like the "merge option" may appear at any depth layer of the yaml config?

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Oct 9, 2024

It looks like the "merge option" may appear at any depth layer of the yaml config?

This is indeed the case. This should not be a big problem for modifying the YAML library. The deep deserialization is done by recursive calls.

@Benyamin-Tehrani
Copy link
Author

@wwqgtxx I suddenly realized that the config with key begins with + fuzzy matching seems to be ambiguous when being parsed or written🤔
Like:

nameserver-policy:
  +.google.cn:
    - 8.8.8.8#Proxy
    - 1.1.1.1#Proxy

@wwqgtxx
Copy link
Collaborator

wwqgtxx commented Oct 11, 2024

I suddenly realized that the config with key begins with + fuzzy matching seems to be ambiguous when being parsed or written🤔 Like:

nameserver-policy:
  +.google.cn:
    - 8.8.8.8#Proxy
    - 1.1.1.1#Proxy

Perhaps we can replace + with ^. In the current YAML configuration, there should be no map key that starts or ends with ^, and it does not conflict with other YAML syntax, like:

^nameserver-policy:
  +.google.cn:
    - 8.8.8.8#Proxy
    - 1.1.1.1#Proxy

and

nameserver-policy^:
  +.google.cn:
    - 8.8.8.8#Proxy
    - 1.1.1.1#Proxy

@pompurin404
Copy link

pompurin404 commented Oct 11, 2024

I'd like to share some ideas for your consideration (Syntax fix only for keys starting with +)

nameserver-policy:
  # override
  <+.google.cn>:
    - 8.8.8.8#Proxy
    - 1.1.1.1#Proxy
  # prepend
  +<+.google.cn>:
    - 8.8.8.8#Proxy
    - 1.1.1.1#Proxy
  # append
  <+.google.cn>+:
    - 8.8.8.8#Proxy
    - 1.1.1.1#Proxy

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.

4 participants