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

Use PEM certificates loaded from secrets for KafkaConnect #11198

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

Use PEM certificates loaded from secrets for KafkaConnect #11198

wants to merge 7 commits into from
+573 −301

Conversation

tinaselenge
Copy link
Contributor

@tinaselenge tinaselenge commented Feb 27, 2025 •
edited
Loading

Type of change

Select the type of your PR

  • Refactoring

Description

  • TLS certificates are loaded from secrets directly using KubernetesSecretConfigProvider, therefore also removes the population of the certificate related environment variables and the script for preparing TLS.
  • Use key pattern (*.crt) of KubernetesSecretConfigProvider to load multiple certificates from a single secret for ssl.truststore.certificates configuration. OAuth truststore is however configured differently, because multiline line certificates in Jaas config is not parsed correctly. Instead it will continue to use ssl.truststore.location which maps to the given secret's volume mount path for the certificate in PEM format.
  • Add RBAC right for reading secrets for KafkaConnect service account.

Resolves part of #11294

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md
  • Supply screenshots for visual changes, such as Grafana dashboards

Sorry, something went wrong.

@tinaselenge tinaselenge force-pushed the use-pem branch 2 times, most recently from b26740e to 33a167a Compare February 27, 2025 14:03
@tinaselenge
Copy link
Contributor Author

@scholzj @ppatierno @katheris can one of you please kick off the regression tests? I ran some of the relevant ST locally which seemed to pass, but there are many I haven't run so would like to try running the full suite. Thanks!

@scholzj
Copy link
Member

scholzj commented Feb 27, 2025

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tinaselenge
Copy link
Contributor Author

thanks @scholzj for kicking off the tests. Looks like the failing tests are not related to this PR (time out failures in CruiseControlST which doesn't deploy any connect resource).

I will mark this PR ready for review now :) .

@tinaselenge tinaselenge marked this pull request as ready for review February 28, 2025 10:28
@tinaselenge tinaselenge requested review from scholzj, ppatierno and katheris and removed request for scholzj and ppatierno February 28, 2025 10:28
@scholzj scholzj added this to the 0.46.0 milestone Feb 28, 2025
scholzj
scholzj reviewed Feb 28, 2025
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

I left some nits, but otherwise it mostly looks good.

@tinaselenge tinaselenge force-pushed the use-pem branch 3 times, most recently from b6cedbf to 5e44da7 Compare March 4, 2025 12:33
@tinaselenge tinaselenge force-pushed the use-pem branch 5 times, most recently from 888109a to 0352e9e Compare March 11, 2025 17:40
ppatierno
ppatierno reviewed Mar 22, 2025
View reviewed changes
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

@tinaselenge can you take a look at the failed tests within the build?

@tinaselenge tinaselenge mentioned this pull request Mar 24, 2025
Open
8 tasks
@tinaselenge
Copy link
Contributor Author

Thanks @ppatierno. The build had failed due to timeout on an unrelated test but after addressing your comment, the build is green.

Can someone please kick off the regression tests on this PR? Thanks!

@im-konge
Copy link
Member

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tinaselenge
Use PEM certificates loaded from secrets for KafkaConnect
3304f20 
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
@tinaselenge
Address review comments from Jakub
c601b03 
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
@tinaselenge
Address review comments
dcbf99b 
Remove volume mounts for TLS secrets

Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
@tinaselenge
Create an internal secret for TLS trusted certificates
e127bb5 
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
@tinaselenge
Improve internal cert secret name
a99972a 
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
@tinaselenge
Fix failing STs due to missing reconcilation of the internal secret f…
Loading
Loading status checks…
bb03a47 
…or MM2

Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
@im-konge
Copy link
Member

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tinaselenge
Remove hiding of password as its no longer needed
Loading
Loading status checks…
6569a2d 
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
@tinaselenge tinaselenge mentioned this pull request Mar 26, 2025
Open
@tinaselenge
Copy link
Contributor Author

I created an issue for this PR to discuss some of the points that would also apply when making the other operands use PEM files as well. I thought it would be easier to discuss in an issue than the PR.

@ppatierno
Copy link
Member

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

ppatierno
ppatierno previously approved these changes Mar 27, 2025
View reviewed changes
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Tina!

@ppatierno ppatierno self-requested a review March 27, 2025 07:54
@ppatierno ppatierno dismissed their stale review March 27, 2025 08:00

sorry If I approved but didn't notice your comment on #11294 so I guess we need more discussion.

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

@scholzj scholzj

@katheris katheris

@fvaleri fvaleri

@ppatierno ppatierno

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.46.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tinaselenge @scholzj @im-konge @ppatierno @katheris @fvaleri