[KIP-430] DescribeConsumerGroups, DescribeTopics, DescribeCluster with authorized AclOperations #2021
Conversation
|
|
emasab
changed the title
add SASL users to docker compose
| node?.ToString() ?? "null" | ||
| ).ToList()); | ||
| var authorizedOperations = string.Join(",", | ||
| AuthorizedOperations.Select(authorizedOperation => |
There was a problem hiding this comment.
I suppose AuthorizedOperations can be null, too, right?
needs handling for that
| ).ToList(); | ||
| } | ||
|
|
||
| private unsafe List<AclOperation> extractAuthorizedOperations(IntPtr authorizedOperationsPtr, int authorizedOperationCount) |
There was a problem hiding this comment.
authorizedOperationsPtr maybe a null pointer, and in that case we should return null, rather than an empty list, what do you think?
| /// <summary> | ||
| /// Extension methods for default <see cref="IAdminClient"/> implementations. | ||
| /// </summary> | ||
| public static class IAdminClientExtensions |
There was a problem hiding this comment.
Question: why are we using IAdminClientExtensions, and not using IAdminClient?
There was a problem hiding this comment.
I see we are doing this to avoid breaking binary compatibility but we don't guarantee that for .NET, right?
Also, as per https://learn.microsoft.com/en-us/dotnet/core/compatibility/categories#binary-compatibility
If we just add methods or method implementations, that doesn't break binary compatibility, too, right?
There was a problem hiding this comment.
Yeah, it's extending an interface that breaks it, in case one has it implemented. We're starting to avoid breaking it.
examples/AdminClient/Program.cs
Outdated
| foreach (var partition in topic.Partitions) | ||
| { | ||
| Console.WriteLine($" Partition ID: {partition.Partition} with leader: {partition.Leader}"); | ||
| if(!partition.ISR.Any()){ |
There was a problem hiding this comment.
nit: stick to the formatting norms of the brace on next line, here and elsewhere in this file
| Members.Select(member => | ||
| member.ToString() | ||
| ).ToList()); | ||
| var authorizedOperations = string.Join(",", |
There was a problem hiding this comment.
Add handling for if AuthorizedOperations is null
| Assert.Equal(2, resCount); | ||
|
|
||
| // TODO: remove after fix | ||
| ex.Results.TopicDescriptions.Sort( |
| Assert.Empty(ex.Results.TopicDescriptions[0].AuthorizedOperations); | ||
| Assert.Empty(ex.Results.TopicDescriptions[1].AuthorizedOperations); | ||
| Assert.True(ex.Results.TopicDescriptions[0].Error.IsError); | ||
| Assert.True(!ex.Results.TopicDescriptions[1].Error.IsError); |
There was a problem hiding this comment.
nit: use Assert.False instead.
|
|
||
| var descResAuthOps = | ||
| descResWithAuthOps.TopicDescriptions[0].AuthorizedOperations; | ||
| Assert.Equal( |
There was a problem hiding this comment.
I think we should OrderBy before asserting equality, because while the order of the operations will be exactly what you described, there is no reason for it to be so (and it's not something we or the user should assume)
Here and elsewhere
There was a problem hiding this comment.
The order will be that one because it comes from the order in the enum that corresponds to the bitfield order too
| topicList.IndexOf(b.Name); | ||
| } | ||
| ); | ||
| Assert.Empty(ex.Results.TopicDescriptions[0].AuthorizedOperations); |
There was a problem hiding this comment.
Note to change this to asserting null after making change to allow for null authorized operations
here and in other files
| }, | ||
| Entry = new AccessControlEntry | ||
| { | ||
| Principal = "User:user", |
There was a problem hiding this comment.
Use
"User:(value of string user)"
rather than hard-coding it.
For all the relevant tests.
examples/AdminClient/Program.cs
Outdated
| { | ||
| Console.WriteLine(" There is no In-Sync-Replica broker for the partition"); | ||
| } | ||
| else{ |
There was a problem hiding this comment.
| else{ | |
| else | |
| { |
examples/AdminClient/Program.cs
Outdated
| { | ||
| Console.WriteLine(" There is no Replica broker for the partition"); | ||
| } | ||
| else{ |
There was a problem hiding this comment.
these are nits but
| else{ | |
| else | |
| { |
examples/AdminClient/Program.cs
Outdated
|
|
||
| } | ||
| Console.WriteLine($" Is internal: {topic.IsInternal}"); | ||
| if(includeAuthorizedOperations) |
There was a problem hiding this comment.
| if(includeAuthorizedOperations) | |
| if (includeAuthorizedOperations) |
| }, | ||
| Entry = new AccessControlEntry | ||
| { | ||
| Principal = "User:user", |
There was a problem hiding this comment.
Maybe, this comment was missed for this test : #2021 (comment)
We should pick this dynamically
No description provided.