Conversation
|
Cictrone
left a comment
Cictrone
left a comment
There was a problem hiding this comment.
Leaving the Golang for @KCarretto
|
|
||
| pb = { workspace = true, features = ["imix"]} | ||
| transport = { workspace = true, features = ["grpc"] } | ||
| transport = { workspace = true, features = ["http"] } |
There was a problem hiding this comment.
Do we want main branch to be http1 by default?
| Ok(res) | ||
| } | ||
|
|
||
| fn encrypt_impl(pt_vec: Vec<u8>) -> Result<Vec<u8>> { |
There was a problem hiding this comment.
Why are we touching this?
There was a problem hiding this comment.
Each transport needs to implement their own crypto.
I refactored the crypto to be easier for each transport to work with swapping input / output types to Vec instead of the specific buffer each transport expects.
| Usage: "Run a redirector connecting agents using a specific transport to the server", | ||
| Subcommands: []cli.Command{ | ||
| { | ||
| Name: "http", |
There was a problem hiding this comment.
make this "http1"? Feels like the default should be http/2
There was a problem hiding this comment.
I've been using http and grpc to differentiate.
Would you prefer http1 and grpc?
There was a problem hiding this comment.
yeah I think it's good to be clear that it's forcing http1
| return | ||
| } | ||
|
|
||
| fmt.Printf("[gRPC -> HTTP] Response size: %d bytes\n", len(responseBody)) |
There was a problem hiding this comment.
We don't plan to leave the print statements here right? If we do, we should move this to slog like we do elsewhere
There was a problem hiding this comment.
Oh good catch yeah i'll add it to slog debug
| Action: func(cCtx *cli.Context) error { | ||
| // Convert main.Config options to redirector.Config options | ||
| redirectorOptions := []func(*redirector.Config){ | ||
| func(cfg *redirector.Config) { |
There was a problem hiding this comment.
can probably clean this up a bit but feel free to land and I'll make changes later on
|
|
||
| // streamConfig represents gRPC stream configuration | ||
| type streamConfig struct { | ||
| StreamName string |
There was a problem hiding this comment.
nit: Just embed a grpc.StremDesc here since you're using the same fields
type streamConfig struct {
*grpc.StreamDesc
// other fields that aren't in that
}
| func createStream(ctx context.Context, conn *grpc.ClientConn, cfg streamConfig) (grpc.ClientStream, error) { | ||
| return conn.NewStream( | ||
| ctx, | ||
| &grpc.StreamDesc{ |
There was a problem hiding this comment.
if you do the embedding I mentioned above, you can just pass the cfg here (or it might require cfg.StreamDesc)
| // RawCodec passes through raw bytes without marshaling/unmarshaling | ||
| type RawCodec struct{} | ||
|
|
||
| func (RawCodec) Marshal(v interface{}) ([]byte, error) { |
There was a problem hiding this comment.
nit: Use any instead of interface {}
| // HTTPRedirectorRun starts an HTTP/1.1 to gRPC proxy/redirector | ||
| func HTTPRedirectorRun(ctx context.Context, upstream string, options ...func(*Config)) error { | ||
| // Initialize Config | ||
| cfg := &Config{} |
There was a problem hiding this comment.
Do we want to set any defaults? This would be the place to do it
| return fmt.Errorf("failed to parse upstream address: %v", err) | ||
| } | ||
|
|
||
| port := url.Port() |
There was a problem hiding this comment.
port := url.Port()
if port == "" {
if(url.Scheme == "http") {
port = "80"
}
port = "443"
}
| url.Host, | ||
| grpc.WithTransportCredentials(tc), | ||
| grpc.WithContextDialer(func(ctx context.Context, _ string) (net.Conn, error) { | ||
| // Resolve using IPv4 only (A records, not AAAA records) |
There was a problem hiding this comment.
do we want to support IPv6?
There was a problem hiding this comment.
No, I don't think we should. By default it uses IPv6 if a AAAA record is present.
If IPv6 isn't supported on the network, it'll fail.
This caused issues on my dev machine since my network doesn't support V6 and I imagine this will be an issue for other people as well I think only supporting ipv4 is a reasonable assumption.
|
some small changes but nothing blocking, lgtm |
|
shell mux test is failing again gonna force merge. |
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #988