-
Notifications
You must be signed in to change notification settings - Fork 70
Sub domain replace feature #307
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
base: master
Are you sure you want to change the base?
Conversation
Update new feature: subdomain replace
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.
Does this actually do anything that the existing "replace" doesn't? https://github.com/folbricht/routedns/blob/master/doc/configuration.md#Replace looks more flexible as it's possible to replace any part of the domain using regexes
No, it doesn't but it is simpler & faster which is why I created it |
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.
One thing I'm not to fond of is the same. This isn't replacing "sub"-domains, but actually it's replacing domains. Should this be called domain-replace
perhaps?
Also, could you add a section to the documentation? This is effectively a more efficient and simple replacing group, faster than the regexp replacer.
exp = append(exp, subDomainReplaceExp{o.From, o.To}) | ||
} else { | ||
return nil, errors.New("{} not found") | ||
} |
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.
Do you still need this since {}
isn't necessary in the config anymore?
func (r subDomainReplaceExp) substitute(qname string) (string, bool) { | ||
fromDomain := r.from | ||
toDomain := r.to | ||
// from {}.ctptech.dev to {}.charlesp.tech |
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.
// from {}.ctptech.dev to {}.charlesp.tech | |
// from .ctptech.dev to .charlesp.tech |
} else { | ||
return "", false | ||
} |
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.
} else { | |
return "", false | |
} | |
} | |
return "", false |
// from {}.ctptech.dev to {}.charlesp.tech | ||
// from test.ctptech.dev to test.charlesp.tech | ||
if strings.HasSuffix(qname, fromDomain) { | ||
str := strings.Replace(qname, fromDomain, toDomain, 1) |
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 think there's a small issue with just doing a simple replace since the "fromDomain" string can show up more than once in the domain. Let's say you have fromDomain = .local.
and the domain name being looked up is example.local.something.local.
then it'd replace the first occurrance. It might be best to simply strip the fromDomain
with strings.TrimSuffix()
, then add the toDomain
value to the result with +
like so:
sub := strings.TrimSuffix(qname, fromDomain)
str := sub+toDomain
return str, true
For sure |
If you want to use |
No doc or unit test done yet will provide when time is abundant