resolver: cleanup of deprecated grpc resolver target.Endpoint field #14922
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #14922 +/- ##
==========================================
- Coverage 74.73% 74.49% -0.25%
==========================================
Files 415 415
Lines 34338 34342 +4
==========================================
- Hits 25664 25583 -81
- Misses 7037 7111 +74
- Partials 1637 1648 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
fuweid
reviewed
Dec 11, 2022
| r := &resolver{ | ||
| c: b.c, | ||
| target: target.Endpoint, | ||
| target: strings.TrimPrefix(target.URL.Path, "/"), |
Member
There was a problem hiding this comment.
Member
There was a problem hiding this comment.
The comment says Deprecated: use URL.Path or URL.Opaque instead.. Should we consider the URL.Opaque as well? Shouldn't the logic something like below?
prefix := target.URL.Path
if prefix == "" {
prefix = target.URL.Opaque
}
prefix = strings.TrimPrefix(prefix, "/")
Contributor
Author
There was a problem hiding this comment.
Hi @ahrtr,
Done.
I looked into your query: indeed if we declare URL with no '/' after the scheme, endpoint can be written to URL.Opaque.
fuweid
approved these changes
Dec 11, 2022
Member
fuweid
left a comment
fuweid
left a comment
There was a problem hiding this comment.
LGTM(non-binding)
TestEtcdGrpcResolver already test it.
ramil600
force-pushed
the
target-endpoint
branch
from
474e8b0 to
db2fd0f
Compare
ahrtr
reviewed
Dec 12, 2022
| } | ||
|
|
||
| func (b builder) Build(target gresolver.Target, cc gresolver.ClientConn, opts gresolver.BuildOptions) (gresolver.Resolver, error) { | ||
| endpoint := target.URL.Path |
Member
There was a problem hiding this comment.
Could you add a comment above line 35 something like below?
Refer to https://github.com/grpc/grpc-go/blob/16d3df80f029f57cff5458f1d6da6aedbc23545d/clientconn.go#L1587-L1611
Contributor
Author
There was a problem hiding this comment.
Done, thank you for the review.
target.Endpoint and some other fields are deprecated, URL field is suggested to use instead path is required to be stripped of "/" prefix for naming/resolver to work porperly Signed-off-by: Ramil Mirhasanov <[email protected]>
ramil600
force-pushed
the
target-endpoint
branch
from
db2fd0f to
932cb95
Compare
ahrtr
approved these changes
Dec 13, 2022
chaochn47
added a commit
to chaochn47/etcd
that referenced
this pull request
Oct 17, 2023
The last step with gRPC update behavior changes auditing to resolve CVE etcd-io#16740 in 3.5 This PR backports etcd-io#14922, etcd-io#16338, etcd-io#16587, etcd-io#16630, etcd-io#16636 and etcd-io#16739 to release-3.5. Signed-off-by: Chao Chen <[email protected]>
dusk125
pushed a commit
to dusk125/etcd
that referenced
this pull request
Oct 18, 2023
The last step with gRPC update behavior changes auditing to resolve CVE etcd-io#16740 in 3.5 This PR backports etcd-io#14922, etcd-io#16338, etcd-io#16587, etcd-io#16630, etcd-io#16636 and etcd-io#16739 to release-3.5. Signed-off-by: Chao Chen <[email protected]>
dusk125
pushed a commit
to dusk125/etcd
that referenced
this pull request
Oct 18, 2023
The last step with gRPC update behavior changes auditing to resolve CVE etcd-io#16740 in 3.5 This PR backports etcd-io#14922, etcd-io#16338, etcd-io#16587, etcd-io#16630, etcd-io#16636 and etcd-io#16739 to release-3.5. Signed-off-by: Chao Chen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
grpc.resolver.Target.Endpointand some other fields are deprecated, URL field is suggested to use instead.URL.Pathis required to be stripped of "/" prefix for naming/resolver to work properlySigned-off-by: Ramil Mirhasanov [email protected]
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.