Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DPE-6260][DPE-6261] - chore: use chain from certificate_available #297

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[DPE-6260][DPE-6261] - chore: use chain from certificate_available #297

wants to merge 6 commits into from

Conversation

marcoppenheimer
Copy link
Contributor

Fixes #290
Fixes #289

Changes Made

chore: load chain to truststore

  • When chain is provided using normal self-signed-certificates, the chain includes CA+cert. This is omitted in the data model
  • When chain is provided using manual-tls-certificates, the chain is likely to consist of rootCA+intermediate
  • Java doesn't seem to like loading crt chains directly to keystores, so we break them up in to separate aliases: chain0 --> chainN for N length chains

chore: avoid cert renewal for unit|app name SANs diffs

  • Digicert omits characters after / in CSR SANs that it signs. This caused the charm to think that there was a 'change' in SANs, and requested new ones
  • This is resolved by not looking for diffs in unit|app name SANs DNS, as these are not used for hostnames typically

@marcoppenheimer marcoppenheimer changed the title [DPE-6260][DPE-6261] chore: use chain from certificate_available [DPE-6260][DPE-6261] - chore: use chain from certificate_available Jan 16, 2025
deusebio
deusebio approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems sensible! Thanks!

imanenami
imanenami approved these changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@imanenami imanenami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks great to me, thanks! It seems the int. test timeout needs tweaking. In general, it appears to me that after upgrade to Kafka 3.9, our tests generally take longer to converge. I think one possible reason may be increased default timeout values in some config options, but I haven't investigated this theory.

deusebio
deusebio approved these changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@deusebio deusebio left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done on improving significantly the tests!!!

ps. just make sure the CI is happy before merging ofc. If making the CI happy requires more than 3 triggers, we should definitely have a look at this. Not critical for this PR (provided we manage to get the CI green) but worth having a ticket and a look at

@marcoppenheimer
Copy link
Contributor Author

@deusebio - CI isn't working because it requires ZK to have the same feature released in edge, which is in the other open PR. Once that merges, this will resolve :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imanenami imanenami imanenami approved these changes

@deusebio deusebio deusebio approved these changes

@Batalex Batalex Awaiting requested review from Batalex

@phvalguima phvalguima Awaiting requested review from phvalguima

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@marcoppenheimer @deusebio @imanenami