-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Override outbounds in multi config files #2659
Conversation
infra/conf/xray.go
Outdated
if idx := c.findOutboundTag(o.OutboundConfigs[0].Tag); idx > -1 { | ||
c.OutboundConfigs[idx] = o.OutboundConfigs[0] | ||
ctllog.Println("[", fn, "] updated outbound with tag: ", o.OutboundConfigs[0].Tag) | ||
outboundPrepends := o.OutboundConfigs |
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.
why do you need outboundPrepends? this logic doesn't seem right to me.
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.
as you know when file name includes "tail", outbound configs will append to original config and if not, the override outbounds should prepend to original config.
in several test cases some outbounds of the original config will update if they have the same Tag name in :
c.OutboundConfigs[idx] = o.OutboundConfigs[i]
so if i use c.OutboundConfigs for prepend instead of outboundPrepends
, it may cause of duplicate outbound with same tag name in final config.
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.
o.OutboundConfigs are incoming data, c.OutboundConfigs are existing data. While we iterate through o.OutboundConfigs there are three case:
- tag match, replace one of existing
- append
- prepend
1 and 3 should not both happen for the same entry. Why it create duplicates?
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.
@yuhan6665 actually after write a lot of explanations ,suddenly realized what happened here. i think outboundPrepends
seems reasonable now :)
Looks good. Thanks! |
seems override outbounds should also consider in last PR #2655