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

new server option to specify protocol #208

Merged
merged 4 commits into from Mar 19, 2018

Conversation

Projects
None yet
3 participants
@zolia
Copy link
Member

commented Mar 16, 2018

No description provided.

@@ -43,6 +45,17 @@ func (service *serviceIPTables) Stop() error {
}

func (service *serviceIPTables) enableIPForwarding() (err error) {
out, err := exec.Command("sysctl", "-n", "net.ipv4.ip_forward").CombinedOutput()

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 16, 2018

Member

Would be nice to that enableIPForwarding would have 1 responsibility.
Suggest to move this logic to another function e.g. isIPForwardingEnabled()

@@ -17,3 +17,6 @@ MYSTERIUM_DISCOVERY_ADDRESS: ""

# Mysterium broker address (eg.: testnet.mysterium.network)
MYSTERIUM_BROKER_ADDRESS: ""

# Mysterium server protocol (options: {udp, tcp})
MYSTERIUM_SERVER_PROTOCOL: ""

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 16, 2018

Member

Wrong syntax here VAR: "value" should be VAR="value"

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 16, 2018

Member

But @tadovas is dropping container ENV variables, dont spent time on introducing new ones

@@ -104,6 +106,13 @@ func ParseArguments(args []string) (options CommandOptions, err error) {
"Address (URL form) of ipify service",
)

flags.StringVar(
&options.Protocol,
"protocol",

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 16, 2018

Member

I would go with configuring style like this:

mysterium_server 
  --identity=..
  --openvpn.binary=openvpn
  --openvpn.proto=tcp

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 16, 2018

Member

Absolutely, lets look at options as variables. "protocol" is global and doesn't say anything about context it is used. openvpn.protocol cleary states that this option is for openvpn and has something to do with openvpn connect type

This comment has been minimized.

Copy link
@zolia

zolia Mar 16, 2018

Author Member

We might need to have it global, but since we don't have other transports yet, will do as you suggest.

zolia added some commits Mar 16, 2018

@zolia zolia requested review from Waldz and removed request for tadovas Mar 16, 2018

}

if strings.TrimSpace(string(out)) == "1" {
service.forward = true

This comment has been minimized.

Copy link
@Waldz

Waldz Mar 16, 2018

Member

Getter function IsEnabled() should only read, but not modify state

@zolia zolia requested review from tadovas and Waldz Mar 19, 2018

@Waldz

Waldz approved these changes Mar 19, 2018

@zolia zolia merged commit 1cf143a into master Mar 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@zolia zolia deleted the feature/MYST-433-server-protocol-option branch Mar 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.