From 54ceb7ee70b75eb38c541fcbd42f7470059d10b9 Mon Sep 17 00:00:00 2001 From: absurdfarce Date: Tue, 25 Jun 2024 00:02:03 -0500 Subject: [PATCH 1/6] Drive-by fix while working on this issue. Noted there wasn't really a clear way to tell from logs whether a given query was handled by the proxy or passed on to upstream servers. Added some minimal logging to provide some additional info on this point. --- proxy/proxy.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/proxy/proxy.go b/proxy/proxy.go index 57443cb..f5bb79c 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -672,6 +672,7 @@ func (c *client) handleQuery(raw *frame.RawFrame, msg *partialQuery, customPaylo handled, stmt, err := parser.IsQueryHandled(parser.IdentifierFromString(c.keyspace), msg.query) if handled { + c.proxy.logger.Debug("Query is handled") if err != nil { c.proxy.logger.Error("error parsing query to see if it's handled", zap.Error(err)) c.send(raw.Header, &message.Invalid{ErrorMessage: err.Error()}) @@ -679,6 +680,7 @@ func (c *client) handleQuery(raw *frame.RawFrame, msg *partialQuery, customPaylo c.interceptSystemQuery(raw.Header, stmt) } } else { + c.proxy.logger.Debug("Query is not handled") c.execute(raw, c.getDefaultIdempotency(customPayload), c.keyspace, msg) } } From 5813a9f9515a05511eb7e53bec9e92bf09563dda Mon Sep 17 00:00:00 2001 From: absurdfarce Date: Tue, 25 Jun 2024 00:05:40 -0500 Subject: [PATCH 2/6] Added some explanatory comments --- proxy/proxy.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/proxy/proxy.go b/proxy/proxy.go index f5bb79c..c0376d8 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -818,6 +818,11 @@ func (c *client) interceptSystemQuery(hdr *frame.Header, stmt interface{}) { c.send(hdr, &message.ServerError{ErrorMessage: "Proxy unable to create new session for keyspace"}) } else { c.keyspace = s.Keyspace + // We might have received a quoted keyspace name in the UseStatement so remove any + // quotes before sending back this result message. This keeps us consistent with + // how Cassandra implements the same functionality and avoids any issues with + // drivers sending follow-on "USE" requests after wrapping the keyspace name in + // quotes. ks := parser.IdentifierFromString(s.Keyspace) c.send(hdr, &message.SetKeyspaceResult{Keyspace: ks.ID()}) } From dbdd98648334d91abd751d9050c01a39a9b0ed2a Mon Sep 17 00:00:00 2001 From: absurdfarce Date: Tue, 25 Jun 2024 11:41:34 -0500 Subject: [PATCH 3/6] Adding test per code review comments --- proxy/proxy_test.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index 46e1c44..af23c85 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -206,13 +206,17 @@ func TestProxy_UseKeyspace(t *testing.T) { cl := connectTestClient(t, ctx, proxyContactPoint) - resp, err := cl.SendAndReceive(ctx, frame.NewFrame(primitive.ProtocolVersion4, 0, &message.Query{Query: "USE system"})) - require.NoError(t, err) + testKeyspaces := []string{"system", "\"system\""} + for _, testKeyspace := range testKeyspaces { - assert.Equal(t, primitive.OpCodeResult, resp.Header.OpCode) - res, ok := resp.Body.Message.(*message.SetKeyspaceResult) - require.True(t, ok, "expected set keyspace result") - assert.Equal(t, "system", res.Keyspace) + resp, err := cl.SendAndReceive(ctx, frame.NewFrame(primitive.ProtocolVersion4, 0, &message.Query{Query: "USE " + testKeyspace})) + require.NoError(t, err) + + assert.Equal(t, primitive.OpCodeResult, resp.Header.OpCode) + res, ok := resp.Body.Message.(*message.SetKeyspaceResult) + require.True(t, ok, "expected set keyspace result") + assert.Equal(t, "system", res.Keyspace) + } } func TestProxy_NegotiateProtocolV5(t *testing.T) { From bd60d7490fa980768f8cebc9df9735e469ff6e8f Mon Sep 17 00:00:00 2001 From: absurdfarce Date: Thu, 27 Jun 2024 17:27:46 -0500 Subject: [PATCH 4/6] Clean up logging --- proxy/proxy.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/proxy/proxy.go b/proxy/proxy.go index c0376d8..2ffb370 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -667,12 +667,9 @@ func (c *client) handleExecute(raw *frame.RawFrame, msg *partialExecute, customP } func (c *client) handleQuery(raw *frame.RawFrame, msg *partialQuery, customPayload map[string][]byte) { - c.proxy.logger.Debug("handling query", zap.String("query", msg.query), zap.Int16("stream", raw.Header.StreamId)) - handled, stmt, err := parser.IsQueryHandled(parser.IdentifierFromString(c.keyspace), msg.query) - if handled { - c.proxy.logger.Debug("Query is handled") + c.proxy.logger.Debug("Query handled by proxy", zap.String("query", msg.query), zap.Int16("stream", raw.Header.StreamId)) if err != nil { c.proxy.logger.Error("error parsing query to see if it's handled", zap.Error(err)) c.send(raw.Header, &message.Invalid{ErrorMessage: err.Error()}) @@ -680,7 +677,7 @@ func (c *client) handleQuery(raw *frame.RawFrame, msg *partialQuery, customPaylo c.interceptSystemQuery(raw.Header, stmt) } } else { - c.proxy.logger.Debug("Query is not handled") + c.proxy.logger.Debug("Query not handled by proxy, forwarding", zap.String("query", msg.query), zap.Int16("stream", raw.Header.StreamId)) c.execute(raw, c.getDefaultIdempotency(customPayload), c.keyspace, msg) } } From cd418fe119eaab173a32b63fcd0d791ba958838e Mon Sep 17 00:00:00 2001 From: Lukasz Antoniak Date: Mon, 29 Jul 2024 18:45:38 +0200 Subject: [PATCH 5/6] Return detailed C* error for USE query. More test comments --- parser/lexer_test.go | 14 +++++++++----- proxy/proxy.go | 9 ++++++++- proxy/proxy_test.go | 31 +++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/parser/lexer_test.go b/parser/lexer_test.go index 85810bf..4b5e1ef 100644 --- a/parser/lexer_test.go +++ b/parser/lexer_test.go @@ -86,11 +86,15 @@ func TestLexerIdentifiers(t *testing.T) { {`"system"`, tkIdentifier, "system"}, {`"system"`, tkIdentifier, "system"}, {`"System"`, tkIdentifier, "System"}, - {`""""`, tkIdentifier, "\""}, - {`""""""`, tkIdentifier, "\"\""}, - {`"A"""""`, tkIdentifier, "A\"\""}, - {`"""A"""`, tkIdentifier, "\"A\""}, - {`"""""A"`, tkIdentifier, "\"\"A"}, + // below test verify correct escaping double quote character as per CQL definition: + // identifier ::= unquoted_identifier | quoted_identifier + // unquoted_identifier ::= re('[a-zA-Z][link:[a-zA-Z0-9]]*') + // quoted_identifier ::= '"' (any character where " can appear if doubled)+ '"' + {`""""`, tkIdentifier, "\""}, // outermost quotes indicate quoted string, inner two double quotes shall be treated as single quote + {`""""""`, tkIdentifier, "\"\""}, // same as above, but 4 inner quotes result in 2 quotes + {`"A"""""`, tkIdentifier, "A\"\""}, // outermost quotes indicate quoted string, 4 quotes after A result in 2 quotes + {`"""A"""`, tkIdentifier, "\"A\""}, // outermost quotes indicate quoted string, 2 quotes before and after A result in single quotes + {`"""""A"`, tkIdentifier, "\"\"A"}, // analogical to previous tests {`";`, tkInvalid, ""}, {`"""`, tkIdentifier, ""}, } diff --git a/proxy/proxy.go b/proxy/proxy.go index 2ffb370..bed4a45 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -812,7 +812,14 @@ func (c *client) interceptSystemQuery(hdr *frame.Header, stmt interface{}) { } case *parser.UseStatement: if _, err := c.proxy.maybeCreateSession(hdr.Version, s.Keyspace); err != nil { - c.send(hdr, &message.ServerError{ErrorMessage: "Proxy unable to create new session for keyspace"}) + var cqlError *proxycore.CqlError + switch { + case errors.As(err, &cqlError): + errMsg := cqlError.Message + c.send(hdr, errMsg) + default: + c.send(hdr, &message.ServerError{ErrorMessage: "Proxy unable to create new session for keyspace"}) + } } else { c.keyspace = s.Keyspace // We might have received a quoted keyspace name in the UseStatement so remove any diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index af23c85..995b597 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -219,6 +219,37 @@ func TestProxy_UseKeyspace(t *testing.T) { } } +func TestProxy_UseKeyspace_Error(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + tester, proxyContactPoint, err := setupProxyTest(ctx, 1, proxycore.MockRequestHandlers{ + primitive.OpCodeQuery: func(cl *proxycore.MockClient, frm *frame.Frame) message.Message { + qry := frm.Body.Message.(*message.Query) + if qry.Query == "USE non_existing" { + return &message.ServerError{ + ErrorMessage: "Keyspace 'non_existing' does not exist", + } + } + return cl.InterceptQuery(frm.Header, frm.Body.Message.(*message.Query)) + }}) + defer func() { + cancel() + tester.shutdown() + }() + require.NoError(t, err) + + cl := connectTestClient(t, ctx, proxyContactPoint) + + resp, err := cl.SendAndReceive(ctx, frame.NewFrame(primitive.ProtocolVersion4, 0, &message.Query{Query: "USE non_existing"})) + require.NoError(t, err) + + assert.Equal(t, primitive.OpCodeError, resp.Header.OpCode) + res, ok := resp.Body.Message.(*message.ServerError) + require.True(t, ok) + // make sure that CQL Proxy returns the same error of 'USE keyspace' command + // as backend C* cluster has and does not wrap it inside a custom one + assert.Equal(t, "Keyspace 'non_existing' does not exist", res.ErrorMessage) +} + func TestProxy_NegotiateProtocolV5(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) tester, proxyContactPoint, err := setupProxyTest(ctx, 1, nil) From 68126a86917ed42c814c625e991f6d2eb0a80a1a Mon Sep 17 00:00:00 2001 From: Lukasz Antoniak Date: Tue, 30 Jul 2024 18:06:36 +0200 Subject: [PATCH 6/6] Review comments --- proxy/proxy.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/proxy/proxy.go b/proxy/proxy.go index bed4a45..fc585f4 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -812,14 +812,13 @@ func (c *client) interceptSystemQuery(hdr *frame.Header, stmt interface{}) { } case *parser.UseStatement: if _, err := c.proxy.maybeCreateSession(hdr.Version, s.Keyspace); err != nil { + errMsg := "Proxy unable to create new session for keyspace" var cqlError *proxycore.CqlError - switch { - case errors.As(err, &cqlError): - errMsg := cqlError.Message - c.send(hdr, errMsg) - default: - c.send(hdr, &message.ServerError{ErrorMessage: "Proxy unable to create new session for keyspace"}) + if errors.As(err, &cqlError) { + // copy detailed error reason from downstream message + errMsg = cqlError.Message.GetErrorMessage() } + c.send(hdr, &message.ServerError{ErrorMessage: errMsg}) } else { c.keyspace = s.Keyspace // We might have received a quoted keyspace name in the UseStatement so remove any