-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix DNS servers with same tag wrongly merged #3005
Fix DNS servers with same tag wrongly merged #3005
Conversation
func mergeSameTag(s []interface{}, path string) ([]interface{}, error) { | ||
// from: [a,"",b,"",a,"",b,""] | ||
// to: [a,"",b,"",merged,"",merged,""] | ||
merged := &struct{}{} | ||
tagKeyOnly := false | ||
// See https://github.com/v2fly/v2ray-core/issues/2981 | ||
if strings.HasPrefix(path, ".dns.servers") { | ||
tagKeyOnly = true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For certain settings (identified by path), do not use "tag"
field to search for tag. "_tag"
is preserved.
// from: [a,"",b,"",a,"",b,""] | ||
// to: [a,"",b,"",merged,"",merged,""] | ||
merged := &struct{}{} | ||
tagKeyOnly := false | ||
// See https://github.com/v2fly/v2ray-core/issues/2981 | ||
if strings.HasPrefix(path, ".dns.servers") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to documentation, mergeSameTag
should only be applied on inbounds
and outbounds
.
This looks like a "workaround" instead of a real fix for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to apply it to only inbounds
and outbounds
, but it will actually break this test that also applies to routing config:
v2ray-core/infra/conf/merge/merge_test.go
Lines 60 to 99 in 26ea22a
func TestMergeTag(t *testing.T) { | |
json1 := ` | |
{ | |
"routing": { | |
"rules": [{ | |
"tag":"1", | |
"inboundTag": ["in-1"], | |
"outboundTag": "out-1" | |
}] | |
} | |
} | |
` | |
json2 := ` | |
{ | |
"routing": { | |
"rules": [{ | |
"_tag":"1", | |
"inboundTag": ["in-2"], | |
"outboundTag": "out-2" | |
}] | |
} | |
} | |
` | |
expected := ` | |
{ | |
"routing": { | |
"rules": [{ | |
"tag":"1", | |
"inboundTag": ["in-1", "in-2"], | |
"outboundTag": "out-2" | |
}] | |
} | |
} | |
` | |
m, err := merge.JSONs([][]byte{[]byte(json1), []byte(json2)}) | |
if err != nil { | |
t.Error(err) | |
} | |
assertResult(t, m, expected) | |
} |
I think this merge request is ready to be merged. To be fair I didn't fully understood the implementation details, but the test seems fine. |
Fixes #2981