-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Initial pass on adding proxy peer server and configuration #10223
Conversation
aae8f1c
to
1bc2457
Compare
1bc2457
to
77ef008
Compare
@dboslee Ping us when this is ready to review. |
expectErr: true, | ||
}, | ||
{ | ||
desc: "certificates with correct role from different CAs", |
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 think this particular test case does not really represent what happens in the code.
The expected error is the result of the way the test is setup rather than actually be a legitimate error. I've removed it from my client branch as I was reworking the tests a bit after I implemented the equivalent of getConfigForClient on the client side.
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.
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.
Not sure I understand the issue. This is specifically testing the server side cert validation. Could we leave this and have a separate test for the client side?
One change I might add is setting the tc.client.InsecureSkipVerify = true
to ensure the failure happens server side.
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.
Unless I am seeing this wrong, there's nothing in the code itself that prevents a connection between a client and a server with certs from two different CAs. It is just triggered by the way the test case is implemented.
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.
The underlying call to ClientHandshake/ServerHandshake should return an error in this case.
@dboslee Is this PR ready for review? |
api/types/networking_test.go
Outdated
name string | ||
in string | ||
want ProxyPeering | ||
wantErr func(*testing.T, error) |
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.
wantErr func(*testing.T, error) | |
wantErr require.ErrorAssertionFunc |
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.
Will resolve this once we settle on a config format.
api/types/types.proto
Outdated
ProxyPeering ProxyPeering = 9 [ (gogoproto.jsontag) = "proxy_peering,omitempty" ]; | ||
} | ||
|
||
// ProxyPeeringMode represents the cluster proxy peering mode. | ||
enum ProxyPeering { | ||
// Disabled is set if proxy peering is disabled. | ||
Disabled = 0; | ||
// Enabled is set if proxy peering is enabled. | ||
Enabled = 1; |
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.
Instead of the faux boolean could we do something that is both backward compatible and open to changes in the future?
ProxyPeering ProxyPeering = 9 [ (gogoproto.jsontag) = "proxy_peering,omitempty" ]; | |
} | |
// ProxyPeeringMode represents the cluster proxy peering mode. | |
enum ProxyPeering { | |
// Disabled is set if proxy peering is disabled. | |
Disabled = 0; | |
// Enabled is set if proxy peering is enabled. | |
Enabled = 1; | |
TunnelDialingStrategy TunnelDialingStrategy = 9 [ (gogoproto.jsontag) = "tunnel_dialing_strategy,omitempty" ]; | |
} | |
// TunnelDialingStrategy | |
enum TunnelDialingStrategy { | |
// Legacy/WarDialer/Whatever we are calling it | |
Legacy = 0; | |
// ProxyPeering | |
ProxyPeering = 1; | |
..... | |
// SomeNewMechanism in the future could go here | |
SomeNewMechanism = 2; |
The naming in the suggestion could use some work - it was meant more to get the suggestion across than be anything concrete.
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.
Makes sense. I would be curious to hear your thoughts on updating the configuration based on this discussion as well. Basically another configuration we want to add is reversetunnel_count
which would be dependent on ProxyPeering
being enabled.
It feels a bit wrong but maybe not a big deal for reversetunnel_count
to be ignored when TunnelDialingStrategy
is not equal to ProxyPeering
.
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 haven't fully looked into that discussion yet - but as far as allowing each type to have its own config parameters wouldn't that be similar to say how teleport storage configuration can have different parameters based on the type?
storage:
type: etcd
peers: [https://etcd-0.etcd:2379, https://etcd-1.etcd:2379, https://etcd-2.etcd:2379]
tls_cert_file: /etc/etcd/certs/client-cert.pem
tls_key_file: /etc/etcd/certs/client-key.pem
tls_ca_file: /etc/etcd/certs/ca-cert.pem
prefix: teleport
storage:
type: dynamodb
table_name: tablename
region: us-east-2
lib/proxy/service.go
Outdated
|
||
sent, received := pipeConn(stream.Context(), streamConn, nodeConn) | ||
log.Debugf("Closing dial request from peer. sent: %d reveived %d", sent, received) | ||
return nil |
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.
Should this return any errors from streamConn/pipeConn instead of always returning nil
?
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.
Fixed comments on error handling here 39555e5
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.
Do you plan on adding any integration tests or extending the existing reverse tunnel tests to cover both agent mesh and proxy peering?
lib/proxy/server_test.go
Outdated
func() { | ||
defer conn.Close() | ||
|
||
client := proto.NewProxyServiceClient(conn) | ||
_, err = client.DialNode(context.Background()) | ||
tc.assertErr(t, err) | ||
}() |
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.
Does this really need to be wrapped in a function?
func() { | |
defer conn.Close() | |
client := proto.NewProxyServiceClient(conn) | |
_, err = client.DialNode(context.Background()) | |
tc.assertErr(t, err) | |
}() | |
defer conn.Close() | |
client := proto.NewProxyServiceClient(conn) | |
_, err = client.DialNode(context.Background()) | |
tc.assertErr(t, err) | |
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.
@rosstimothy these server tests change a little bit on the client branch
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.
It made sense when this wasn't wrapped in the t.Run(tc.desc, func(t *testing.T)
. But agreed not necessary now.
@rosstimothy I think that will make sense to add in the pr that has the changes for the agent/reverse tunneling. |
@rosstimothy / @fspmarshall / @espadolini could we get another review please? |
func (c *streamConn) Read(b []byte) (n int, err error) { | ||
c.rLock.Lock() |
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.
func (c *streamConn) Read(b []byte) (n int, err error) { | |
c.rLock.Lock() | |
func (c *streamConn) Read(b []byte) (n int, err error) { | |
if len(b) == 0 { | |
return 0, nil | |
} | |
c.rLock.Lock() |
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 would rather let this through that way it will fail if the stream is closed.
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.
A zero read would still be a noop if we had a non-empty buffer ready.
Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>
} | ||
|
||
// streamConn wraps a grpc stream with a net.streamConn interface. | ||
type streamConn struct { |
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.
Can you add some test coverage for this and pipeConn? They seem like pretty fundamental components yet are only indirectly tested through DialNode
. Some dedicated unit tests would ensure all the edge cases are taken care of
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've addressed these comments here a4053ec
lib/proxy/conn.go
Outdated
Recv() (*proto.Frame, error) | ||
} | ||
|
||
// streamConn wraps a grpc stream with a net.streamConn interface. |
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.
// streamConn wraps a grpc stream with a net.streamConn interface. | |
// streamConn wraps a grpc stream with a net.Conn interface. |
lib/proxy/conn.go
Outdated
} | ||
} | ||
|
||
// Read reads data reveived over the grpc stream. |
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.
// Read reads data reveived over the grpc stream. | |
// Read reads data received over the grpc stream. |
func (c *streamConn) Close() error { | ||
var err error | ||
if cstream, ok := c.stream.(grpc.ClientStream); ok { | ||
err = cstream.CloseSend() |
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.
// CloseSend closes the send direction of the stream. It closes the stream
// when non-nil error is met. It is also not safe to call CloseSend
// concurrently with SendMsg.
https://pkg.go.dev/google.golang.org/grpc#ClientStream
Should we prevent Close
and Write
from being called concurrently?
lib/proxy/service.go
Outdated
"github.com/sirupsen/logrus" | ||
) | ||
|
||
// proxyService impelements the grpc ProxyService. |
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.
// proxyService impelements the grpc ProxyService. | |
// proxyService implements the grpc ProxyService. |
lib/proxy/service.go
Outdated
log logrus.FieldLogger | ||
} | ||
|
||
// DialNode opens a bidrectional stream to the requested node. |
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.
// DialNode opens a bidrectional stream to the requested node. | |
// DialNode opens a bidirectional stream to the requested node. |
return nil | ||
} | ||
|
||
func (s *TunnelStrategyV1) CheckAndSetDefaults() error { |
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: missing godoc
lib/proxy/conn_test.go
Outdated
require.NoError(t, err) | ||
require.Equal(t, len(data), n) |
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.
Assertions in spawned goroutines will cause problems due to semantics of t.FailNow
. Here and in all other occurrences in this file the data needs to be sent over a channel and asserted on the main goroutine that is running the test
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.
So I believe using assert fixes this since it calls testing.Errorf
and testing.Log
under the hood which allows the test to continue and fails at the end. Updated here 7c047ff.
If its preferred to use require
I can move all the checks to the the main goroutine but using assert feels a little easier to read/reason about in this case.
This implements the proxy service for peer proxies to connect and dial nodes through.
Closes https://github.com/gravitational/cloud/issues/1305