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

fix(machine): throw error connection refused after set proxy #18936

Merged
merged 1 commit into from
Jun 20, 2023
Merged

fix(machine): throw error connection refused after set proxy #18936

merged 1 commit into from
Jun 20, 2023

Conversation

BlackHole1
Copy link
Contributor

@BlackHole1 BlackHole1 commented Jun 20, 2023 •
edited
Loading

When the machine start command is executed, Podman automatically retrieves the current host's *_PROXY environment variable and assigns it directly to the virtual machine in QEMU. However, most *_PROXY variables are set with 127.0.0.1 or localhost, such as 127.0.0.1:8888. This causes failures in network-related operations within the virtual machine due to incorrect proxy settings.

Does this PR introduce a user-facing change?

Fix a bug proxy IP did not get properly translated when executing `machine start`

/cc @Luap99

@BlackHole1 BlackHole1 force-pushed the fix-proxy branch 2 times, most recently from 0ad5edf to ae67b40 Compare June 20, 2023 05:50
@BlackHole1 BlackHole1 changed the title Fix proxy fix(machine): throw connect: connection refused after set proxy Jun 20, 2023
@BlackHole1 BlackHole1 changed the title fix(machine): throw connect: connection refused after set proxy fix(machine): throw connect: connection refused after set proxy Jun 20, 2023
@BlackHole1 BlackHole1 changed the title fix(machine): throw connect: connection refused after set proxy fix(machine): throw error connection refused after set proxy Jun 20, 2023
@BlackHole1 BlackHole1 mentioned this pull request Jun 20, 2023
Merged
vrothberg
vrothberg reviewed Jun 20, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Thanks! Please write a useful commit message explaining the issue and how the commit fixes it.

@BlackHole1
Copy link
Contributor Author

BlackHole1 commented Jun 20, 2023 •
edited
Loading

Hey @vrothberg. Do you mean to modify Does this PR introduce a user-facing change? or pr title or commit body message

@vrothberg
Copy link
Member

I mean the commit body message. It's very helpful when reviewing a commit and it makes sure that we have a clean git history. This way, future maintainers have an easier time to understand why certain changes were made.

@BlackHole1
Copy link
Contributor Author

Hey @vrothberg. I have finished making the modifications. Thanks 😄.

Luap99
Luap99 approved these changes Jun 20, 2023
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.

LGTM

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2023
vrothberg
vrothberg approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 20, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BlackHole1, Luap99, vrothberg

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

edsantiago
edsantiago reviewed Jun 20, 2023
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

There's a lot of duplication, which makes me queasy. And the test for 'contains("")is anti-useful. See suggestion inline for a possible way to handle""(empty),=` (no need to duplicate duplication), and a default case where you don't need to spell out variables duplicatedly.

Thank you for your initiative and your contribution!

pkg/machine/qemu/machine_test.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023
@BlackHole1
fix(machine): throw connect: connection refused after set proxy
81e6322 
When the `machine start` command is executed, Podman automatically retrieves the current host's `*_PROXY` environment variable and assigns it directly to the virtual machine in QEMU. However, most `*_PROXY` variables are set with `127.0.0.1` or `localhost`, such as `127.0.0.1:8888`. This causes failures in network-related operations within the virtual machine due to incorrect proxy settings.

Fixes: containers#14087
Signed-off-by: Black-Hole1 <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Jun 20, 2023

Cleanup the todo in separate PR.
/lgtm
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 20, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023
@openshift-merge-robot openshift-merge-robot merged commit a77f896 into containers:main Jun 20, 2023
@BlackHole1 BlackHole1 deleted the fix-proxy branch June 21, 2023 01:54
@BlackHole1 BlackHole1 mentioned this pull request Jun 21, 2023
Merged
@edsantiago edsantiago mentioned this pull request Jun 28, 2023
Closed
@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 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jakecorrenti jakecorrenti jakecorrenti left review comments

@edsantiago edsantiago edsantiago left review comments

@Luap99 Luap99 Luap99 approved these changes

@vrothberg vrothberg vrothberg approved these changes

Assignees

@rhatdan rhatdan

@vrothberg vrothberg

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. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BlackHole1 @vrothberg @rhatdan @edsantiago @Luap99 @jakecorrenti @openshift-merge-robot