Skip to content

Commit 48649ab

Browse files
fernando-villalbafernando-villalba
committed
Refactor resolver package and fix global state mutation safety
* **Refactor:** Split `resolver.go` into `cluster.go`, `cell.go`, and `shard.go` to separate domain logic and align with package size best practices. * **Refactor:** Split `resolver_test.go` into corresponding test files (`cluster_test.go`, `cell_test.go`, `shard_test.go`). * **Rename:** Rename `constants.go` to `defaults.go` to accurately reflect its purpose (providing default values) and implementation (variables vs constants). * **Fix:** Apply `.DeepCopy()` when assigning global default resources (`DefaultResourcesAdmin`, `DefaultResourcesEtcd`). This prevents critical global state pollution where modifying one cluster's spec could inadvertently mutate the process-wide defaults due to shared map pointers. * **Improvement:** Replace `reflect.DeepEqual` with explicit length checks (`isResourcesEmpty`) for more robust handling of empty resource requirements. * **Style:** Add package documentation and standardize import grouping.
1 parent 805043c commit 48649ab

File tree

9 files changed

+1489
-1353
lines changed

9 files changed

+1489
-1353
lines changed

‎pkg/resolver/cell.go‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
package resolver
2+
3+
import (
4+
"context"
5+
"fmt"
6+
7+
"k8s.io/apimachinery/pkg/api/errors"
8+
"k8s.io/apimachinery/pkg/types"
9+
10+
multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1"
11+
)
12+
13+
// ResolveCellTemplate fetches and resolves a CellTemplate by name, handling defaults.
14+
func (r *Resolver) ResolveCellTemplate(
15+
ctx context.Context,
16+
templateName string,
17+
) (*multigresv1alpha1.CellTemplate, error) {
18+
name := templateName
19+
isImplicitFallback := false
20+
21+
if name == "" {
22+
name = r.TemplateDefaults.CellTemplate
23+
}
24+
if name == "" {
25+
name = FallbackCellTemplate
26+
isImplicitFallback = true
27+
}
28+
29+
tpl := &multigresv1alpha1.CellTemplate{}
30+
err := r.Client.Get(ctx, types.NamespacedName{Name: name, Namespace: r.Namespace}, tpl)
31+
if err != nil {
32+
if errors.IsNotFound(err) {
33+
if isImplicitFallback {
34+
return &multigresv1alpha1.CellTemplate{}, nil
35+
}
36+
return nil, fmt.Errorf("referenced CellTemplate '%s' not found: %w", name, err)
37+
}
38+
return nil, fmt.Errorf("failed to get CellTemplate: %w", err)
39+
}
40+
return tpl, nil
41+
}
42+
43+
// MergeCellConfig merges a template spec with overrides and an inline spec to produce the final configuration.
44+
func MergeCellConfig(
45+
template *multigresv1alpha1.CellTemplate,
46+
overrides *multigresv1alpha1.CellOverrides,
47+
inline *multigresv1alpha1.CellInlineSpec,
48+
) (multigresv1alpha1.StatelessSpec, *multigresv1alpha1.LocalTopoServerSpec) {
49+
var gateway multigresv1alpha1.StatelessSpec
50+
var localTopo *multigresv1alpha1.LocalTopoServerSpec
51+
52+
if template != nil {
53+
if template.Spec.MultiGateway != nil {
54+
gateway = *template.Spec.MultiGateway.DeepCopy()
55+
}
56+
if template.Spec.LocalTopoServer != nil {
57+
localTopo = template.Spec.LocalTopoServer.DeepCopy()
58+
}
59+
}
60+
61+
if overrides != nil {
62+
if overrides.MultiGateway != nil {
63+
mergeStatelessSpec(&gateway, overrides.MultiGateway)
64+
}
65+
}
66+
67+
if inline != nil {
68+
return inline.MultiGateway, inline.LocalTopoServer
69+
}
70+
71+
return gateway, localTopo
72+
}

‎pkg/resolver/cell_test.go‎

Lines changed: 257 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,257 @@
1+
package resolver
2+
3+
import (
4+
"errors"
5+
"strings"
6+
"testing"
7+
8+
"github.com/google/go-cmp/cmp"
9+
"github.com/google/go-cmp/cmp/cmpopts"
10+
multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1"
11+
corev1 "k8s.io/api/core/v1"
12+
"k8s.io/apimachinery/pkg/api/resource"
13+
"k8s.io/apimachinery/pkg/runtime"
14+
"k8s.io/utils/ptr"
15+
"sigs.k8s.io/controller-runtime/pkg/client"
16+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
17+
)
18+
19+
func TestResolver_ResolveCellTemplate(t *testing.T) {
20+
t.Parallel()
21+
22+
scheme := runtime.NewScheme()
23+
_ = multigresv1alpha1.AddToScheme(scheme)
24+
25+
_, cellTpl, _, ns := setupFixtures(t)
26+
customCell := cellTpl.DeepCopy()
27+
customCell.Name = "custom-cell"
28+
29+
tests := map[string]struct {
30+
existingObjects []client.Object
31+
defaults multigresv1alpha1.TemplateDefaults
32+
reqName string
33+
wantErr bool
34+
errContains string
35+
wantFound bool
36+
wantResName string
37+
}{
38+
"Explicit Found": {
39+
existingObjects: []client.Object{customCell},
40+
reqName: "custom-cell",
41+
wantFound: true,
42+
wantResName: "custom-cell",
43+
},
44+
"Explicit Not Found (Error)": {
45+
existingObjects: []client.Object{},
46+
reqName: "missing-cell",
47+
wantErr: true,
48+
errContains: "referenced CellTemplate 'missing-cell' not found",
49+
},
50+
"Implicit Fallback Found": {
51+
existingObjects: []client.Object{cellTpl},
52+
defaults: multigresv1alpha1.TemplateDefaults{},
53+
reqName: "",
54+
wantFound: true,
55+
wantResName: "default",
56+
},
57+
"Implicit Fallback Not Found (Safe Empty Return)": {
58+
existingObjects: []client.Object{},
59+
defaults: multigresv1alpha1.TemplateDefaults{},
60+
reqName: "",
61+
wantFound: false,
62+
wantErr: false,
63+
},
64+
}
65+
66+
for name, tc := range tests {
67+
t.Run(name, func(t *testing.T) {
68+
t.Parallel()
69+
c := fake.NewClientBuilder().
70+
WithScheme(scheme).
71+
WithObjects(tc.existingObjects...).
72+
Build()
73+
r := NewResolver(c, ns, tc.defaults)
74+
75+
res, err := r.ResolveCellTemplate(t.Context(), tc.reqName)
76+
if tc.wantErr {
77+
if err == nil {
78+
t.Fatal("Expected error, got nil")
79+
}
80+
if tc.errContains != "" && !strings.Contains(err.Error(), tc.errContains) {
81+
t.Errorf(
82+
"Error message mismatch: got %q, want substring %q",
83+
err.Error(),
84+
tc.errContains,
85+
)
86+
}
87+
return
88+
} else if err != nil {
89+
t.Fatalf("Unexpected error: %v", err)
90+
}
91+
92+
if !tc.wantFound {
93+
if res == nil {
94+
t.Fatal(
95+
"Expected non-nil result structure even for not-found implicit fallback",
96+
)
97+
}
98+
if res.GetName() != "" {
99+
t.Errorf("Expected empty result, got object with name %q", res.GetName())
100+
}
101+
return
102+
}
103+
104+
if got, want := res.GetName(), tc.wantResName; got != want {
105+
t.Errorf("Result name mismatch: got %q, want %q", got, want)
106+
}
107+
})
108+
}
109+
}
110+
111+
func TestMergeCellConfig(t *testing.T) {
112+
t.Parallel()
113+
114+
tests := map[string]struct {
115+
tpl *multigresv1alpha1.CellTemplate
116+
overrides *multigresv1alpha1.CellOverrides
117+
inline *multigresv1alpha1.CellInlineSpec
118+
wantGw multigresv1alpha1.StatelessSpec
119+
wantTopo *multigresv1alpha1.LocalTopoServerSpec
120+
}{
121+
"Full Merge With Resources and Affinity Overrides": {
122+
tpl: &multigresv1alpha1.CellTemplate{
123+
Spec: multigresv1alpha1.CellTemplateSpec{
124+
MultiGateway: &multigresv1alpha1.StatelessSpec{
125+
Replicas: ptr.To(int32(1)),
126+
PodAnnotations: map[string]string{"foo": "bar"},
127+
Resources: corev1.ResourceRequirements{
128+
Requests: corev1.ResourceList{corev1.ResourceCPU: parseQty("100m")},
129+
},
130+
},
131+
LocalTopoServer: &multigresv1alpha1.LocalTopoServerSpec{
132+
Etcd: &multigresv1alpha1.EtcdSpec{Image: "base"},
133+
},
134+
},
135+
},
136+
overrides: &multigresv1alpha1.CellOverrides{
137+
MultiGateway: &multigresv1alpha1.StatelessSpec{
138+
Replicas: ptr.To(int32(2)),
139+
PodAnnotations: map[string]string{"baz": "qux"},
140+
Resources: corev1.ResourceRequirements{
141+
Requests: corev1.ResourceList{corev1.ResourceCPU: parseQty("200m")},
142+
},
143+
Affinity: &corev1.Affinity{
144+
NodeAffinity: &corev1.NodeAffinity{},
145+
},
146+
},
147+
},
148+
wantGw: multigresv1alpha1.StatelessSpec{
149+
Replicas: ptr.To(int32(2)),
150+
PodAnnotations: map[string]string{"foo": "bar", "baz": "qux"},
151+
Resources: corev1.ResourceRequirements{
152+
Requests: corev1.ResourceList{corev1.ResourceCPU: parseQty("200m")},
153+
},
154+
Affinity: &corev1.Affinity{
155+
NodeAffinity: &corev1.NodeAffinity{},
156+
},
157+
},
158+
wantTopo: &multigresv1alpha1.LocalTopoServerSpec{
159+
Etcd: &multigresv1alpha1.EtcdSpec{Image: "base"},
160+
},
161+
},
162+
"Template Only (Nil Overrides)": {
163+
tpl: &multigresv1alpha1.CellTemplate{
164+
Spec: multigresv1alpha1.CellTemplateSpec{
165+
MultiGateway: &multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(1))},
166+
},
167+
},
168+
overrides: nil,
169+
wantGw: multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(1))},
170+
},
171+
"Preserve Base (Empty Override)": {
172+
tpl: &multigresv1alpha1.CellTemplate{
173+
Spec: multigresv1alpha1.CellTemplateSpec{
174+
MultiGateway: &multigresv1alpha1.StatelessSpec{
175+
Replicas: ptr.To(int32(1)),
176+
PodAnnotations: map[string]string{"foo": "bar"},
177+
},
178+
},
179+
},
180+
overrides: &multigresv1alpha1.CellOverrides{
181+
MultiGateway: &multigresv1alpha1.StatelessSpec{},
182+
},
183+
wantGw: multigresv1alpha1.StatelessSpec{
184+
Replicas: ptr.To(int32(1)),
185+
PodAnnotations: map[string]string{"foo": "bar"},
186+
},
187+
},
188+
"Map Init (Nil Base)": {
189+
tpl: &multigresv1alpha1.CellTemplate{
190+
Spec: multigresv1alpha1.CellTemplateSpec{
191+
MultiGateway: &multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(1))},
192+
},
193+
},
194+
overrides: &multigresv1alpha1.CellOverrides{
195+
MultiGateway: &multigresv1alpha1.StatelessSpec{
196+
PodAnnotations: map[string]string{"a": "b"},
197+
PodLabels: map[string]string{"c": "d"},
198+
},
199+
},
200+
wantGw: multigresv1alpha1.StatelessSpec{
201+
Replicas: ptr.To(int32(1)),
202+
PodAnnotations: map[string]string{"a": "b"},
203+
PodLabels: map[string]string{"c": "d"},
204+
},
205+
},
206+
"Inline Priority": {
207+
tpl: &multigresv1alpha1.CellTemplate{
208+
Spec: multigresv1alpha1.CellTemplateSpec{
209+
MultiGateway: &multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(1))},
210+
},
211+
},
212+
inline: &multigresv1alpha1.CellInlineSpec{
213+
MultiGateway: multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(99))},
214+
},
215+
wantGw: multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(99))},
216+
},
217+
"Nil Template (Override Only)": {
218+
tpl: nil,
219+
overrides: &multigresv1alpha1.CellOverrides{
220+
MultiGateway: &multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(2))},
221+
},
222+
wantGw: multigresv1alpha1.StatelessSpec{Replicas: ptr.To(int32(2))},
223+
},
224+
"Nil Everything": {
225+
tpl: nil,
226+
wantGw: multigresv1alpha1.StatelessSpec{},
227+
wantTopo: nil,
228+
},
229+
}
230+
231+
for name, tc := range tests {
232+
t.Run(name, func(t *testing.T) {
233+
t.Parallel()
234+
gw, topo := MergeCellConfig(tc.tpl, tc.overrides, tc.inline)
235+
236+
if diff := cmp.Diff(tc.wantGw, gw, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" {
237+
t.Errorf("Gateway mismatch (-want +got):\n%s", diff)
238+
}
239+
if diff := cmp.Diff(tc.wantTopo, topo); diff != "" {
240+
t.Errorf("Topo mismatch (-want +got):\n%s", diff)
241+
}
242+
})
243+
}
244+
}
245+
246+
func TestResolver_ClientErrors_Cell(t *testing.T) {
247+
t.Parallel()
248+
errSimulated := errors.New("simulated database connection error")
249+
mc := &mockClient{failGet: true, err: errSimulated}
250+
r := NewResolver(mc, "default", multigresv1alpha1.TemplateDefaults{})
251+
252+
_, err := r.ResolveCellTemplate(t.Context(), "any")
253+
if err == nil ||
254+
err.Error() != "failed to get CellTemplate: simulated database connection error" {
255+
t.Errorf("Error mismatch: got %v, want simulated error", err)
256+
}
257+
}

0 commit comments

Comments
 (0)