K8SPSMDB-1503: include horizon domains in certificates#2157
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements support for including MongoDB split horizon domains in TLS certificates, addressing K8SPSMDB-1503. The change extracts duplicate horizon-handling logic into a reusable GetHorizons method and uses it to add horizon domains to certificate Subject Alternative Names (SANs) for clusters running version 1.22.0 or later.
Key Changes:
- Added
GetHorizonsmethod to consolidate horizon domain retrieval with optional port handling - Updated certificate SANs generation to include horizon domains when horizons are configured
- Refactored MongoDB replica set member configuration to use the new shared method
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/apis/psmdb/v1/psmdb_types.go | Added GetHorizons method to extract and process horizon domains with override support |
| pkg/controller/perconaservermongodb/mgo.go | Replaced inline horizon processing with call to new GetHorizons method |
| pkg/psmdb/tls/tls.go | Added horizon domains to certificate SANs for version 1.22.0+; removed version gate for wildcard DNS suffix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pull-request-size
bot
added
size/XL
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/apis/psmdb/v1/psmdb_types.go:754
- Missing documentation: The GetHorizons method lacks a doc comment explaining its purpose, parameters, and behavior. Add a comment describing that it returns horizon configurations for all pods, with optional port handling based on the withPorts parameter, and that overrides take precedence over base horizons.
PodSecurityContext *corev1.PodSecurityContext `json:"podSecurityContext,omitempty"`
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pooknull
requested review from
egegunes,
eleo007,
gkech,
hors,
jvpasinatto,
nmarukovich and
valmiranogueira
as code owners
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
egegunes
left a comment
egegunes
left a comment
There was a problem hiding this comment.
LGTM. Please check copilot comments
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| horizons[podName][h] = domain | ||
| } | ||
| } |
There was a problem hiding this comment.
The GetHorizons method only iterates over base Horizons (line 769) and applies overrides to them. This means if a pod has horizons defined exclusively in ReplsetOverrides but not in the base Horizons map, those override-only horizons will be ignored. Consider also iterating over ReplsetOverrides to ensure all horizon configurations are included, even if they don't have a corresponding entry in the base Horizons map.
| } | |
| } | |
| for podName, overrides := range r.ReplsetOverrides { | |
| if len(overrides.Horizons) == 0 { | |
| continue | |
| } | |
| // Skip pods that already have horizons from the base spec. | |
| if _, exists := horizons[podName]; exists { | |
| continue | |
| } | |
| horizons[podName] = make(map[string]string, len(overrides.Horizons)) | |
| for h, domain := range overrides.Horizons { | |
| if withPorts { | |
| if !strings.Contains(domain, ":") { | |
| domain = fmt.Sprintf("%s:%d", domain, r.GetPort()) | |
| } | |
| } | |
| horizons[podName][h] = domain | |
| } | |
| } |
| func TestReplsetSpec_GetHorizons(t *testing.T) { | ||
| r := &ReplsetSpec{ | ||
| Horizons: map[string]map[string]string{ | ||
| "pod-0": { | ||
| "ext": "a.example.com", | ||
| "int": "a.internal:27018", | ||
| }, | ||
| "pod-1": { | ||
| "ext": "b.example.com:27019", | ||
| }, | ||
| }, | ||
| ReplsetOverrides: map[string]ReplsetOverride{ | ||
| "pod-0": { | ||
| Horizons: map[string]string{ | ||
| "ext": "override.example.com", | ||
| }, | ||
| }, | ||
| }, | ||
| Configuration: `net: | ||
| port: 27017`, | ||
| } | ||
|
|
||
| t.Run("withPorts=true", func(t *testing.T) { | ||
| actual := r.GetHorizons(true) | ||
|
|
||
| expected := map[string]map[string]string{ | ||
| "pod-0": { | ||
| "ext": "override.example.com:27017", | ||
| "int": "a.internal:27018", | ||
| }, | ||
| "pod-1": { | ||
| "ext": "b.example.com:27019", | ||
| }, | ||
| } | ||
|
|
||
| assert.Equal(t, expected, actual, "GetHorizons(true) mismatch") | ||
| }) | ||
|
|
||
| t.Run("withPorts=false", func(t *testing.T) { | ||
| actual := r.GetHorizons(false) | ||
|
|
||
| expected := map[string]map[string]string{ | ||
| "pod-0": { | ||
| "ext": "override.example.com", | ||
| "int": "a.internal", | ||
| }, | ||
| "pod-1": { | ||
| "ext": "b.example.com", | ||
| }, | ||
| } | ||
|
|
||
| assert.Equal(t, expected, actual, "GetHorizons(false) mismatch") | ||
| }) | ||
| } |
There was a problem hiding this comment.
The test case for GetHorizons doesn't cover the scenario where horizons are defined only in ReplsetOverrides but not in the base Horizons map. Add a test case where a pod (e.g., "pod-2") has horizons defined only in ReplsetOverrides to ensure the method handles this edge case correctly.
commit: d63d7c4 |
hors
requested review from
bogdanjeler-ev and
oksana-grishchenko
as code owners
https://perconadev.atlassian.net/browse/K8SPSMDB-1503
DESCRIPTION
This PR includes split-horizon domains from
.spec.replsets[].splitHorizonsto the operator-generated TLS certificates. When.spec.replsets[].replsetOverridesare present, their domains override the corresponding split-horizon domains in the certificates.CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability