Skip to content

Add clusterip exposure for in-cluster-only access - #920

Open
WalBeh wants to merge 3 commits into
masterfrom
wb/clusterip-exposure
Open

WalBeh wants to merge 3 commits into
masterfrom
wb/clusterip-exposure

Conversation

@WalBeh

@WalBeh WalBeh commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary of changes

In-cluster clients don't need a cloud load balancer, but loadbalancer (the default) always requests one, and traefik needs externalDNS plus Traefik CRDs. clusterip serves the data Service as a plain ClusterIP. Exposure switches reuse the in-place Service patch, so the #851 NLB cleanup still applies. Resume only waits for an LB address with loadbalancer, otherwise a clusterip cluster would never resume. Suspend keeps a clusterip Service: it costs nothing, and recreating it would change the ClusterIP that in-cluster clients may have cached.

Checklist

@WalBeh

WalBeh commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Known gaps, not addressed here:

  • loadbalancer → clusterip: the external-IP webhook ignores the address going away, so webhook consumers keep the old LB address (same as loadbalancer → traefik today).
  • With grandCentral.exposure unset, a clusterip cluster still gets a public nginx Ingress for grand-central. Cloud-only; a follow-up makes that mapping explicit.
  • Resume with clusterip is covered by a unit test only: test-mode get_host needs an LB address, so the e2e tests can't connect to a ClusterIP cluster.

@itenev

itenev commented Oct 21, 2026

Copy link
Copy Markdown

Thanks for explicitly documenting the ClusterIP test-harness limitation above. Our review of commit38d0f3191403d3f6ceb280abc09daae4ced0ca7f confirms the distinction: TESTING=True falls through to the LoadBalancer-address lookup, while normal in-cluster operation uses discovery DNS. We therefore regard this as a known test-coverage limitation, not a production defect. Would you prefer to track support for ClusterIP connectivity in the external e2e harness as a separate follow-up? Our planned EKS verification will run the operator in-cluster and cover ClusterIP connectivity and suspend/resume; those live checks have not run yet.

@WalBeh

WalBeh commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor Author

Yes, a separate follow-up. Production connections don't depend on the exposure: get_host always returns the headless crate-discovery-<name> service, and #920 has a unit test for that. A harness test would mainly guard the test-mode branch. Our e2e suite runs the operator in the pytest process outside the test cluster, so it can only reach CrateDB through a public LB address; a ClusterIP test needs a port-forward or a test-only LB service. Tracked in #923. The full integration suite passed on the current head. Your in-cluster EKS run is the more useful check; please post the results here when you have them.

Without Traefik, every cluster requested a cloud load balancer, even
when only in-cluster clients connect. Resume now waits for a load
balancer address only with exposure=loadbalancer.

Closes #918

Assisted-by: Claude Code (LLM-assisted)
Suspend deletes the data service so a suspended cluster does not pay for
a load balancer. A clusterip service has no such cost, and recreating it
on resume gives it a new cluster IP.

Assisted-by: Claude Code (LLM-assisted)
The operator reaches the cluster through the headless discovery service.
A clusterip cluster has no external address, so routing get_host
through the data service would break its health check.

Assisted-by: Claude Code (LLM-assisted)
@WalBeh
WalBeh force-pushed the wb/clusterip-exposure branch from 38d0f31 to d073943 Compare October 22, 2026 08:24

@tomach tomach left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! 👍

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants