feat: pass auth header to connect client for RBAC integration

[agavra]

Description

This patch passes along the auth header that is used to the connect client so that we can forward it to the connect cluster when making requests.

Testing done

I setup RBAC following https://github.com/confluentinc/examples/tree/5.3.0-post/security/rbac and then tried to run KSQL locally to create a connector:

ksql> CREATE SOURCE CONNECTOR `datagen-pageviews` WITH(  "connector.class"='io.confluent.connect.jdbc.JdbcSourceConnector',  "connection.url"='jdbc:postgresql://localhost:5432/almog.gavra',  "mode"='timestamp',  "timestamp.column.name"='created_at',  "topic.prefix"='jdbc-',  "key"='username',  "table.whitelist"='users');

 Error
------------------------
 Unauthorized operation
------------------------

In another shell, run

confluent iam rolebinding create --principal User:ksqlcli --role ResourceOwner --resource Connector:datagen-pageviews --kafka-cluster-id pFESsWFwRHKy9E-3szNATQ --connect-cluster-id connect-cluster

We now can create the datagen-pageviews connector. I was lazy and didn't actually successfully create the connector because that means adding more roles to more topics and jazz like that, but we can see that the auth header passes along well.

ksql> CREATE SOURCE CONNECTOR `datagen-pageviews` WITH(  "connector.class"='io.confluent.connect.jdbc.JdbcSourceConnector',  "connection.url"='jdbc:postgresql://localhost:5432/almog.gavra',  "mode"='timestamp',  "timestamp.column.name"='created_at',  "topic.prefix"='jdbc-',  "key"='username',  "table.whitelist"='users');

 Error
------------------------------------------------------------------------------------------------
 {
  "error_code" : 400,
  "message" : "... error message is not important, we can now create it ..."
}
------------------------------------------------------------------------------------------------

Reviewer checklist

• Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
• Ensure relevant issues are linked (description should include text like "Fixes #")

[big-andy-coates]

Thanks @agavra

The changes LGTM, but the testing looks a little on the light side. There isn't actually a test that would fail if the headers weren't being set. I would say this PR needs unit tests adding to DefaultConnectClientTest to ensure headers are being set correctly AND ideally a test to ensures this works end-to-end, e.g. maybe add something to SecureIntegerationTest to prove it can connect to, and use, a secure Connect cluster.

I know the second test is a pain to write, but without it the integration could be completely broken and we'd never know without manual testing.

[agavra]

Thanks for the review @big-andy-coates! I think the first test (that the auth headers are actually being set) are a valuable test to add and I will do that.

I don't think that adding end-to-end automated tests for this at this level is worthwhile. The integration point is extremely limited and standard (just adding an auth header to the request) - if this integration point changes, it'll be a massive change for all confluent community products. Philosophically, I also don't think we should be testing Connect APIs - we cover our part of it just by adding the first test that you mention - we should be relying on Connect having tests for their public API. Otherwise the development burden is unsustainable (where do we draw the line of what external APIs we need to test and which we don't?)

Further, adding a test has more than just the implementation overhead; it will add flakiness, maintenance overhead, overall test time and unnecessary coupling with connect. Lastly, there are lots of different ways to configure both connect and KSQL security, the ways that I tested in the testing done description require proprietary plugins.

If we do want to add a test for this, the KSQL repo is not the right place for it and it should be done in some more system-test like environment.

[spena]

LGTM

[big-andy-coates]

Thanks for the review @big-andy-coates! I think the first test (that the auth headers are actually being set) are a valuable test to add and I will do that.

I don't think that adding end-to-end automated tests for this at this level is worthwhile. The integration point is extremely limited and standard (just adding an auth header to the request) - if this integration point changes, it'll be a massive change for all confluent community products. Philosophically, I also don't think we should be testing Connect APIs - we cover our part of it just by adding the first test that you mention - we should be relying on Connect having tests for their public API. Otherwise the development burden is unsustainable (where do we draw the line of what external APIs we need to test and which we don't?)

Further, adding a test has more than just the implementation overhead; it will add flakiness, maintenance overhead, overall test time and unnecessary coupling with connect. Lastly, there are lots of different ways to configure both connect and KSQL security, the ways that I tested in the testing done description require proprietary plugins.

If we do want to add a test for this, the KSQL repo is not the right place for it and it should be done in some more system-test like environment.

The end-to-end test request was not so much to do with headers, but just in general. At the moment we don't have any such test. Which means the integration could be broken and we would not know. Yes, we may have tests that check what we're passing to Connect APIs is what we're expecting, but we don't have any tests that prove what we're passing is correct i.e. our assumptions about what to pass might be wrong.

I've seen this kind of issue again and again. Someone writes an integration and manually tests it. Then someone else changes the code and doesn't test it fully and breaks it. Or the other side of the integration changes so that what was a valid request no longer is. Both result in a broken integration that isn't detected.

I'm not a fan of external integration tests. They are hard to debug and work with. Much better to have something in the KSQL build that our engineers can see/run/debug easily. I'm assuming we don't have any muckrake integration tests for our connect integration? Personally, I wouldn't want to be adding python tests when I can just add a java test in our own build.

[big-andy-coates]

Adding the additional tests has been discussed and agreed, but we can do that in a follow up PR, so approving...

[agavra]

Documenting that I will be adding the tests sometime in the next two weeks!