Skip to content

Commit 89434a2

Browse files
committed
fix: resolve all 117 golangci-lint issues in maas-controller
apply auto-fixes for 93 modernize (interface{} -> any) and gci (import ordering) issues via golangci-lint --fix. manually fix the remaining 24 issues: - extract repeated cel sub-expressions into constants and helper functions (subscriptionCacheKeySelector, authzCacheKeySelector) to resolve 5 lll violations while reducing duplication - add safe type assertion checks for errcheck violations in controller indexers and test helpers - use require.True for type assertions in externalmodel tests - check json.Marshal error returns for errchkjson compliance - rename shadowed builtin variable max -> limit (predeclared) - add ireturn exclusion for externalModelPredicate in .golangci.yml no functional changes — all existing tests pass and cache key isolation tests confirm cel expressions are byte-identical. Signed-off-by: Chaitanya Kulkarni <ckulkarn@redhat.com> Signed-off-by: Chaitanya Kulkarni <chkulkar@redhat.com> Made-with: Cursor
1 parent cab2990 commit 89434a2

File tree

11 files changed

+199
-133
lines changed

11 files changed

+199
-133
lines changed

maas-controller/.golangci.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,9 @@ linters:
120120
- linters:
121121
- ireturn
122122
path: pkg/controller/maas/maassubscription_controller\.go
123+
- linters:
124+
- ireturn
125+
path: pkg/reconciler/externalmodel/reconciler\.go
123126
- linters:
124127
- modernize
125128
text: "(omitzero|mapsloop|rangeint):"

maas-controller/cmd/manager/main.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -218,13 +218,13 @@ func main() {
218218
os.Exit(1)
219219
}
220220
if err := (&maas.MaaSAuthPolicyReconciler{
221-
Client: mgr.GetClient(),
222-
Scheme: mgr.GetScheme(),
223-
MaaSAPINamespace: maasAPINamespace,
224-
GatewayName: gatewayName,
225-
ClusterAudience: clusterAudience,
226-
MetadataCacheTTL: metadataCacheTTL,
227-
AuthzCacheTTL: authzCacheTTL,
221+
Client: mgr.GetClient(),
222+
Scheme: mgr.GetScheme(),
223+
MaaSAPINamespace: maasAPINamespace,
224+
GatewayName: gatewayName,
225+
ClusterAudience: clusterAudience,
226+
MetadataCacheTTL: metadataCacheTTL,
227+
AuthzCacheTTL: authzCacheTTL,
228228
}).SetupWithManager(mgr); err != nil {
229229
setupLog.Error(err, "unable to create controller", "controller", "MaaSAuthPolicy")
230230
os.Exit(1)

maas-controller/pkg/controller/maas/cross_namespace_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,10 @@ func TestMaaSSubscriptionReconciler_CrossNamespace(t *testing.T) {
360360
WithObjects(modelA, routeA, modelB, routeB, maasSub).
361361
WithStatusSubresource(&maasv1alpha1.MaaSSubscription{}).
362362
WithIndex(&maasv1alpha1.MaaSSubscription{}, "spec.modelRef", func(obj client.Object) []string {
363-
sub := obj.(*maasv1alpha1.MaaSSubscription)
363+
sub, ok := obj.(*maasv1alpha1.MaaSSubscription)
364+
if !ok {
365+
return nil
366+
}
364367
var refs []string
365368
for _, modelRef := range sub.Spec.ModelRefs {
366369
refs = append(refs, modelRef.Namespace+"/"+modelRef.Name)

maas-controller/pkg/controller/maas/maasauthpolicy_controller.go

Lines changed: 116 additions & 76 deletions
Large diffs are not rendered by default.

maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"context"
2121
"testing"
2222

23-
maasv1alpha1 "github.com/opendatahub-io/models-as-a-service/maas-controller/api/maas/v1alpha1"
2423
apierrors "k8s.io/apimachinery/pkg/api/errors"
2524
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2625
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -29,6 +28,8 @@ import (
2928
ctrl "sigs.k8s.io/controller-runtime"
3029
"sigs.k8s.io/controller-runtime/pkg/client/fake"
3130
gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1"
31+
32+
maasv1alpha1 "github.com/opendatahub-io/models-as-a-service/maas-controller/api/maas/v1alpha1"
3233
)
3334

3435
// newPreexistingAuthPolicy builds a Kuadrant AuthPolicy as an unstructured object
@@ -1140,11 +1141,11 @@ func TestMaaSAuthPolicyReconciler_NoIdentityHeadersUpstream(t *testing.T) {
11401141

11411142
// Verify essential fields for TRLP
11421143
requiredFields := []string{
1143-
"userid", // User identification
1144-
"groups", // User groups
1145-
"selected_subscription_key", // Model-scoped subscription key for TRLP
1146-
"selected_subscription", // Subscription name
1147-
"subscription_info", // Full subscription object (includes labels, organizationId, costCenter, etc.)
1144+
"userid", // User identification
1145+
"groups", // User groups
1146+
"selected_subscription_key", // Model-scoped subscription key for TRLP
1147+
"selected_subscription", // Subscription name
1148+
"subscription_info", // Full subscription object (includes labels, organizationId, costCenter, etc.)
11481149
}
11491150

11501151
for _, field := range requiredFields {
@@ -1175,7 +1176,7 @@ func TestMaaSAuthPolicyReconciler_NoIdentityHeadersUpstream(t *testing.T) {
11751176
t.Error("filters.identity must include 'subscription_info' field (full object from subscription-select)")
11761177
} else {
11771178
// Verify it's an expression that returns the full object
1178-
subscriptionInfoMap, ok := subscriptionInfoField.(map[string]interface{})
1179+
subscriptionInfoMap, ok := subscriptionInfoField.(map[string]any)
11791180
if !ok {
11801181
t.Errorf("subscription_info should be a map, got %T", subscriptionInfoField)
11811182
} else {
@@ -1231,4 +1232,3 @@ func contains(s, substr string) bool {
12311232
return false
12321233
}())
12331234
}
1234-

maas-controller/pkg/controller/maas/maassubscription_controller.go

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ import (
2020
"context"
2121
"errors"
2222
"fmt"
23-
"sort"
2423
"slices"
24+
"sort"
2525
"strings"
2626

2727
"github.com/go-logr/logr"
@@ -194,27 +194,27 @@ func (r *MaaSSubscriptionReconciler) reconcileTRLPForModel(ctx context.Context,
194194
return fmt.Errorf("failed to fetch HTTPRoute %s/%s: %w", httpRouteNS, httpRouteName, err)
195195
}
196196

197-
limitsMap := map[string]interface{}{}
197+
limitsMap := map[string]any{}
198198
var subNames []string
199199

200200
type subInfo struct {
201201
sub maasv1alpha1.MaaSSubscription
202202
mRef maasv1alpha1.ModelSubscriptionRef
203-
rates []interface{}
203+
rates []any
204204
}
205205
var subs []subInfo
206206
for _, sub := range allSubs {
207207
for _, mRef := range sub.Spec.ModelRefs {
208208
if mRef.Namespace != modelNamespace || mRef.Name != modelName {
209209
continue
210210
}
211-
var rates []interface{}
211+
var rates []any
212212
if len(mRef.TokenRateLimits) > 0 {
213213
for _, trl := range mRef.TokenRateLimits {
214-
rates = append(rates, map[string]interface{}{"limit": trl.Limit, "window": trl.Window})
214+
rates = append(rates, map[string]any{"limit": trl.Limit, "window": trl.Window})
215215
}
216216
} else {
217-
rates = append(rates, map[string]interface{}{"limit": int64(100), "window": "1m"})
217+
rates = append(rates, map[string]any{"limit": int64(100), "window": "1m"})
218218
}
219219
subs = append(subs, subInfo{sub: sub, mRef: mRef, rates: rates})
220220
break
@@ -241,15 +241,15 @@ func (r *MaaSSubscriptionReconciler) reconcileTRLPForModel(ctx context.Context,
241241

242242
// TRLP limit key must be safe for YAML (no slashes)
243243
safeKey := strings.ReplaceAll(subRef, "/", "-")
244-
limitsMap[fmt.Sprintf("%s-%s-tokens", safeKey, si.mRef.Name)] = map[string]interface{}{
244+
limitsMap[fmt.Sprintf("%s-%s-tokens", safeKey, si.mRef.Name)] = map[string]any{
245245
"rates": si.rates,
246-
"when": []interface{}{
247-
map[string]interface{}{
246+
"when": []any{
247+
map[string]any{
248248
"predicate": fmt.Sprintf(`auth.identity.selected_subscription_key == "%s"`, modelScopedRef),
249249
},
250250
},
251-
"counters": []interface{}{
252-
map[string]interface{}{"expression": "auth.identity.userid"},
251+
"counters": []any{
252+
map[string]any{"expression": "auth.identity.userid"},
253253
},
254254
}
255255
}
@@ -279,8 +279,8 @@ func (r *MaaSSubscriptionReconciler) reconcileTRLPForModel(ctx context.Context,
279279
return fmt.Errorf("failed to set owner reference on TokenRateLimitPolicy %s/%s: %w", policy.GetNamespace(), policy.GetName(), err)
280280
}
281281

282-
spec := map[string]interface{}{
283-
"targetRef": map[string]interface{}{
282+
spec := map[string]any{
283+
"targetRef": map[string]any{
284284
"group": "gateway.networking.k8s.io",
285285
"kind": "HTTPRoute",
286286
"name": httpRouteName,
@@ -609,7 +609,10 @@ func (r *MaaSSubscriptionReconciler) SetupWithManager(mgr ctrl.Manager) error {
609609
&maasv1alpha1.MaaSSubscription{},
610610
modelRefIndexKey,
611611
func(obj client.Object) []string {
612-
sub := obj.(*maasv1alpha1.MaaSSubscription)
612+
sub, ok := obj.(*maasv1alpha1.MaaSSubscription)
613+
if !ok {
614+
return nil
615+
}
613616
var refs []string
614617
for _, modelRef := range sub.Spec.ModelRefs {
615618
// Index value format: "namespace/name"

maas-controller/pkg/controller/maas/maassubscription_controller_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,10 @@ import (
3939
// subscriptionModelRefIndexer is the field indexer function for MaaSSubscription.
4040
// Extracted as a helper to be reused across tests with fake clients.
4141
func subscriptionModelRefIndexer(obj client.Object) []string {
42-
sub := obj.(*maasv1alpha1.MaaSSubscription)
42+
sub, ok := obj.(*maasv1alpha1.MaaSSubscription)
43+
if !ok {
44+
return nil
45+
}
4346
var refs []string
4447
for _, modelRef := range sub.Spec.ModelRefs {
4548
refs = append(refs, modelRef.Namespace+"/"+modelRef.Name)

maas-controller/pkg/reconciler/externalmodel/reconciler.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,7 @@ func specFromExternalModel(extModel *maasv1alpha1.ExternalModel, model *maasv1al
295295
if portStr, ok := ann[AnnPort]; ok {
296296
p, err := strconv.ParseInt(portStr, 10, 32)
297297
if err != nil {
298-
return spec, fmt.Errorf("invalid port %q: %v", portStr, err)
298+
return spec, fmt.Errorf("invalid port %q: %w", portStr, err)
299299
}
300300
if p < 1 || p > 65535 {
301301
return spec, fmt.Errorf("port %d out of range (1-65535)", p)
@@ -306,14 +306,14 @@ func specFromExternalModel(extModel *maasv1alpha1.ExternalModel, model *maasv1al
306306
if tlsStr, ok := ann[AnnTLS]; ok {
307307
parsed, err := strconv.ParseBool(tlsStr)
308308
if err != nil {
309-
return spec, fmt.Errorf("invalid tls value %q: %v", tlsStr, err)
309+
return spec, fmt.Errorf("invalid tls value %q: %w", tlsStr, err)
310310
}
311311
spec.TLS = parsed
312312
}
313313

314314
if extraStr, ok := ann[AnnExtraHeaders]; ok && extraStr != "" {
315315
spec.ExtraHeaders = map[string]string{}
316-
for _, pair := range strings.Split(extraStr, ",") {
316+
for pair := range strings.SplitSeq(extraStr, ",") {
317317
kv := strings.SplitN(pair, "=", 2)
318318
if len(kv) == 2 {
319319
spec.ExtraHeaders[strings.TrimSpace(kv[0])] = strings.TrimSpace(kv[1])

maas-controller/pkg/reconciler/externalmodel/resources.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -54,12 +54,12 @@ func BuildServiceEntry(spec ExternalModelSpec, modelName, namespace string, labe
5454
se.SetNamespace(namespace)
5555
se.SetLabels(labels)
5656

57-
se.Object["spec"] = map[string]interface{}{
58-
"hosts": []interface{}{spec.Endpoint},
57+
se.Object["spec"] = map[string]any{
58+
"hosts": []any{spec.Endpoint},
5959
"location": "MESH_EXTERNAL",
6060
"resolution": "DNS",
61-
"ports": []interface{}{
62-
map[string]interface{}{
61+
"ports": []any{
62+
map[string]any{
6363
"number": int64(spec.Port),
6464
"name": portName,
6565
"protocol": protocol,
@@ -81,16 +81,16 @@ func BuildDestinationRule(spec ExternalModelSpec, modelName, namespace string, l
8181
dr.SetNamespace(namespace)
8282
dr.SetLabels(labels)
8383

84-
tlsConfig := map[string]interface{}{
84+
tlsConfig := map[string]any{
8585
"mode": "SIMPLE",
8686
}
8787
if spec.TLSInsecureSkipVerify {
8888
tlsConfig["insecureSkipVerify"] = true
8989
}
9090

91-
dr.Object["spec"] = map[string]interface{}{
91+
dr.Object["spec"] = map[string]any{
9292
"host": spec.Endpoint,
93-
"trafficPolicy": map[string]interface{}{
93+
"trafficPolicy": map[string]any{
9494
"tls": tlsConfig,
9595
},
9696
}

maas-controller/pkg/reconciler/externalmodel/resources_test.go

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"testing"
55

66
"github.com/stretchr/testify/assert"
7+
"github.com/stretchr/testify/require"
78
)
89

910
func TestSanitize(t *testing.T) {
@@ -68,11 +69,16 @@ func TestBuildServiceEntry(t *testing.T) {
6869
assert.Equal(t, ModelServiceEntryName("my-gpt4"), se.GetName())
6970
assert.Equal(t, "llm", se.GetNamespace())
7071

71-
hosts := se.Object["spec"].(map[string]interface{})["hosts"].([]interface{})
72+
seSpec, ok := se.Object["spec"].(map[string]any)
73+
require.True(t, ok, "spec must be map[string]any")
74+
hosts, ok := seSpec["hosts"].([]any)
75+
require.True(t, ok, "hosts must be []any")
7276
assert.Equal(t, "api.openai.com", hosts[0])
7377

74-
ports := se.Object["spec"].(map[string]interface{})["ports"].([]interface{})
75-
port := ports[0].(map[string]interface{})
78+
ports, ok := seSpec["ports"].([]any)
79+
require.True(t, ok, "ports must be []any")
80+
port, ok := ports[0].(map[string]any)
81+
require.True(t, ok, "port must be map[string]any")
7682
assert.Equal(t, "https", port["name"])
7783
assert.Equal(t, "HTTPS", port["protocol"])
7884
}
@@ -87,9 +93,12 @@ func TestBuildServiceEntryNoTLS(t *testing.T) {
8793
labels := commonLabels("test-model")
8894

8995
se := BuildServiceEntry(spec, "test-model", "llm", labels)
90-
seSpec := se.Object["spec"].(map[string]interface{})
91-
ports := seSpec["ports"].([]interface{})
92-
port := ports[0].(map[string]interface{})
96+
seSpec, ok := se.Object["spec"].(map[string]any)
97+
require.True(t, ok, "spec must be map[string]any")
98+
ports, ok := seSpec["ports"].([]any)
99+
require.True(t, ok, "ports must be []any")
100+
port, ok := ports[0].(map[string]any)
101+
require.True(t, ok, "port must be map[string]any")
93102
assert.Equal(t, "HTTP", port["protocol"])
94103
assert.Equal(t, "http", port["name"])
95104
}
@@ -110,11 +119,15 @@ func TestBuildDestinationRule(t *testing.T) {
110119
assert.Equal(t, ModelDestinationRuleName("my-gpt4"), dr.GetName())
111120
assert.Equal(t, "llm", dr.GetNamespace())
112121

113-
drSpec := dr.Object["spec"].(map[string]interface{})
122+
drSpec, ok := dr.Object["spec"].(map[string]any)
123+
require.True(t, ok, "spec must be map[string]any")
114124
assert.Equal(t, "api.openai.com", drSpec["host"])
115125

116126
// Default: no insecureSkipVerify key
117-
tlsCfg := drSpec["trafficPolicy"].(map[string]interface{})["tls"].(map[string]interface{})
127+
tp, ok := drSpec["trafficPolicy"].(map[string]any)
128+
require.True(t, ok, "trafficPolicy must be map[string]any")
129+
tlsCfg, ok := tp["tls"].(map[string]any)
130+
require.True(t, ok, "tls must be map[string]any")
118131
assert.Equal(t, "SIMPLE", tlsCfg["mode"])
119132
_, hasInsecure := tlsCfg["insecureSkipVerify"]
120133
assert.False(t, hasInsecure, "insecureSkipVerify should not be set by default")
@@ -132,10 +145,14 @@ func TestBuildDestinationRuleInsecureSkipVerify(t *testing.T) {
132145

133146
dr := BuildDestinationRule(spec, "simulator-model", "llm", labels)
134147

135-
drSpec := dr.Object["spec"].(map[string]interface{})
148+
drSpec, ok := dr.Object["spec"].(map[string]any)
149+
require.True(t, ok, "spec must be map[string]any")
136150
assert.Equal(t, "3.150.113.9", drSpec["host"])
137151

138-
tlsCfg := drSpec["trafficPolicy"].(map[string]interface{})["tls"].(map[string]interface{})
152+
tp, ok := drSpec["trafficPolicy"].(map[string]any)
153+
require.True(t, ok, "trafficPolicy must be map[string]any")
154+
tlsCfg, ok := tp["tls"].(map[string]any)
155+
require.True(t, ok, "tls must be map[string]any")
139156
assert.Equal(t, "SIMPLE", tlsCfg["mode"])
140157
assert.Equal(t, true, tlsCfg["insecureSkipVerify"], "insecureSkipVerify must be true when opted in")
141158
}

0 commit comments

Comments
 (0)