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

Add Command type #258

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Add Command type #258

merged 1 commit into from
Aug 16, 2023

Conversation

jakecorrenti
Copy link
Member

Adds the Command type which allows projects to generate a properly formatted gvproxy command in the form of a string slice without hard-coding it. Adds the functionality to convert Command to a format in which os/exec can directly execute it.

@jakecorrenti jakecorrenti force-pushed the config-to-cmdline branch 2 times, most recently from 6d88b0d to 69b0d7d Compare August 11, 2023 15:39
cfergeau
cfergeau reviewed Aug 16, 2023
View reviewed changes
Copy link
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

Minor Pidfile -> PidFile needed, otherwise looks good to me!

pkg/types/command.go Outdated Show resolved Hide resolved
pkg/types/command.go Outdated Show resolved Hide resolved
@jakecorrenti
Add Command type
4818381 
Adds the `Command` type which allows projects to generate a properly formatted
gvproxy command in the form of a string slice without hard-coding it. Adds the
functionality to convert `Command` to a format in which os/exec can directly
execute it.

Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>
cfergeau
cfergeau approved these changes Aug 16, 2023
View reviewed changes
Copy link
Collaborator

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Aug 16, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, jakecorrenti

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

@openshift-merge-robot openshift-merge-robot merged commit 26b7109 into containers:main Aug 16, 2023
@cfergeau
Copy link
Collaborator

make lint reports 2 issues:

golangci-lint run
pkg/types/command.go:192:9: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
	return exec.Command(gvproxyPath, c.ToCmdline()...)
	       ^
pkg/types/command.go:88:2: S1036: unnecessary guard around map access (gosimple)
	if _, ok := c.forwardInfo[flag]; ok {
	^
make: *** [Makefile:45: lint] Error 1

@jakecorrenti
Copy link
Member Author

Ok, I'll make a new PR for the changes

@cfergeau cfergeau mentioned this pull request Aug 17, 2023
Merged
jakecorrenti added a commit to jakecorrenti/gvisor-tap-vsock that referenced this pull request Aug 17, 2023
@jakecorrenti
Fix golangci-lint errors
e0d0584 
Fixes golangci-lint errors introduced by containers#258

Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>
@jakecorrenti jakecorrenti mentioned this pull request Aug 17, 2023
Merged
jakecorrenti added a commit to jakecorrenti/gvisor-tap-vsock that referenced this pull request Aug 17, 2023
@jakecorrenti
Fix golangci-lint errors
0d4226d 
Fixes golangci-lint errors introduced by containers#258

If `gosec` linting is enabled for the return statement in `Cmd`, an
error will be returned: `G204: Subprocess launched with a potential tainted
input or cmd arguments (gosec)`. This error tries to make sure a
user cannot provide a binary that could cause harm to the system. However,
since the binary path is sanitized and the arguments are generated by
gvproxy, this should be safe to ignore.

Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>
jakecorrenti added a commit to jakecorrenti/gvisor-tap-vsock that referenced this pull request Aug 21, 2023
@jakecorrenti
Fix golangci-lint errors
fc1e7fd 
Fixes golangci-lint errors introduced by containers#258

If `gosec` linting is enabled for the return statement in `Cmd`, an
error will be returned: `G204: Subprocess launched with a potential tainted
input or cmd arguments (gosec)`. This error tries to make sure a
user cannot provide a binary that could cause harm to the system.
We can't do much about this since the binary is externally
provided. It is up to the caller to make sure they don't provide a
"dangerous" binary. Therefore, it's ok that we ignore this linting
error.

Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>
jakecorrenti added a commit to jakecorrenti/gvisor-tap-vsock that referenced this pull request Aug 21, 2023
@jakecorrenti
Fix golangci-lint errors
408588c 
Fixes golangci-lint errors introduced by containers#258

If `gosec` linting is enabled for the return statement in `Cmd`, an
error will be returned: `G204: Subprocess launched with a potential tainted
input or cmd arguments (gosec)`. This error tries to make sure a
user cannot provide a binary that could cause harm to the system.
We can't do much about this since the binary is externally
provided. It is up to the caller to make sure they don't provide a
"dangerous" binary. Therefore, it's ok that we ignore this linting
error.

Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>
cfergeau pushed a commit that referenced this pull request Aug 21, 2023
@jakecorrenti @cfergeau
Fix golangci-lint errors
db10306 
Fixes golangci-lint errors introduced by #258

If `gosec` linting is enabled for the return statement in `Cmd`, an
error will be returned: `G204: Subprocess launched with a potential tainted
input or cmd arguments (gosec)`. This error tries to make sure a
user cannot provide a binary that could cause harm to the system.
We can't do much about this since the binary is externally
provided. It is up to the caller to make sure they don't provide a
"dangerous" binary. Therefore, it's ok that we ignore this linting
error.

Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>
@jakecorrenti jakecorrenti mentioned this pull request Aug 23, 2023
Merged
jakecorrenti added a commit to jakecorrenti/podman that referenced this pull request Sep 19, 2023
@jakecorrenti
Implement gvproxy networking using cmdline wrapper
289e59e 
Converts the host networking code in `podman machine` to use the
`GvproxyCommand` type introduced in containers/gvisor-tap-vsock#258

[NO NEW TESTS NEEDED]

Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfergeau cfergeau cfergeau approved these changes

Assignees

@cfergeau cfergeau

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakecorrenti @cfergeau @openshift-merge-robot