You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[lb pub endpt] MetalLB public endpoint reuse: service cert SANs, BYO cert, front port - #2207
Support a MetalLB LoadBalancer public endpoint with a custom hostname, certificate and port (RHOSP 17.1 -> RHOSO adoption), in the openstack-operator. Two commits: "Public endpoint reuse: service cert, front port, live port name" and "Reconcile when a bring-your-own service cert secret changes".
Jira: OSPRH-39364 (cert SAN + bring-your-own cert), OSPRH-39366 (front port). Depends on the lib-common changes this bumps (service.GetVHostServerName, endpointURL validation).
SANs come from lib-common's canonical set {custom host, short, FQDN} (service.GetVHostServerName, the same helper the vhost uses), for the public and the internal endpoint. A bad endpointURL falls back to the in-cluster names and is logged. additionalSubjectNames stays cert-only.
Bring-your-own pod cert via the existing spec.<svc>.template.tls.api.{public,internal}.secretName (no new API field). Every EnsureEndpointConfig call site snapshots the user's Template.TLS before the per-reconcile restore and passes the snapshot as the BYO signal; reading it after the restore would mistake the operator's own issued secret name for a user override and delete the issued Certificate. Octavia now participates too (it passed an empty value before).
apiOverride.tls.secretName is the Route cert only and is now validated only when a Route exists.
Fix the cert cleanup delete target (the Service cert, not the Route cert).
Which field governs which cert:
Endpoint
Pod (Service) cert
Route cert
public, Route-backed
always operator-issued; tls.api.public.secretName is ignored (logged)
For a TLS LoadBalancer public endpoint with an endpointURL, inject a ports[] override with the endpointURL port (the scheme default, 443/80, when the URL has none). There is no implicit director legacy port: without an endpointURL the Service keeps its native port. Adopters who keep a RHOSP 17.1 front port put it in the endpointURL, e.g. https://keystone.example.com:13000 (the endpointURL host must be an FQDN). The port name is read from the live Service; targetPort is untouched. A user-set port number is kept; only its name is re-synced.
Injection is skipped for: non-TLS, Route-backed, non-LoadBalancer Services, multi-port Services, and while the live port is not yet named after its endpoint type (public), since an override carrying the old <service>-public name would be rejected by the service operator's by-name port merge before the Service is patched.
A watch on managed Services (port names and type) wakes the control plane after the service operator renames the port, so the injection follows without waiting for another event.
CreateRoute takes the target port name from the live Service (single port; otherwise the Service name, the annotation still wins).
A user endpointURL is kept when an endpoint moves from a Route to a LoadBalancer (it used to be cleared for one reconcile, flapping the catalog endpoint).
Glance: the loop returns the requeue of the API that asked for it, and a later API's valid secret no longer overwrites CustomTLSReady=False set earlier in the same reconcile.
BYO secret watch (second commit)
Editing the content of a BYO cert secret now reconciles the control plane, like apiOverride.tls.secretName does. The field index covers tls.api.{public,internal} of all services (found by walking the spec, so new services are covered) plus the simple TLS sections of horizon, novncproxy, prometheus and alertmanager. Operator-owned secrets (ovn, vencrypt, metadata) are not indexed. The indexed spec comes from the informer cache, i.e. the persisted user input.
Upgrade notes
tls.api.internal.secretName used to be ignored. If a control plane already carries it, the pod cert switches to that secret and the operator-issued Certificate is deleted (missing secret: requeue every 10s with CustomTLSReady=False).
The front port follows the endpointURL port for TLS LoadBalancer public endpoints, once the service operator names the port public. Without an endpointURL nothing changes: the Service keeps its native port.
Cert SANs are unchanged for endpoints without a custom endpointURL (same list and order, no re-issue). Endpoints with a user endpointURL gain the host as a SAN.
Rollout ordering
This is the openstack-operator side. With a service operator that still names its port <svc>-public, no front port is injected and the Service is unchanged; cert SANs, BYO certs and the Route target port work. The custom front port, endpointURL admission validation and the custom-hostname vhost need the per-service operator changes (port rename, new lib-common), which follow separately.
Notes for reviewers
The parent OpenStackControlPlane spec is never persisted by the reconcile (the only metadata writes are the finalizer add/remove); the snapshot and the secret index rely on that. A comment at the finalizer add documents the constraint.
DNS CNAME handling is intentionally not part of this change.
go.mod: only lib-common/modules/common is bumped; the other lib-common modules are left for the next bump.
Testing
make test passes (unit + envtest).
New unit tests cover: the SAN set (public/internal, bad URL, additionalSubjectNames), BYO public/internal, Route-backed BYO ignored, the cert delete target, BYO missing/valid conditions, port injection (TLS LB, skips, endpointURL follow, stale name, unrenamed port, multi-port, non-LoadBalancer), the Route target port, Route -> LoadBalancer keeping endpointURL, the Service watch predicate, and the secret index walker.
New envtests cover: operator-issued keystone service certs surviving repeated reconciles, a removed missing BYO secret clearing CustomTLSReady, and the Service watch enqueuing the contr
ol plane on port rename / type change but not for unmanaged Services.
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
New Features
Custom TLS certificates are recognized across more services, and endpoint certificates use configured service hostnames.
Service port or type changes are detected, and route targets can follow the configured service port name.
Endpoint port overrides are synchronized with eligible live service ports.
Bug Fixes
TLS settings and endpoint updates are handled more consistently during reconciliation, including preserving endpoint changes for processing.
Readiness reporting and operator-issued certificates are handled more reliably.
Disabling route creation no longer clears a service’s endpoint URL override.
Endpoint reconciliation now resolves service certificates from canonical endpoint hostnames and configured secrets, and applies Service port and Route target-port rules. Service reconcilers preserve user-provided TLS settings. The control-plane controller indexes TLS secret names and watches relevant managed Service changes.
Changes: Endpoint TLS and reconciliation
Layer / File(s)
Summary
Endpoint certificate and port behavior internal/openstack/common.go, internal/openstack/common_test.go
Endpoint certificates use canonical endpoint hostnames and resolve configured service secrets. The code handles missing secrets, readiness conditions, eligible LoadBalancer port overrides, Route target ports, and Route TLS behavior. Tests cover these cases.
Service reconciler TLS inputs internal/openstack/{barbican,cinder,designate,glance,heat,horizon,ironic,keystone,manila,neutron,nova,octavia,placement,swift,telemetry,watcher}.go, go.mod, api/go.mod
Service reconcilers pass saved user TLS settings to endpoint configuration. Glance retains non-empty reconciliation results. Both module files update the lib-common/modules/common revision.
Control-plane TLS indexing and Service watches internal/controller/core/openstackcontrolplane_controller.go, internal/controller/core/openstackcontrolplane_controller_test.go, test/functional/ctlplane/*
The controller indexes user-provided TLS service secrets and watches managed Service port-name or type changes. Tests cover secret lookup, reconcile enqueueing, TLS readiness, and service certificates.
sequenceDiagram
participant ServiceReconciler
participant EnsureEndpointConfig
participant CertificateResolver
participant KubernetesAPI
ServiceReconciler->>EnsureEndpointConfig: pass endpoint configuration and TLS settings
EnsureEndpointConfig->>CertificateResolver: resolve certificate using endpoint hostnames and configured secret
CertificateResolver->>KubernetesAPI: validate Secret or manage issued Certificate
Loading
Merge Risk: Minimal · up to 010bb
The checked certificate-readiness and Secret-rotation paths do not show a merge-blocking issue. The change is mergeable after normal checks.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title names MetalLB public endpoint reuse, service certificate SANs, bring-your-own certificate, and front port. The changeset implements these: SAN set from GetVHostServerName, BYO pod certs via …
Docstring Coverage
Docstring coverage is 93.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Description check
The description directly explains the MetalLB LoadBalancer endpoint, TLS certificate, BYO secret, port, Service watch, and reconciliation changes in the pull request.
Full details: Title check
Explanation
The title names MetalLB public endpoint reuse, service certificate SANs, bring-your-own certificate, and front port. The changeset implements these: SAN set from GetVHostServerName, BYO pod certs via tls.api.{public,internal}.secretName, and legacy front port injection for TLS LoadBalancer public endpoints. The title is long (85 characters) and uses a bracketed prefix, but it describes real, primary changes.
Full details: Docstring Coverage
Explanation
Docstring coverage is 93.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 30 functions across 22 files. (2 skipped: 2 unsupported.)
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
ast-grep (0.51.1)
ast-grep could not parse rule config: /ast-grep-rules/ast-grep-essentials/rules/rust/security/tokio-postgres-empty-password-rust.yml
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a15be832-4e10-4720-8d13-39a220df2a83
Commits
Reviewing files that changed from the base of the PR and between 6c94a2b and 010bb6e.
Files ignored due to path filters: 2
api/go.sum is excluded by !**/*.sum
go.sum is excluded by !**/*.sum
Files selected for processing: 20
api/go.mod
go.mod
internal/openstack/barbican.go
internal/openstack/cinder.go
internal/openstack/common.go
internal/openstack/common_test.go
internal/openstack/designate.go
internal/openstack/glance.go
internal/openstack/heat.go
internal/openstack/horizon.go
internal/openstack/ironic.go
internal/openstack/keystone.go
internal/openstack/manila.go
internal/openstack/neutron.go
internal/openstack/nova.go
internal/openstack/octavia.go
internal/openstack/placement.go
internal/openstack/swift.go
internal/openstack/telemetry.go
internal/openstack/watcher.go
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Additional comments: 1
internal/openstack/common.go-413-427: 1
413-427: Major · Functional Correctness · Heavy lift
Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.
The legacy front-port default changes the public port of existing TLS LoadBalancer deployments on upgrade.
This block has no opt-in. It runs for every TLS LoadBalancer public endpoint without a Route that has one live port named public. If endpointURL is unset, it injects endpoint.GetLegacyPublicTLSPort(legacyServiceName) into ports[].
Consider an existing RHOSO deployment that already exposes a public endpoint through MetalLB on its native port, for example keystone on 5000. After the service operator renames the port to public, the ServicePortOrTypeChangedPredicate watch fires. The next reconcile then moves the LoadBalancer front port to the director legacy value, for example 13000. The catalog follows the live Service. External clients, firewall rules, and DNS/VIP configurations that target the old port stop working. The user did not request this change.
The feature targets RHOSP-to-RHOSO adoption, so the legacy default should apply only when the user requests it. Options:
Inject only when endpointURL is set, and derive the port from that URL. This path is already explicit user input.
Gate the legacy-port lookup behind an explicit opt-in, such as an annotation or a spec flag.
♻️ Minimal fix: drop the implicit legacy default
if p, ok, err := service.GetEndpointURLPort(ed.Service.OverrideSpec.EndpointURL); err == nil && ok {
desiredPort, wantInject = p, true
- } else if ed.Service.OverrideSpec.EndpointURL == nil || *ed.Service.OverrideSpec.EndpointURL == "" {- if legacyPort, ok := endpoint.GetLegacyPublicTLSPort(legacyServiceName); ok {- desiredPort, wantInject = legacyPort, true- }- }+ } else if legacyPortOptIn(&svc) { // explicit adoption opt-in+ if legacyPort, ok := endpoint.GetLegacyPublicTLSPort(legacyServiceName); ok {+ desiredPort, wantInject = legacyPort, true+ }+ }
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 992fceb9-dcc2-4b15-8c14-438b7cbc7f1e
Commits
Reviewing files that changed from the base of the PR and between ea0a6b8 and 6c94a2b.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Warning:
File .ssh/known_hosts.old found in build home directory during playbook review.rdoproject.org/config/playbooks/base-minimal/post-logs.yaml for job podified-multinode-edpm-deployment-crc-antelope. Add to preserve-home-paths if safe, or otherwise remove.
File .ssh/known_hosts.old found in build home directory during playbook review.rdoproject.org/config/playbooks/base-minimal/post-logs.yaml for job podified-multinode-edpm-deployment-crc. Add to preserve-home-paths if safe, or otherwise remove.
File .ssh/known_hosts.old found in build home directory during playbook review.rdoproject.org/config/playbooks/base-minimal/post-logs.yaml for job openstack-operator-tempest-multinode. Add to preserve-home-paths if safe, or otherwise remove.
File .ssh/known_hosts.old found in build home directory during playbook review.rdoproject.org/config/playbooks/base-minimal/post-logs.yaml for job adoption-standalone-to-crc-ceph-provider. Add to preserve-home-paths if safe, or otherwise remove.
…port name
Support a MetalLB LoadBalancer public endpoint with a custom hostname,
certificate and port (RHOSP 17.1 -> RHOSO 18 adoption).
- Service cert: SANs come from the canonical service.GetVHostServerName set
({custom host, short, FQDN}); the custom endpointURL host is added for
public and internal endpoints, a bad endpointURL falls back to the
in-cluster names and is logged. A bring-your-own pod-level cert is taken
from tls.api.<endpoint>.secretName (snapshotted pre-restore at every
EnsureEndpointConfig call site), else one is issued. It applies to the
LoadBalancer public endpoint and the internal endpoint; a Route-backed
public endpoint keeps the operator-issued cert (a custom cert there goes
on the Route via apiOverride.tls.secretName, which is validated only when
a Route exists). Fix the cert cleanup delete target (Service cert, not
Route cert).
- Front port: for a TLS LoadBalancer public endpoint with an endpointURL,
inject a ports[] override with the endpointURL port (the scheme default
when the URL has none). There is no implicit director legacy port: without
an endpointURL the Service keeps its native port, and adopters who keep a
RHOSP 17.1 front port put it in the endpointURL (e.g.
https://keystone.example.com:13000). Injection is skipped for non-TLS or
routed endpoints, non-LoadBalancer Services, multi-port Services, and
while the live port is not yet named after its endpoint type ("public"),
since an override carrying the old <service>-public name is rejected by
the service operator's by-name port merge before the Service is patched.
A watch on managed Services' port names and type wakes the control plane
when the service operator renames the port. The override Spec is copied
before it is mutated.
- Routes: CreateRoute reads the target port name from the live Service
instead of assuming it equals the object name. A user endpointURL is kept
when a Route is replaced by a LoadBalancer.
- Glance: return the requeue of the API that asked for it, and do not let a
later endpoint's valid secret overwrite CustomTLSReady False set earlier
in the same reconcile.
Jira: OSPRH-39364
Jira: OSPRH-39366
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Martin Schuppert <mschuppert@redhat.com>
…anges
When a user updates the content of a secret referenced by a service
template's bring-your-own pod level cert - tls.api.{public,internal}
.secretName, or the simple TLS sections of horizon, novncproxy,
prometheus and alertmanager - the control plane was not reconciled, so a
missing or invalid secret was only noticed through the requeue.
Index those secret names of all services so the existing Secret watch
enqueues the control plane, like apiOverride.tls.secretName. The tls.api
names are collected by walking the spec for tls.APIService, so new
services are covered without a list to maintain; the simple TLS sections
are read explicitly. Operator owned secrets (ovn, vencrypt, metadata) are
not indexed. The indexed spec comes from the informer cache, i.e. the
persisted user input, never the operator-issued names written back in
memory.
Add unit tests for the secret name collection and for findObjectsForSrc.
Jira: OSPRH-39364
Jira: OSPRH-39366
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Martin Schuppert <mschuppert@redhat.com>
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
Support a MetalLB
LoadBalancerpublic endpoint with a custom hostname, certificate and port (RHOSP 17.1 -> RHOSO adoption), in the openstack-operator. Two commits: "Public endpoint reuse: service cert, front port, live port name" and "Reconcile when a bring-your-own service cert secret changes".Jira: OSPRH-39364 (cert SAN + bring-your-own cert), OSPRH-39366 (front port). Depends on the lib-common changes this bumps (
service.GetVHostServerName, endpointURL validation).What changes
Service cert (OSPRH-39364)
{custom host, short, FQDN}(service.GetVHostServerName, the same helper the vhost uses), for the public and the internal endpoint. A badendpointURLfalls back to the in-cluster names and is logged.additionalSubjectNamesstays cert-only.spec.<svc>.template.tls.api.{public,internal}.secretName(no new API field). EveryEnsureEndpointConfigcall site snapshots the user'sTemplate.TLSbefore the per-reconcile restore and passes the snapshot as the BYO signal; reading it after the restore would mistake the operator's own issued secret name for a user override and delete the issued Certificate. Octavia now participates too (it passed an empty value before).apiOverride.tls.secretNameis the Route cert only and is now validated only when a Route exists.Which field governs which cert:
tls.api.public.secretNameis ignored (logged)apiOverride.tls.secretName, else issuertls.api.public.secretName, else operator-issuedtls.api.internal.secretName, else operator-issuedFront port and live port name (OSPRH-39366)
LoadBalancerpublic endpoint with anendpointURL, inject aports[]override with theendpointURLport (the scheme default, 443/80, when the URL has none). There is no implicit director legacy port: without anendpointURLthe Service keeps its native port. Adopters who keep a RHOSP 17.1 front port put it in theendpointURL, e.g.https://keystone.example.com:13000(theendpointURLhost must be an FQDN). The port name is read from the live Service;targetPortis untouched. A user-set port number is kept; only its name is re-synced.public), since an override carrying the old<service>-publicname would be rejected by the service operator's by-name port merge before the Service is patched.CreateRoutetakes the target port name from the live Service (single port; otherwise the Service name, the annotation still wins).endpointURLis kept when an endpoint moves from a Route to a LoadBalancer (it used to be cleared for one reconcile, flapping the catalog endpoint).CustomTLSReady=Falseset earlier in the same reconcile.BYO secret watch (second commit)
Editing the content of a BYO cert secret now reconciles the control plane, like
apiOverride.tls.secretNamedoes. The field index coverstls.api.{public,internal}of all services (found by walking the spec, so new services are covered) plus the simple TLS sections of horizon, novncproxy, prometheus and alertmanager. Operator-owned secrets (ovn, vencrypt, metadata) are not indexed. The indexed spec comes from the informer cache, i.e. the persisted user input.Upgrade notes
tls.api.internal.secretNameused to be ignored. If a control plane already carries it, the pod cert switches to that secret and the operator-issued Certificate is deleted (missing secret: requeue every 10s withCustomTLSReady=False).endpointURLport for TLS LoadBalancer public endpoints, once the service operator names the portpublic. Without anendpointURLnothing changes: the Service keeps its native port.endpointURL(same list and order, no re-issue). Endpoints with a userendpointURLgain the host as a SAN.Rollout ordering
This is the openstack-operator side. With a service operator that still names its port
<svc>-public, no front port is injected and the Service is unchanged; cert SANs, BYO certs and the Route target port work. The custom front port,endpointURLadmission validation and the custom-hostname vhost need the per-service operator changes (port rename, new lib-common), which follow separately.Notes for reviewers
OpenStackControlPlanespec is never persisted by the reconcile (the only metadata writes are the finalizer add/remove); the snapshot and the secret index rely on that. A comment at the finalizer add documents the constraint.go.mod: onlylib-common/modules/commonis bumped; the other lib-common modules are left for the next bump.Testing
make testpasses (unit + envtest).additionalSubjectNames), BYO public/internal, Route-backed BYO ignored, the cert delete target, BYO missing/valid conditions, port injection (TLS LB, skips, endpointURL follow, stale name, unrenamed port, multi-port, non-LoadBalancer), the Route target port, Route -> LoadBalancer keepingendpointURL, the Service watch predicate, and the secret index walker.CustomTLSReady, and the Service watch enqueuing the control plane on port rename / type change but not for unmanaged Services.