dt: Improve cert lookup in crl_test.py#25433
Open
michael-redpanda wants to merge 1 commit intoredpanda-data:devfrom
Open
dt: Improve cert lookup in crl_test.py#25433michael-redpanda wants to merge 1 commit intoredpanda-data:devfrom
michael-redpanda wants to merge 1 commit intoredpanda-data:devfrom
Conversation
The test originally assumed that the order of the certs matched the order of the Redpanda nodes. This change will instead search through the certificates to find the cert matching the node to revoke. Signed-off-by: Michael Boquard <michael.j.boquard@gmail.com>
Contributor
Author
|
@oleiman sorry for the noise, but since you originally wrote the test I'm just making sure I didn't invalidate it in some way, thanks! |
Contributor
Author
|
Oh @oleiman is off for like two weeks.... ha |
michael-redpanda
requested review from
pgellert
and removed request for
oleiman
BenPope
approved these changes
Mar 19, 2025
|
|
||
| def find_broker_cert(self, node: ClusterNode) -> Optional[tls.Certificate]: | ||
| return next( | ||
| (c for c in self.broker_certs if node.account.hostname in c.crt), |
Member
There was a problem hiding this comment.
It;s not obvious to me that node.name and node.account.hostname are the same in every environment this runs in, but that assumption does seem to run throughout the test.
It might be simpler to make self.broker_certs a map.
pgellert
approved these changes
Mar 19, 2025
Contributor
pgellert
left a comment
pgellert
left a comment
There was a problem hiding this comment.
I am not yet following how these certs could get reordered. But the fix seems like a net improvement, so lgtm.
Comment on lines
+262
to
+263
| broker_cert = self.provider.find_broker_cert(node) | ||
| assert broker_cert is not None, f"Failed to find certificate for node {node}" |
Contributor
There was a problem hiding this comment.
Q: do you know how the broker certs could end up in an order different from the order of nodes? It seems that the certs are created in the same order as the seld.redpanda.nodes inside write_tls_certs, so I am wondering how this reordering could happen.
Collaborator
CI test resultstest results on build#63349
test results on build#63380
test results on build#63432
|
Contributor
Author
|
/ci-repeat 5 |
Collaborator
Retry command for Build#63380please wait until all jobs are finished before running the slash command |
Contributor
Author
|
/ci-repeat 1 |
Contributor
Author
|
/ci-repeat 1 |
Collaborator
Retry command for Build#63432please wait until all jobs are finished before running the slash command |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The test originally assumed that the order of the certs matched the order of the Redpanda nodes. This change will instead search through the certificates to find the cert matching the node to revoke.
Fixes: CORE-8969
Todo:
Backports Required
Release Notes