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(dns): Support DNS queryStrategy config per NameServer. #2564

Merged

Conversation

cty123
Copy link
Contributor

@cty123 cty123 commented Sep 17, 2023

There are multiple feature requests already to enable queryStrategy configuration at the granularity of DNS name server: #1953 #1940. The PR adds a field queryStrategy in NameServer struct to allow user to configure DNS query strategy for every NameServer instead of using a single option for all name servers.

The configuration before looks likes this,

"dns": {
  "servers": [
    {
      ... NameServer1
    },
    {
      ... NameServer2
    }
  ],
  "queryStrategy": "UseIP6"
}

After this PR, users are allowed to specify the queryStrategy option for each of the NameServers, for example,

"dns": {
  "servers": [
    {
      ... NameServer1
      +"queryStrategy": "UseIP4"
    },
    {
      ... NameServer2
      +"queryStrategy": "UseIP6"
    }
  ],
  -"queryStrategy": "UseIP6"
}

To be compatible with the old configuration,

  1. The old queryStrategy field applied on the DNS config level still works. The NameServer level queryStrategy option can be treated as another filter. This should be pretty straight-forward, for example, in the following case, the queryStrategy would be set to UseIP6,
"dns": {
  "servers": [
    {
      ... NameServer1
      "queryStrategy": "UseIP6"
    },
  ],
  "queryStrategy": "UseIP"
}

but in the following case, the queryStrategy would always result in empty response, due to conflicting DNS option,

"dns": {
  "servers": [
    {
      ... NameServer1
      "queryStrategy": "UseIP6"
    },
  ],
  "queryStrategy": "UseIP4"
}

Therefore, users are encouraged to use NameServer level queryStrategy field.

  1. If queryStrategy is not set at NameServer level, the NameService will not override the queryStrategy setting. The behavior of the DNS resolver should remain the same as previous.

@yuhan6665
Copy link
Member

麻烦看一下 TestLocalNameServerWithIPv6Override 在 windows 这个测试

@@ -408,3 +414,24 @@ func (s *DoHNameServer) QueryIP(ctx context.Context, domain string, clientIP net
}
}
}

func (s *DoHNameServer) resolveIpOptionOverride(ipOption dns_feature.IPOption) dns_feature.IPOption {
switch s.queryStrategy {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it make sense to extract this method to a common place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a more general function under nameserver.go

@cty123 cty123 force-pushed the cty123/implement-dns-query-strategy-per-nameserver branch from 8507be4 to 422a15d Compare September 19, 2023 20:38
@cty123 cty123 force-pushed the cty123/implement-dns-query-strategy-per-nameserver branch from 422a15d to d52944f Compare September 19, 2023 21:03
@cty123
Copy link
Contributor Author

cty123 commented Sep 19, 2023

麻烦看一下 TestLocalNameServerWithIPv6Override 在 windows 这个测试

可能跟windows下go的DNS lookup实现有关系,macos和linux都能过这个测试。稍微看了下源码,

       if r.preferGoOverWindows() {
		return r.goLookupIP(ctx, network, name)
	}
	// TODO(bradfitz,brainman): use ctx more. See TODO below.

	var family int32 = syscall.AF_UNSPEC
	switch ipVersion(network) {
	case '4':
		family = syscall.AF_INET
	case '6':
		family = syscall.AF_INET6
	}
...

也有可能跟运行环境有关系,但是目前手上暂时没有windows的机器,所以没法测试。所以暂时移除了在local nameserver使用queryStrategy的功能。

@yuhan6665 yuhan6665 merged commit 4f6042c into XTLS:main Sep 22, 2023
35 checks passed
@yuhan6665
Copy link
Member

感谢佬!

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.

2 participants