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.

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

fix(client): use grpc-go default dialer for tcp and unix connections #4127

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

hown3d
Copy link

@hown3d hown3d commented Aug 8, 2023

Closes #4126.

@hown3d hown3d changed the title fix(client): use grpc-go default dialer f fix(client): use grpc-go default dialer for tcp and unix connections Aug 8, 2023
client/client.go Outdated Show resolved Hide resolved
client/connhelper/npipe/npipe.go Outdated Show resolved Hide resolved
@jedevc
Copy link
Member

jedevc commented Aug 9, 2023

I think this seems good to me in principle. Though maybe @tonistiigi or @crazy-max have additional context for why we use a custom dialer here, instead of just using the default.

@hown3d
Copy link
Author

hown3d commented Jan 31, 2024

Is there something missing to review this PR?

@jedevc jedevc closed this Jan 31, 2024
@jedevc jedevc reopened this Jan 31, 2024
@jedevc
Copy link
Member

jedevc commented Jan 31, 2024

Closing/re-opening to kick CI to rerun against master (since this has been open a while).

Looks like linting is now failing (sorry).

Copy link
Member

@jedevc jedevc Jan 31, 2024

Choose a reason for hiding this comment

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

I think this package should now be included in cmd/buildctl/main.go since npipe is refactored to a connhelper. Otherwise, this connhelper can't actually get used.

I think this is what is causing the windows tests to fail.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. I needed to refactor to support windows and non windows builds, because importing the package in main assumes that it's always build.

When building non-windows and trying to use npipe connections, it will default to an error:
https://github.com/moby/buildkit/pull/4127/files#diff-0010b4ef47621e028d85218a669df091152ba11f4bafee2566797d759dc28214R12-R14

@jedevc
Copy link
Member

jedevc commented Feb 27, 2024

Rebased.

@jedevc
Copy link
Member

jedevc commented Feb 27, 2024

Note: this probably shouldn't be for v0.13.0.

@tonistiigi tonistiigi added this to the v0.14.0 milestone Feb 27, 2024
Copy link
Collaborator

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

The other part is that, try to focus first on the main changes of the PR other than the styling (unless you are changing the same line specifically as part of the functional change). That way, the review can be a little focused.

@@ -0,0 +1,14 @@
//go:build !windows

package npipe
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think the idea was that we try as much as possible to abstract the naming so that it's not platform specific (especially for Windows); that's why we'd gone with just a generic package client. This npipe package is very lean and it's Windows-specific; might be good to go this route only after we've a number of things we are doing with npipe other than just one helper.

Uses the default grpc dialer to allow [proxy configuration](https://github.com/grpc/grpc-go/blob/v1.57.0/Documentation/proxy.md)

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>
@jedevc jedevc merged commit 1d5a3ee into moby:master Mar 18, 2024
73 checks passed
@jedevc
Copy link
Member

jedevc commented Mar 18, 2024

Squashed commits + rebased, and merged 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buildctl not respecting proxy environment variables
5 participants