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

support outbound-addr #7216

Merged
merged 1 commit into from
Aug 9, 2020
Merged

support outbound-addr #7216

merged 1 commit into from
Aug 9, 2020

Conversation

5eraph
Copy link
Contributor

@5eraph 5eraph commented Aug 4, 2020

Changes to support binding to specific outbound address. Mentioned in #6064. Follow ups to #6324 and #6965.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 4, 2020
@5eraph 5eraph force-pushed the master branch 2 times, most recently from bff7100 to 49629c2 Compare August 4, 2020 13:46
zhangguanzhang
zhangguanzhang reviewed Aug 4, 2020
View reviewed changes
libpod/networking_linux.go Outdated Show resolved Hide resolved
libpod/networking_linux.go Outdated Show resolved Hide resolved
libpod/networking_linux.go Outdated Show resolved Hide resolved
libpod/networking_linux.go Outdated Show resolved Hide resolved
mheon
mheon reviewed Aug 4, 2020
View reviewed changes
libpod/networking_linux.go Outdated Show resolved Hide resolved
@5eraph 5eraph force-pushed the master branch 2 times, most recently from 66f0ab9 to 0fc4b14 Compare August 4, 2020 14:47
@zhangguanzhang
Copy link
Collaborator

you must add the test/e2e/

@5eraph
Copy link
Contributor Author

5eraph commented Aug 4, 2020

Yes I know @zhangguanzhang, will do. I did not have time to test in my env so far, I will include tests later. That's why it is WIP so far. I did not expect that fast reviews.

Thank you both for reviews and suggestions.

@5eraph 5eraph force-pushed the master branch 5 times, most recently from a5f1f85 to 4f80b0e Compare August 4, 2020 18:27
@5eraph
Copy link
Contributor Author

5eraph commented Aug 5, 2020

The test I added yesterday is not correct. I will fix it today.

I wonder do we need ipv6 test as well? Slirp4netns does not test ipv6 yet.
Also any chance we can include this in version 2.1?

@5eraph 5eraph force-pushed the master branch 2 times, most recently from 15cebd1 to 76dcb56 Compare August 5, 2020 20:02
@5eraph
Copy link
Contributor Author

5eraph commented Aug 5, 2020

OK, I finished and tested changes and everything works as expected. But ... I am not sure about tests. We need to check whether we are running podman on slirp4netns with outbound-addr, but right now there is no exposed function in the test suite to do this. (Or did I miss it?) Anyway I implemented it with SystemExec and this should be sufficient for now, but it just does not feel clean enough. Any suggestions?

@mheon
Copy link
Member

mheon commented Aug 5, 2020

For testing - I would restrict testing to rootless only, which guarantees it will be run on a slirp4netns system. You'll probably have to use localhost as the source address, I don't think we can guarantee a stable external IP on the test systems.

@5eraph
Copy link
Contributor Author

5eraph commented Aug 5, 2020

Yes that is what I did in latest commit. Regarding restriction to rootless only --network slirp4netns should guarantee slirp4netns system or am I wrong?

@mheon
Copy link
Member

mheon commented Aug 5, 2020

Yes - you're right, that would guarantee slirp would be used, even as root

@5eraph 5eraph force-pushed the master branch 2 times, most recently from 399b43f to 2246be1 Compare August 6, 2020 18:29
@5eraph 5eraph changed the title WIP: support outbound-addr support outbound-addr Aug 6, 2020
@mheon
Copy link
Member

mheon commented Aug 7, 2020

/approve

Restarted tests in case they flaked.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2020
@mheon
Copy link
Member

mheon commented Aug 7, 2020

Tests seem green.
@rhatdan @baude @vrothberg @giuseppe PTAL

@mheon
Copy link
Member

mheon commented Aug 7, 2020

LGTM, for reference

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 5eraph, AkihiroSuda, mheon, zhangguanzhang

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

The pull request process is described 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

@5eraph 5eraph force-pushed the master branch 2 times, most recently from f8e2020 to ba3301d Compare August 7, 2020 15:38
@5eraph
Copy link
Contributor Author

5eraph commented Aug 7, 2020

Hi @TomSweeneyRedHat, reuqested docs changes implemented.

@TomSweeneyRedHat
Copy link
Member

tests aren't hip.
One possible small nit to chase if you're ambitious or have other changes to make.
LGTM otherwise.

@5eraph
Copy link
Contributor Author

5eraph commented Aug 7, 2020

@TomSweeneyRedHat comment updated. Well I do not have anything else for now which has to be included in this pull request, so from my side we are good to go, unless you would like to add something else ofc. 🙂

TomSweeneyRedHat
TomSweeneyRedHat reviewed Aug 7, 2020
View reviewed changes
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @5eraph !

@rhatdan
Copy link
Member

rhatdan commented Aug 9, 2020

/lgtm
Thanks @5eraph

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2020
@openshift-merge-robot openshift-merge-robot merged commit 95e2e15 into containers:master Aug 9, 2020
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@mheon mheon mheon left review comments

@zhangguanzhang zhangguanzhang zhangguanzhang approved these changes

Assignees

@mrunalp mrunalp

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@5eraph @zhangguanzhang @mheon @openshift-ci-robot @TomSweeneyRedHat @rhatdan @AkihiroSuda @mrunalp @openshift-merge-robot