-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
internal/resolver: introduce a new delegating resolver to handle both target URI and proxy address resolution #7857
Changes from 61 commits
bccef6b
6b89e0d
7c2fb21
e8bb6bc
36d0573
5a87284
e05a895
e724a9a
975e235
4b8ade9
4ba3578
fa55e5b
df11d7c
8bcf2b6
a5aeb9a
47e7936
a2e1415
df87a68
ed3d577
dd5bdb3
d24ec8b
74c6cd0
31d8c2c
1643768
4a5ab5b
fd061c7
c6dbcbc
2be9db3
67e3799
f5e1d37
019f621
8348b1f
f0d5b7b
4ac31f9
ed413f6
402316c
dea9ddd
40f1035
95d98b9
d5f4d8f
e743cbf
e008feb
4aea07d
ed345a1
7f416b0
e68d5de
8848105
61edca1
a01cc7a
845680d
c752bd8
d53100c
99292ea
74b3cf6
c745144
628a2a4
eeae936
bc0adeb
9c2ce28
4482e51
2bafe77
a0a2e50
ca2cc24
1f1e7f5
402badf
163ad20
db8b92c
6623eb5
c4bde98
1c4b416
5dc893c
69253c7
c5f8d6f
bbea8c4
fc2c41f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,55 @@ | ||||||
/* | ||||||
* | ||||||
* Copyright 2024 gRPC authors. | ||||||
* | ||||||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||||||
* you may not use this file except in compliance with the License. | ||||||
* You may obtain a copy of the License at | ||||||
* | ||||||
* http://www.apache.org/licenses/LICENSE-2.0 | ||||||
* | ||||||
* Unless required by applicable law or agreed to in writing, software | ||||||
* distributed under the License is distributed on an "AS IS" BASIS, | ||||||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
* See the License for the specific language governing permissions and | ||||||
* limitations under the License. | ||||||
* | ||||||
*/ | ||||||
|
||||||
// Package proxyattributes contains functions for getting and setting proxy | ||||||
// attributes like the CONNECT address and user info. | ||||||
package proxyattributes | ||||||
|
||||||
import ( | ||||||
"net/url" | ||||||
|
||||||
"google.golang.org/grpc/resolver" | ||||||
) | ||||||
|
||||||
type keyType string | ||||||
|
||||||
const proxyOptionsKey = keyType("grpc.resolver.delegatingresolver.proxyOptions") | ||||||
|
||||||
// Options holds the proxy connection details needed during the CONNECT | ||||||
// handshake. It includes the user information and the connect address. | ||||||
dfawley marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
type Options struct { | ||||||
User url.Userinfo | ||||||
ConnectAddr string | ||||||
} | ||||||
|
||||||
// Populate returns a copy of addr with attributes containing the provided user | ||||||
// and connect address, which are needed during the CONNECT handshake for a | ||||||
// proxy connection. | ||||||
func Populate(addr resolver.Address, opts Options) resolver.Address { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a style guide recommendation against the use of the We have a few options with alternative verbs for
As a last option, we could get rid of the options struct altogether and go with
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Went with Populate to SetOptions and rename Option to ExtractOptions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already use Get and Set here quite extensively, and I don't think would be considered a getter/setter, either. grpc-go/internal/hierarchy/hierarchy.go Line 64 in b3bdacb
grpc-go/internal/metadata/metadata.go Line 61 in b3bdacb
If the name of the package is the thing you want to set/get in the attributes, then it works out very naturally in usages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Changed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm clearly not reading things properly. Yes, In the meantime, I also had a discussion with some folks on the readability team, trying to understand why they don't recommend the use of
|
||||||
addr.Attributes = addr.Attributes.WithValue(proxyOptionsKey, opts) | ||||||
return addr | ||||||
} | ||||||
|
||||||
// Option returns the Options for the proxy [resolver.Address] and a boolean | ||||||
// value representing if the attribute is present or not. | ||||||
func Option(addr resolver.Address) (Options, bool) { | ||||||
if a := addr.Attributes.Value(proxyOptionsKey); a != nil { | ||||||
return a.(Options), true | ||||||
} | ||||||
return Options{}, false | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
/* | ||
* | ||
* Copyright 2024 gRPC authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
*/ | ||
|
||
package proxyattributes | ||
|
||
import ( | ||
"net/url" | ||
"testing" | ||
|
||
"google.golang.org/grpc/attributes" | ||
"google.golang.org/grpc/internal/grpctest" | ||
"google.golang.org/grpc/resolver" | ||
) | ||
|
||
type s struct { | ||
grpctest.Tester | ||
} | ||
|
||
func Test(t *testing.T) { | ||
grpctest.RunSubTests(t, s{}) | ||
} | ||
|
||
// Tests that Option returns a valid proxy attribute. | ||
func (s) TestOption(t *testing.T) { | ||
user := url.UserPassword("username", "password") | ||
tests := []struct { | ||
name string | ||
addr resolver.Address | ||
wantConnectAddr string | ||
wantUser url.Userinfo | ||
wantAttrPresent bool | ||
}{ | ||
{ | ||
name: "connect_address_in_attribute", | ||
addr: resolver.Address{ | ||
Addr: "test-address", | ||
Attributes: attributes.New(proxyOptionsKey, Options{ | ||
ConnectAddr: "proxy-address", | ||
}), | ||
}, | ||
wantConnectAddr: "proxy-address", | ||
wantAttrPresent: true, | ||
}, | ||
{ | ||
name: "user_in_attribute", | ||
addr: resolver.Address{ | ||
Addr: "test-address", | ||
Attributes: attributes.New(proxyOptionsKey, Options{ | ||
User: *user, | ||
}), | ||
}, | ||
wantUser: *user, | ||
wantAttrPresent: true, | ||
}, | ||
{ | ||
name: "no_attribute", | ||
addr: resolver.Address{Addr: "test-address"}, | ||
wantAttrPresent: false, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
gotOption, attrPresent := Option(tt.addr) | ||
if attrPresent != tt.wantAttrPresent { | ||
t.Errorf("Option(%v) = %v, want %v", tt.addr, attrPresent, tt.wantAttrPresent) | ||
} | ||
|
||
if gotOption.ConnectAddr != tt.wantConnectAddr { | ||
t.Errorf("ConnectAddr(%v) = %v, want %v", tt.addr, gotOption.ConnectAddr, tt.wantConnectAddr) | ||
} | ||
|
||
if gotOption.User != tt.wantUser { | ||
t.Errorf("User(%v) = %v, want %v", tt.addr, gotOption.User, tt.wantUser) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
// Tests that Populate returns a copy of addr with attributes containing correct | ||
// user and connect address. | ||
func (s) TestPopulate(t *testing.T) { | ||
addr := resolver.Address{Addr: "test-address"} | ||
pOpts := Options{ | ||
User: *url.UserPassword("username", "password"), | ||
ConnectAddr: "proxy-address", | ||
} | ||
|
||
// Call Populate and validate attributes | ||
populatedAddr := Populate(addr, pOpts) | ||
gotOption, attrPresent := Option(populatedAddr) | ||
if !attrPresent { | ||
t.Errorf("Option(%v) = %v, want %v ", populatedAddr, attrPresent, true) | ||
} | ||
if got, want := gotOption.ConnectAddr, pOpts.ConnectAddr; got != want { | ||
t.Errorf("Unexpected ConnectAddr proxy atrribute = %v, want %v", got, want) | ||
} | ||
if got, want := gotOption.User, pOpts.User; got != want { | ||
t.Errorf("unexpected User proxy attribute = %v, want %v", got, want) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: space after slashes for consistency..
But also: why bother with making it an
any
? It can just have the proper type. The reason for theany
s is to avoid circular dependencies, but the http and url packages in OSS will never import grpc/internal. :)Or even, let's just move this directly into internal/resolver/delegatingresolver as an exported thing there since it won't need to be changed by any non-grpc code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked to keep it
any
to limit the number of packages thatinternal
imports.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
External imports from
internal
should be fine. Internal imports frominternal
should be zero, or as close as possible.Either way, I don't think this needs to be here at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.