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

Under documented breaking change in unix domain handling in v1.42 #4894

Closed
howardjohn opened this issue Oct 21, 2021 · 4 comments
Closed

Under documented breaking change in unix domain handling in v1.42 #4894

howardjohn opened this issue Oct 21, 2021 · 4 comments

Comments

@howardjohn
Copy link
Contributor

What version of gRPC are you using?

Trying out v1.42.0-dev.0.20211020220737-f00baa6c3c84

What version of Go are you using (go version)?

1.17

What operating system (Linux, Windows, …) and version?

Linux

What did you do?

grpc.Dial("unix:/test.sock")
grpc.Dial("unix://test.sock")
grpc.Dial("unix:///test.sock")

What did you expect to see?

All of them work, as is the case in v1.41 and older, or a clear release note describing the breaking change. Preferably the later, as the code I ran into issues is not my code so I cannot easily change it.

What did you see instead?

Case grpc.Dial("unix://test.sock") fails with panic: failed to build resolver: invalid (non-empty) authority: test.sock. I assume this is from #4817. The release notes there do not imply any breaking changes.

howardjohn added a commit to howardjohn/k8s-kms-plugin that referenced this issue Oct 21, 2021
Context: grpc/grpc-go#4894

I have tested this works with grpc 1.41 and 1.42.
@easwars
Copy link
Contributor

easwars commented Oct 26, 2021

As per https://github.com/grpc/grpc/blob/master/doc/naming.md, the supported syntax for unix scheme is unix:path and unix://absolute_path. My line of thought is that we cannot release note possibly every behavior change, especially ones which were not supposed to work (but used to work), and now are not working. But I'm ready to convinced otherwise. @dfawley @menghanl

@howardjohn
Copy link
Contributor Author

My line of thought is that we cannot release note possibly every behavior change, especially ones which were not supposed to work (but used to work), and now are not working

That is the exact thing I would expect to be release noted :-). gRPC is at the core of almost every large application these days. If I upgrade (or even before) and it stops working, the first thing I would check is release notes and expect that every behavioral change is documented.

All I am suggesting is a release note which seems like pretty minimal effort to add? Ideally it would actually follow a deprecation window.

@dfawley
Copy link
Member

dfawley commented Oct 26, 2021

Argh. We spent a lot of time enumerating all the different user-visible behaviors to make sure we didn't break anything with this change, but it looks like we missed this one. 🤦

There was a test case that did unix://relative/path, but I think it was expected to fail before, but apparently because the path didn't exist: https://github.com/grpc/grpc-go/pull/4817/files#diff-b25c92b80601b564d1522420cd7c56d9d0aa7527299266f1256ac5fc5ac3d364R101

Sorry for the breakage! Since this was previously unintentional and undocumented behavior, I'm inclined to treat this as a bug fix and not a behavior change (so, no deprecation window). That said, now that we know there was a change, we should include a mention in the release notes.

@menghanl
Copy link
Contributor

We will cover the behavior change in the 1.42 release notes.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants