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.

Sign up for GitHub

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

Jump to bottom

WireGuard support #1209

Closed
wants to merge 1 commit into from
Closed

WireGuard support #1209

wants to merge 1 commit into from
+15 −0

Conversation

b-m-f
Copy link

@b-m-f b-m-f commented Oct 25, 2022

Draft PR to get containers/netavark#450 integrated into Podman

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: b-m-f
Once this PR has been reviewed and has the lgtm label, please assign luap99 for approval by writing /assign @luap99 in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan
Copy link
Member

rhatdan commented Oct 25, 2022

@Luap99 @mheon PTAL

@b-m-f b-m-f force-pushed the wireguard-support branch 4 times, most recently from 3af94d1 to 9ab80a6 Compare October 25, 2022 15:51
Luap99
Luap99 reviewed Oct 25, 2022
View reviewed changes
libnetwork/types/network.go Outdated
Comment on lines 216 to 218
// Path to WireGuard configuration.
// Will only be used when the network driver is WireGuard
WireGuardConfigPath string `json:"wireguard_config_path"`
Copy link
Member

Choose a reason for hiding this comment

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

This is IMO not sustainable, we should not add driver specific options here. This is exposed via podman API.

I think we should add a generic options field as map[string]string where we could in future add more driver specific fields Without having to update the struct every single time.

Copy link
Member

Choose a reason for hiding this comment

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

Similar to what we're doing in containers.conf for runtimes?

https://github.com/containers/common/blob/main/pkg/config/config.go#L358-L359

Copy link
Author

Choose a reason for hiding this comment

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

Updated this with map[string]string. Multiple values could still be separated by a comma and parsed before they are used

@b-m-f b-m-f force-pushed the wireguard-support branch 2 times, most recently from e25cc42 to 5213206 Compare October 26, 2022 10:27
Luap99
Luap99 reviewed Oct 26, 2022
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Please squash your commits into one and make sure to sign it, see: https://github.com/containers/common/blob/main/CONTRIBUTING.md#sign-your-prs

@b-m-f b-m-f force-pushed the wireguard-support branch 2 times, most recently from 92b219a to c8a8181 Compare October 26, 2022 12:13
@b-m-f
Copy link
Author

b-m-f commented Oct 26, 2022

Please squash your commits into one and make sure to sign it, see: https://github.com/containers/common/blob/main/CONTRIBUTING.md#sign-your-prs

All done.
I was thinking to keep this PR open while I dig into netavark. Might need more changes

@b-m-f b-m-f mentioned this pull request Oct 27, 2022
Closed
@rhatdan
Copy link
Member

rhatdan commented Nov 11, 2022

@b-m-f How is it going? Making any progress?

@b-m-f
Copy link
Author

b-m-f commented Nov 12, 2022 •
edited
Loading

Hi @rhatdan,

the bulk of the work is done. I think I should be able to click the ready for review button tmrw.

2 tests and documentation are still missing and containers/netavark#472 has a lot of changes, so I would do the documentation once things are about to be merged :)

@b-m-f
- adds WireGuard as new network driver for bridge networks
fce343d 
- adds new  type to PerNetwork Options. This way networks can have options passed into them through the cli easily in the future

Signed-off-by: b-m-f <[email protected]>
@b-m-f b-m-f force-pushed the wireguard-support branch from c8a8181 to fce343d Compare November 13, 2022 09:35
@b-m-f b-m-f mentioned this pull request Nov 13, 2022
Closed
@b-m-f
Copy link
Author

b-m-f commented Nov 13, 2022

This PR is now ready for review.

This feature is spread over 3 PRs:

@b-m-f b-m-f marked this pull request as ready for review November 13, 2022 09:37
@rhatdan
Copy link
Member

rhatdan commented Nov 13, 2022

@Luap99 @mheon @baude @flouthoc PTAL

flouthoc
flouthoc reviewed Nov 14, 2022
View reviewed changes
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

I have a small question why is this being implemented as Wiregaurd driver would it make more more sense if it is just VPNDriver and VPNDriver implements Wiregaurd and that is configurable ? In future the design can be extended for OpenVPN.

@flouthoc
Copy link
Collaborator

Also a comment here: containers/netavark#472 (review)

@Luap99
Copy link
Member

Luap99 commented Nov 14, 2022

/hold
This should not be merged until the netavark PR is merged.

@rhatdan
Copy link
Member

rhatdan commented Dec 19, 2022

@b-m-f Are you still working on this?

@b-m-f
Copy link
Author

b-m-f commented Dec 22, 2022

@rhatdan the work is pretty much done from my side.
@baude mentioned a new plugin system for netavark.
Once that lands I'd need to rebase my work on top of it and this PR will stay relevant afaik.

@b-m-f
Copy link
Author

b-m-f commented May 2, 2023 •
edited
Loading

Implemented with netavarks plugin feature and https://github.com/b-m-f/netavark-wireguard-plugin

@b-m-f b-m-f closed this May 2, 2023
@b-m-f b-m-f deleted the wireguard-support branch May 2, 2023 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhatdan rhatdan

@flouthoc flouthoc

@vrothberg vrothberg

@Luap99 Luap99

Assignees
No one assigned
Labels
do-not-merge/hold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@b-m-f @rhatdan @flouthoc @Luap99 @vrothberg