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

Enable real forwarding for wireguard service #596

Merged
merged 3 commits into from Dec 6, 2018
Merged

Conversation

soffokl
Copy link
Member

@soffokl soffokl commented Dec 6, 2018

No description provided.

@soffokl soffokl added this to the Keliukis (0.5) milestone Dec 6, 2018
@soffokl soffokl self-assigned this Dec 6, 2018
@@ -99,6 +117,10 @@ func (ce *connectionEndpoint) Stop() error {
return err
}

if ce.natService != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

can it be nil ? its depedency

Copy link
Member Author

Choose a reason for hiding this comment

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

connectionEndpoint is used both for provider and consumer.
There is no reason to do anything with NAT for the consumer.
So the answer is yes, it can be nil for the consumer.

Copy link
Contributor

Choose a reason for hiding this comment

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

That means that connection endpoint is too low level component to handle forwarding. Behaviour based on ifs is not a very good practice

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved NAT configuration to the service manager.

@@ -68,6 +63,29 @@ func (ce *connectionEndpoint) Start(config *wg.ServiceConfig) error {
ce.privateKey = config.Consumer.PrivateKey
}

if ce.ipResolver != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This shows too much details - could be separate private function with clear name

zolia
zolia previously approved these changes Dec 6, 2018
Copy link
Contributor

@zolia zolia left a comment

Choose a reason for hiding this comment

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

LGTM for now. I see that we will need to move all iptables code into separate service common to any service using iptables. This will be needed to resolve possible rule clash'es. Will create a separate ticket for this.

@soffokl
Copy link
Member Author

soffokl commented Dec 6, 2018

Fixed tests and rebased it. Please take a look again.

Copy link
Contributor

@tadovas tadovas left a comment

Choose a reason for hiding this comment

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

Good enough. Let's do this

@tadovas
Copy link
Contributor

tadovas commented Dec 6, 2018

@zolia @vkuznecovas lets decide

@tadovas tadovas merged commit 21bbd8a into master Dec 6, 2018
@tadovas tadovas deleted the wireguard-forwarding branch December 6, 2018 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants