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

Add DomainStrategy to JSONv5 outbound #2550

Merged
merged 3 commits into from
Nov 19, 2023
Merged

Conversation

57a4c324e742
Copy link
Contributor

@57a4c324e742 57a4c324e742 commented Jun 4, 2023

Added domainStrategy field for outbound in JSONv5 configuration file. This feature was introduced in #2334 and is only available in the JSONv4 configuration format.

This pull request:

Credits:

Usage:

{
	"outbounds": [
		{
			"protocol": "freedom",
			"tag": "freedom",
			"domainStrategy": "USE_IP"
			// USE_IP, Use_IP, UseIP, USE_IPV4, Use_IPv4, UseIPv4, USE_IPV6, Use_IPv6, UseIPv6
		}
	]
}

I was not able to create single commit by using web editor, please consider using squash when merging this pull request.

Copy link
Contributor

@Vigilans Vigilans left a comment

Choose a reason for hiding this comment

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

#2334 Adds DomainStrategy for outbound's SenderSettings, which coexists with freedom's Config in JsonV4. Now both is absent in JsonV5.

Do you think in this PR we should add both config for JsonV5 for compatibility, or just add to outbound's SenderSettings to advocate the usage of newer generic one?


Edit: 40d75fa actually lacks a whole bunch of freedom outbound config in JSONv4:

v4:

message Config {
enum DomainStrategy {
AS_IS = 0;
USE_IP = 1;
USE_IP4 = 2;
USE_IP6 = 3;
}
DomainStrategy domain_strategy = 1;
uint32 timeout = 2 [deprecated = true];
DestinationOverride destination_override = 3;
uint32 user_level = 4;
}

v5:

message SimplifiedConfig {
option (v2ray.core.common.protoext.message_opt).type = "outbound";
option (v2ray.core.common.protoext.message_opt).short_name = "freedom";
}

So bringing freedom's config back may be done in a separated PR.

@57a4c324e742
Copy link
Contributor Author

@Vigilans The new configuration format already brought us lot of breaking change, so I'm fine with the new unified one without the compatible design.

@AkinoKaede
Copy link
Contributor

AkinoKaede commented Jul 29, 2023

I think it's better to throw an exception when domainStrategy is set to an incorrect value to alert the user.

Copy link
Contributor

@xiaokangwang xiaokangwang left a comment

Choose a reason for hiding this comment

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

I think this merge request can be merged in principle, however, some modification will be needed to make sure there is only one recognized value for each options and throws explicit errors otherwise.

@xiaokangwang xiaokangwang added the Extensive Review Required Require an extensive review from organization owner, cannot be merged without owner approval label Nov 19, 2023
@xiaokangwang
Copy link
Contributor

I will merge and make modification myself.

@xiaokangwang xiaokangwang merged commit b583ef4 into v2fly:master Nov 19, 2023
36 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extensive Review Required Require an extensive review from organization owner, cannot be merged without owner approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DomainStrategy Setting Has no Effect in Freedom Outbound while Using jsonv5 Format
4 participants