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). Since Reconcile runs each service sequentially and returns early on the first non-zero ctrl.Result, a missing secret for one service's internal cert also blocks every later service in that reconcile pass, not just the one carrying the field - pre-existing pipeline behavior, just newly reachable here. In practice this only affects adopters who already set the field as part of bringing a pre-existing (director/RHOSP 17.1) cert pair to a MetalLB public endpoint - it's not a plausible standalone config.
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.
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
Bug Fixes
Custom TLS certificates are now recognized consistently across services, and control planes react to changes in referenced secrets.
User-provided certificates are preserved during endpoint setup, with improved certificate validation and readiness reporting—including recovery when a configured secret is removed.
TLS-enabled load balancer endpoints now better handle service ports and route target selection.
Reconciliation now correctly retries when an API service needs another pass.
Improvements
Changes to managed service types or port names now trigger reconciliation.
Endpoint reconciliation now preserves user TLS inputs, resolves service certificates from endpoint settings, and updates port and Route handling. The control-plane controller indexes referenced service TLS secrets and watches managed Service port-name and type 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 configuration resolves BYO or operator-issued certificates and derives certificate hostnames from endpoint settings. Eligible TLS LoadBalancer endpoints receive port-override updates. Route TLS handling and target-port selection also change.
Service reconcilers save TLS settings before later reconciliation steps can replace them, then pass the saved values to endpoint configuration. Glance also returns a retained requeue result after processing its APIs.
Control-plane TLS indexing and Service watches internal/controller/core/openstackcontrolplane_controller.go, internal/controller/core/openstackcontrolplane_controller_test.go, internal/openstack/common.go, internal/openstack/common_test.go, test/functional/ctlplane/openstackoperator_controller_test.go, test/functional/ctlplane/secret_watch_test.go
The controller indexes user-provided service certificate secrets and watches managed Services for port-name or type changes. Tests cover secret lookup, reconcile triggers, TLS readiness, and service certificate persistence.
The examined endpoint and certificate changes have no identified merge-blocking issue. Merge after normal checks.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title names the MetalLB LoadBalancer public endpoint work and lists its three main parts: service certificate SANs, bring-your-own certificate, and front port. The changeset covers these areas: SA…
Description check
The description summarizes the MetalLB public endpoint changes. It covers service certificate SANs, BYO certificate handling, front port injection, the live port name, the BYO secret watch, upgrade no…
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.
Full details: Title check
Explanation
The title names the MetalLB LoadBalancer public endpoint work and lists its three main parts: service certificate SANs, bring-your-own certificate, and front port. The changeset covers these areas: SAN set via GetVHostServerName, BYO pod cert via tls.api.{public,internal}.secretName, and front port injection from endpointURL. The title is specific and matches the main change. The title is long (85 characters) and uses a bracketed prefix, but this does not make it unrelated or vague.
Full details: Description check
Explanation
The description summarizes the MetalLB public endpoint changes. It covers service certificate SANs, BYO certificate handling, front port injection, the live port name, the BYO secret watch, upgrade notes, and tests. These match the changeset in the raw summary.
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.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
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: 736ea41e-2aa1-421f-aae4-f43e12cfeca0
Commits
Reviewing files that changed from the base of the PR and between c0c83da and e3cc1ac.
Files selected for processing: 1
internal/openstack/common_test.go
Files skipped from review as they are similar to previous changes: 1
internal/openstack/common_test.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.
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. 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-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 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.
The reason will be displayed to describe this comment to others. Learn more.
Inline notes on the first items from my review. All of these are speculative, so please tell me if I'm misreading any of them.
The PR description item didn't fit inline: it still says this PR bumps lib-common/modules/common in go.mod. The head no longer touches go.mod, because main already has a5970f05c6a9, so the description may need an update.
The reason will be displayed to describe this comment to others. Learn more.
Small naming thing: I wonder if 'legacy' here could be misread. The PR body says there is no implicit director legacy port, and the injection is driven purely by the endpointURL. Would 'endpointURL front port' read more accurately?
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this comment is ahead of the code. I couldn't find endpointURL validation in api/ or internal/webhook in this repo, and the PR body says the admission validation follows separately. Would it make sense to say 'no admission validation yet, so this fallback is the only guard' instead? Or is there a webhook in another repo I'm missing?
The reason will be displayed to describe this comment to others. Learn more.
Same correction as the comment at line 389: this is covered, just on the service operator's own CR rather than in this repo. keystone-operator's KeystoneAPISpecCore.ValidateCreate/ValidateUpdate call webhook.ValidatePortNameOverrides, which runs webhook.ValidateEndpointURL against spec.override.service on the KeystoneAPI CR. So a malformed endpointURL is rejected at admission when this repo patches the computed override into the KeystoneAPI CR — reworded the comment to say that instead of implying no check exists anywhere.
The reason will be displayed to describe this comment to others. Learn more.
The safety argument here depends on admission rejecting a ports[].port that disagrees with an explicit endpointURL port. I couldn't find that check in this repo, so I'm not sure the two case branches below can never see a mismatch. Could we reword this, or would it help to add the check in this PR?
The reason will be displayed to describe this comment to others. Learn more.
You're right, and I was wrong earlier — I missed webhook.ValidatePortNameOverrides. keystone-operator's KeystoneAPISpecCore.ValidateCreate/ValidateUpdate already call validateServicePortNames → lib-common's webhook.ValidatePortNameOverrides, which runs both ValidateEndpointURL and ValidateEndpointURLPort against spec.override.service on the KeystoneAPI CR itself (nova-operator's NovaNoVNCProxy webhook calls the same two functions directly). So the admission check does exist — just on the service operator's own CR (KeystoneAPI, NovaNoVNCProxy, ...), triggered when this repo patches the computed override into that CR, not in openstack-operator's own webhook. Reworded the comment to say that precisely instead of claiming no check exists.
The reason will be displayed to describe this comment to others. Learn more.
This looks like the mismatch the comment in common.go says can't happen (endpointURL :13000 with port 9999), and the test expects 9999 to win. Is the intent to keep the user's number here, maybe with a log line, rather than rejecting it? If so, the comment in common.go may need updating to match.
The reason will be displayed to describe this comment to others. Learn more.
Before this PR, tls.api.internal.secretName was ignored here, so a missing secret didn't affect anything. Now a missing secret makes resolveServiceCert return RequeueAfter: 10s, and this function returns early. I wonder if that's the behavior we want for an existing CR that happens to carry the field. Would it make sense to call out the blocking part in the upgrade notes? And do other services keep reconciling while this one waits?
The reason will be displayed to describe this comment to others. Learn more.
Good catch — confirmed, and it's actually broader than just this one service. openstackcontrolplane_controller.go's Reconcile runs ReconcileKeystoneAPI → ReconcilePlacementAPI → ReconcileGlance → ReconcileCinder → ... strictly sequentially, each with an early return ctrlResult, nil on any non-zero ctrl.Result. So a missing tls.api.internal.secretName secret for, say, Keystone doesn't just block Keystone — it stops every later service in that reconcile pass too, until the secret shows up or the next 10s requeue happens to land after it's fixed. That's pre-existing pipeline architecture (any blocking ctrl.Result from any service does this), not something new in this PR — this PR just makes that specific field reachable where it was previously a no-op. Will call this out more precisely in the upgrade notes.
The reason will be displayed to describe this comment to others. Learn more.
One more nuance worth adding: realistically this only bites adopters who already set tls.api.internal.secretName as part of bringing a pre-existing (director/RHOSP 17.1) cert pair to a MetalLB public endpoint - that's the only scenario where an existing CR would plausibly carry this field at all, since nothing else in the product sets or documents it standalone. Updated the upgrade notes to say so.
The reason will be displayed to describe this comment to others. Learn more.
I think this is the intended fix, but I wanted to flag it as a live change. An endpointURL with no explicit port (for example https://host) now injects 443 (or 80), so an existing LoadBalancer Service would move its listener off the native port on upgrade, with no change in the CR. The upgrade notes say the front port follows the endpointURL port, but I wonder if the bare-URL case deserves its own line, since it's the one that would surprise someone. Does that match what you expect?
The reason will be displayed to describe this comment to others. Learn more.
its expected. a user must set the port if specify the endpointURL its also the url being registered in keystone catalog. If no endpoint is provided it uses 443 for https or 80 for http.
The reason will be displayed to describe this comment to others. Learn more.
Same naming point as the controller comment: this and the other tests in the file share the LegacyPortInjection prefix. Maybe FrontPort or EndpointURLPort would fit better? Happy to leave it for a follow-up if you'd rather not churn the diff.
Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.
…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 change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
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}
abays
left a comment
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). SinceReconcileruns each service sequentially and returns early on the first non-zeroctrl.Result, a missing secret for one service's internal cert also blocks every later service in that reconcile pass, not just the one carrying the field - pre-existing pipeline behavior, just newly reachable here. In practice this only affects adopters who already set the field as part of bringing a pre-existing (director/RHOSP 17.1) cert pair to a MetalLB public endpoint - it's not a plausible standalone config.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.