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

feat: add user mode networking parameter for Podman machine #3251

Merged
merged 6 commits into from
Aug 14, 2023
Merged

feat: add user mode networking parameter for Podman machine #3251

merged 6 commits into from
Aug 14, 2023

Conversation

jeffmaury
Copy link
Contributor

@jeffmaury jeffmaury commented Jul 18, 2023 •
edited by dgolovin
Loading

Fixes #2865

What does this PR do?

Add a new parameter user-mode-networking when creating Podman machine. This parameter will be accessible only on Windows and for Podman 4.6.0

Screenshot/screencast of this PR

user-mode-networking-new

What issues does this PR fix or reference?

Fixes #2865

How to test this PR?

Create a Podman machine and set the new parameter

@jeffmaury jeffmaury requested review from a team and benoitf as code owners July 18, 2023 15:29
@jeffmaury jeffmaury requested review from cdrage and lstocchi and removed request for a team July 18, 2023 15:29
@benoitf
Copy link
Collaborator

benoitf commented Jul 18, 2023

should we display the parameter only if the version is supported ?

@jeffmaury
Copy link
Contributor Author

should we display the parameter only if the version is supported ?

I would love to :)

@afbjorklund
Copy link
Contributor

should we display the parameter only if the version is supported ?

You might want to do the same for "rootful" (which appeared in v4.1.0)

cdrage
cdrage requested changes Jul 31, 2023
View reviewed changes
extensions/podman/src/extension.ts Show resolved Hide resolved
extensions/podman/src/extension.ts Outdated
@@ -106,6 +110,7 @@ async function updateMachines(provider: extensionApi.Provider): Promise<void> {
memory: parseInt(machine.Memory),
cpus: machine.CPUs,
diskSize: parseInt(machine.DiskSize),
userModeNetworking: isWindows() ? machine.UserModeNetworking : true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have the isWindows outside the set.

ex:

const userModeNetworking : isWindows() ? machine.UserModeNetworking : true

and then add the param to the set

extensions/podman/src/extension.ts Outdated
@@ -899,11 +911,18 @@ export async function deactivate(): Promise<void> {
});
}

const PODMAN_MINIMUM_VERSION_FOR_USER_MODE_NETWORKING = '4.6.0';

function isUserModeNetworkingSupported(podmanVersion: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add comments that this is windows only. maybe we should specific the function name that it's windows related since that's the only one that supports it.

extensions/podman/src/extension.ts Outdated Show resolved Hide resolved
@jeffmaury
feat: add user mode networking parameter for Podman machine
c32a692 
Fixes containers#2865

Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@jeffmaury
fix: use when expression to control when user mode networking should …
942d5c8 
…be exposed

Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@jeffmaury jeffmaury requested a review from cdrage August 8, 2023 15:29
@jeffmaury
fix: fix failing test
376281d 
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
benoitf
benoitf reviewed Aug 10, 2023
View reviewed changes
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

Tested on a Windows VM

installed first Podman 4.5.0

Option is not displayed

image

Then click on 'Update to 4.6.0' button of Podman

Successfully installed

But then it doesn't show the option
image

and then restarted Podman Desktop

This time I got the flag

image

I think we should add a markdown description explaining that it's useful for VPN/DNS issues

And/Or add a pointer to https://docs.podman.io/en/latest/markdown/podman-machine-init.1.html#user-mode-networking

or just copy some part of the text either in a new section with "markdown" type or enrich the description because 'User network mode" won't be understood by people if it's just a toggle

@jeffmaury
fix: update expression when Podman is updated
ba0f147 
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@jeffmaury
fix: switch description to markdown
5e032d9 
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@jeffmaury
Copy link
Contributor Author

Tested on a Windows VM

installed first Podman 4.5.0

Option is not displayed

image

Then click on 'Update to 4.6.0' button of Podman

Successfully installed

But then it doesn't show the option image

and then restarted Podman Desktop

This time I got the flag

image

I think we should add a markdown description explaining that it's useful for VPN/DNS issues

And/Or add a pointer to https://docs.podman.io/en/latest/markdown/podman-machine-init.1.html#user-mode-networking

or just copy some part of the text either in a new section with "markdown" type or enrich the description because 'User network mode" won't be understood by people if it's just a toggle

Fixed by ba0f147 and 5e032d9

@benoitf
Copy link
Collaborator

benoitf commented Aug 10, 2023

I'll test but probably you need to update the body/description of the PR and the screencast as it's no longer using the warning box

benoitf added a commit to benoitf/desktop that referenced this pull request Aug 10, 2023
@benoitf
fix: add back 'Done' text on the button
32c7073 
fixes containers#3251
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
@benoitf benoitf mentioned this pull request Aug 10, 2023
Merged
benoitf added a commit to benoitf/desktop that referenced this pull request Aug 10, 2023
@benoitf
fix: add back 'Done' text on the button
7263a36 
fixes containers#3251
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
benoitf added a commit to benoitf/desktop that referenced this pull request Aug 10, 2023
@benoitf
fix: add back 'Done' text on the button
bf936cc 
fixes containers#3251
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
benoitf added a commit to benoitf/desktop that referenced this pull request Aug 10, 2023
@benoitf
fix: add back 'Done' text on the button
1174c66 
fixes containers#3251
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
benoitf added a commit to benoitf/desktop that referenced this pull request Aug 10, 2023
@benoitf
fix: add back 'Done' text on the button
2d6515e 
fixes containers#3251
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
@benoitf benoitf closed this in f75dc3f Aug 10, 2023
@benoitf benoitf reopened this Aug 10, 2023
benoitf
benoitf approved these changes Aug 10, 2023
View reviewed changes
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

Did the workflow 4.5.1 -> 4.6.0 and everything worked

NFFDdBbx1d.mp4

@benoitf
Copy link
Collaborator

benoitf commented Aug 10, 2023

good job @jeffmaury 🚀

@jeffmaury
fix: addressing @cdrage review
71c028d 
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
@jeffmaury
Copy link
Contributor Author

@cdrage can you review as your request changes prevent to merge

@cdrage cdrage merged commit 595d6fa into containers:main Aug 14, 2023
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.3.0 milestone Aug 14, 2023
@themr0c themr0c mentioned this pull request Aug 17, 2023
Closed
@jeffmaury jeffmaury deleted the GH-2865 branch November 27, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benoitf benoitf benoitf approved these changes

@cdrage cdrage cdrage approved these changes

@lstocchi lstocchi Awaiting requested review from lstocchi lstocchi is a code owner automatically assigned from containers/podman-desktop-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.3.1
Development

Successfully merging this pull request may close these issues.

Support user-mode networking option in the UI
5 participants
@jeffmaury @benoitf @afbjorklund @cdrage @podman-desktop-bot