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

nathole need a ip filter for zerotiler-like virtual network[Feature Request] #3717

Open
1 of 11 tasks
yangyaofei opened this issue Oct 23, 2023 · 8 comments
Open
1 of 11 tasks
Labels

Comments

@yangyaofei
Copy link

yangyaofei commented Oct 23, 2023

Describe the feature request

Like this issue: #3457, the zerotier ip has a low TTL.
So , the nathole will use zerotier for transmission. But the zerotier can be very slow.

I think add a config nat_address_filter like 192.168.0.0/24 which will remove these ips from nathole.

To test that thought, I add several code in https://github.com/fatedier/frp/blob/dev/client/visitor/xtcp.go#L278:

func (sv *XTCPVisitor) makeNatHole() {
	xl := xlog.FromContextSafe(sv.ctx)
	xl.Trace("makeNatHole start")
	if err := nathole.PreCheck(sv.ctx, sv.helper.MsgTransporter(), sv.cfg.ServerName, 5*time.Second); err != nil {
		xl.Warn("nathole precheck error: %v", err)
		return
	}

	xl.Trace("nathole prepare start")
	prepareResult, err := nathole.Prepare([]string{sv.clientCfg.NatHoleSTUNServer})
	if err != nil {
		xl.Warn("nathole prepare error: %v", err)
		return
	}
        // Simple Filter one IP
	var ipAddress = "172.16.1.70"
	var filteredAddrs []string
	for _, addr := range prepareResult.AssistedAddrs {
		addrParts := strings.Split(addr, ":")
		if addrParts[0] != ipAddress {
			filteredAddrs = append(filteredAddrs, addr)
		}
	}
	prepareResult.AssistedAddrs = filteredAddrs    

I test the code above, it will make frp go direct not through zerotier.

Describe alternatives you've considered

The things I don't know is that if this is a good implement or not. Totally newbie for go.

So if this is good and this config make sense, I will make a PR for this.

Affected area

  • Docs
  • Installation
  • Performance and Scalability
  • Security
  • User Experience
  • Test and Release
  • Developer Infrastructure
  • Client Plugin
  • Server Plugin
  • Extensions
  • Others
@fatedier
Copy link
Owner

I don't understand why you would want to use both zerotier and frp at the same time. Zerotier already supports peer-to-peer (p2p) connections, so I don't see the need to introduce more configuration complexity by using frp.

@yangyaofei
Copy link
Author

@fatedier zerotier rubost and easy to use for a group of people to connect each others. I use it for ssh.

but it is not stable, and slow due to the reason that we all know. And it cannot make nat hole in my network(but frp can). So,when we need to debug or transfer files or deploy service from Lan to web site, we will use frp.

I need them all😂😂😂

@fatedier
Copy link
Owner

I tend to put it aside first and wait for more feedback on this scenario.

@yangyaofei
Copy link
Author

Can I make PR for this?

@fatedier
Copy link
Owner

fatedier commented Oct 23, 2023

It will not accepted until we have clear and meaningful problem scenarios that are helpful to most people, especially when new configuration items need to be added.

The scenario you described, from my personal perspective, does not seem worth adding new configuration options to solve the problem. However, I am unsure if this is a common requirement for disabling internal addresses in AssistedAddrs(something like DisableInnerAssistedAddrs) , so I will temporarily leave it here and wait for more feedback from users.

@yangyaofei
Copy link
Author

ok, I think most users can't notice this. This (wrong nathole) will disappear in lots of circumstances. I think it more like a performance bug (but it's a feature).

I will fork this for my own usage, pin me if you need a PR.

@fatedier
Copy link
Owner

@yangyaofei BTW, I think you can consider whether there is a possibility of optimization without introducing new configuration items.

I prefer not to expose complexity to users as it may increase their understanding cost. Many users may also misuse it, resulting in various issues being created, which will ultimately lead to high maintenance costs for the project. Not just this one feature, I mean various kinds of requirements. Some features may only be used by a few people in the end, but in order to ensure the correctness of each refactoring/iteration, it is necessary for developers to invest in long-term maintenance costs.

@yangyaofei
Copy link
Author

I understand this philosophy but not fully agree. I think smart optimization may lead some unknown issues and hard to find and resolve.

Maybe add a config like enableSmartNatHole to enable is a good way.

yangyaofei added a commit to yangyaofei/frp that referenced this issue Oct 24, 2023
yangyaofei added a commit to yangyaofei/frp that referenced this issue Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Todo
Development

No branches or pull requests

2 participants