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.

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

feat: only add non-FQDN variant of fully qualified domain names to SANs #564

Merged
merged 5 commits into from
Feb 24, 2025

Conversation

dervoeti
Copy link
Member

@dervoeti dervoeti commented Feb 14, 2025

Description

Replaces #547

Extends FQDN support to improve DNS lookup performance, see stackabletech/issues#656

We experienced issues, especially with mTLS, when a client connects to a host via its FQDN (pod.service.svc.cluster.local.). Currently, only the FQDN (with the trailing dot) is added to the SANs. With this fix, only the non-FQDN version (without trailing dot) is added. This solves the problems in our tests.
This fix should be backwards compatible and not break anything that works currently, it only improves support for FQDN cluster domains.

I tested the fix with several integration tests (including Zookeeper and Kafka mTLS tests) by setting the env var KUBERNETES_CLUSTER_DOMAIN to cluster.local. for all operators.

Without this fix, at least these tests failed for me:
tls_kafka-3.8.1_zookeeper-latest-3.9.2_use-client-tls-true_use-client-auth-tls-true_openshift-false
and
smoke_zookeeper-3.9.2_use-server-tls-true_use-client-auth-tls-true_openshift-false

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

Preview Give feedback

Reviewer

Preview Give feedback

Acceptance

Preview Give feedback

Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

I trust your experiments, so change looks good to me :)
But before we merge I would like to see some other reviewer on this.

  1. Can you please add a changelog entry? I think we should mark this as breaking, but note that it should be fine (tm)

Looking at #547, I also noticed the following things:

  1. Please also update docs/modules/secret-operator/pages/scope.adoc (mention the truncation)
  2. Normally I would not require you to write tests, on the other hand it would be kind of sad if we would throw away the effort of fix: For cluster internal scopes also add variant without trailing dot #547 writing some tests.
    Would you mind copying and adopting them into your PR?

@dervoeti
Copy link
Member Author

@sbernauer Thanks for the feedback! I tried to address all your points.

Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Thanks! Noticed on minor thing

@dervoeti dervoeti force-pushed the feat/handle-fqdn-hostnames branch from d5730f9 to b5d59f4 Compare February 14, 2025 13:59
@maltesander
Copy link
Member

maltesander commented Feb 14, 2025

Yeah i like this as the minimal change for experimental FQDNs. Fixed most of the problems i stumbled over.
Edit: I remember ZooKeeper was extra special, did you test with that as well (i guess if you tried Kafka)?

@dervoeti
Copy link
Member Author

Yeah i like this as the minimal change for experimental FQDNs. Fixed most of the problems i stumbled over. Edit: I remember ZooKeeper was extra special, did you test with that as well (i guess if you tried Kafka)?

Yes, I tested both ZooKeeper and Kafka

@dervoeti dervoeti force-pushed the feat/handle-fqdn-hostnames branch from 7900073 to d84ab26 Compare February 14, 2025 17:20
@dervoeti dervoeti force-pushed the feat/handle-fqdn-hostnames branch from d84ab26 to 114343b Compare February 14, 2025 17:23
sbernauer
sbernauer previously approved these changes Feb 17, 2025
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

LGTM

@dervoeti dervoeti force-pushed the feat/handle-fqdn-hostnames branch from f8a7a82 to 12dcfb3 Compare February 24, 2025 11:13
@dervoeti dervoeti added this pull request to the merge queue Feb 24, 2025
Merged via the queue into main with commit 95af4ac Feb 24, 2025
17 checks passed
@dervoeti dervoeti deleted the feat/handle-fqdn-hostnames branch February 24, 2025 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Development: Done
Development

Successfully merging this pull request may close these issues.

4 participants