Preempt topic creation action with metadata list#1196
Preempt topic creation action with metadata list#1196rwaweber wants to merge 4 commits intoAiven-Open:mainfrom
Conversation
|
@rwaweber thanks for the pr. |
|
Hey @muralibasani! Yeah absolutely, will hopefully get around to it tonight or this weekend! |
muralibasani
force-pushed
the
preempt-topic-create
branch
from
71bc661 to
c80c2fb
Compare
|
@rwaweber mypy check failed. |
|
Hey @muralibasani - so trying to run the mypy check locally, it seems like it does a bit more than just linting with the Looking at this line in the GHA run, I can't seem to suss out what exactly is going wrong in the docker compose setup. Do you have any suggestions on where I should be digging around to see? |
@rwaweber I tried running your branch/fork locally and schema registry fails to start and is unhealthy. _schemas topic is not created ? |
muralibasani
force-pushed
the
preempt-topic-create
branch
2 times, most recently
from
e055eec to
3699e08
Compare
|
That was super helpful, thanks @muralibasani! Found out I missed a pretty clear warning in the confluent-kafka docs:
I think that the "describe" call I'm introducing was accidentally creating the Also got the mypy tests to pass locally, so hopefully this go around fixes things! |
|
@rwaweber will look at it next week. Sorry. |
|
No need to apologize! I appreciate the collaboration, no rush at all! |
muralibasani
force-pushed
the
preempt-topic-create
branch
from
15897a5 to
e1c86a5
Compare
| schema_topic_exists = False | ||
| # Perform a metadata list, which is allowed in a describe permission, whereas | ||
| # topic-create permissions are a coarser permission grant, not typically given out | ||
| schema_topic_exists = self.config.topic_name in self.admin_client.list_topics().topics.keys() |
There was a problem hiding this comment.
If list_topics() fails because of any permission issues (Describe for ex), then we might run into failures.
Hence a try/catch with except TopicAuthorizationFailedError: ?
There was a problem hiding this comment.
Sounds good! In that instance, I'm thinking we handle the exception, set schema_topic_exists = False and then, just continue on or we raise to shortcircuit?
I'm just thinking practically, where one would grant a permission to be able to create a topic but deny listing topics on the cluster seems unlikely, but I could be oversimplifying.
- Perform a get on topics that match the config topic name before we try to create the schema storage topic - In ACL-enabled kafka clusters, karapace would require a "CreateTopics" permission on the "Cluster" resource to complete this operation as-is. With these changes, the karapace identity only requires a "Describe" on a "Topic" resource with the "Metadata" API, which is easier to scope for an administrator perspective.
- Not a big deal since we're just doing a containment operation - Turns out I missed this warning on the list_topics operation: > Warning: If auto.create.topics.enable is set to true on the broker > and an unknown topic is specified, it will be created. [cite](https://docs.confluent.io/platform/current/clients/confluent-kafka-python/html/index.html#confluent_kafka.admin.AdminClient.list_topics)
rwaweber
force-pushed
the
preempt-topic-create
branch
from
e1c86a5 to
c6aac3b
Compare
| LOG.warning( | ||
| "[Schema Topic] not authorized to list topics, assuming topic: %r does not exist", | ||
| self.config.topic_name | ||
| ) |
There was a problem hiding this comment.
Let's add broader exception catch block too
except Exception:
schema_topic_exists = False
LOG.warning(
"[Schema Topic] failed to list topics, assuming topic: %r does not exist",
self.config.topic_name
)
| # Perform a metadata list, which is allowed in a describe permission, whereas | ||
| # topic-create permissions are a coarser permission grant, not typically given out | ||
| try: | ||
| schema_topic_exists = ( |
There was a problem hiding this comment.
Can we add a test for this extra check?
2 participants
In an ACL-enabled kafka environment trying to get karapace running as a schema registry, we ran into this hiccup since we aren't giving karapace full admin permissions.
Tracing down the topic authorization failure we were getting, it looks like karapace attempts topic creation before checking if the topic exists, then takes action based on that failure.
Instead, I think it makes sense to check the topic metadata before attempting to create the topic, as that's possible to do with a smaller, more scoped set of permissions.
About this change - What it does
References: N/A (I figured a small patch may set the scene better than a bug report, but happy to create one if you all prefer!)
Why this way
In ACL-enabled kafka clusters, karapace would require a "CreateTopics" permission on the "Cluster" resource to complete this operation as-is. With these changes, the karapace identity only requires a "Describe" on a "Topic" resource with the "Metadata" API, which is easier to scope for an administrator perspective.
That being said, happy to consider alternative approaches, as I'm still pretty unfamiliar with this codebase at large!