Fix import shadowing and unchecked type assertions in test (#2574)

Author: lucacome

.golangci.yml

@@ -9,28 +9,36 @@ linters-settings:
     ignore-generated-header: true
     rules:
       - name: blank-imports
+      - name: constant-logical-expr
       - name: context-as-argument
       - name: context-keys-type
+      - name: defer
       - name: dot-imports
         arguments:
           - allowedPackages:
               - github.com/onsi/gomega
               - github.com/onsi/ginkgo/v2
+      - name: duplicated-imports
       - name: empty-block
       - name: error-naming
       - name: error-return
       - name: error-strings
       - name: errorf
       - name: exported
+      - name: import-shadowing
       - name: increment-decrement
       - name: indent-error-flow
       - name: package-comments
       - name: range
+      - name: range-val-address
+      - name: range-val-in-closure
       - name: receiver-naming
       - name: redefines-builtin-id
+      - name: string-of-int
       - name: superfluous-else
       - name: time-naming
       - name: unexported-return
+      - name: unnecessary-stmt
       - name: unreachable-code
       - name: unused-parameter
       - name: var-declaration
@@ -80,9 +88,11 @@ linters:
     - lll
     - loggercheck
     - makezero
+    - mirror
     - misspell
     - musttag
     - nilerr
+    - nilnil
     - noctx
     - nolintlint
     - predeclared

internal/framework/controller/reconciler_test.go

@@ -66,8 +66,11 @@ var _ = Describe("Reconciler", func() {
 		) error {
 			Expect(object).To(BeAssignableToTypeOf(&v1.HTTPRoute{}))
 			Expect(nsname).To(Equal(client.ObjectKeyFromObject(hr)))
+			Expect(hr).ToNot(BeNil())
 
-			hr.DeepCopyInto(object.(*v1.HTTPRoute))
+			hrObj, ok := object.(*v1.HTTPRoute)
+			Expect(ok).To(BeTrue(), "object is not *v1.HTTPRoute")
+			hr.DeepCopyInto(hrObj)
 
 			return nil
 		}

internal/framework/events/first_eventbatch_preparer_test.go

@@ -108,7 +108,8 @@ var _ = Describe("FirstEventBatchPreparer", func() {
 				fakeReader.ListCalls(
 					func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error {
 						httpRoute := v1.HTTPRoute{ObjectMeta: metav1.ObjectMeta{Name: "test"}}
-						typedList := list.(*v1.HTTPRouteList)
+						typedList, ok := list.(*v1.HTTPRouteList)
+						Expect(ok).To(BeTrue(), "expected list to be of type *v1.HTTPRouteList")
 						typedList.Items = append(typedList.Items, httpRoute)
 
 						return nil

internal/framework/helpers/helpers.go

@@ -33,12 +33,12 @@ func GetPointer[T any](v T) *T {
 // making it is possible to use it in tests when comparing against values returned by the fake client.
 // It panics if it fails to process the time.
 func PrepareTimeForFakeClient(t metav1.Time) metav1.Time {
-	bytes, err := t.Marshal()
+	b, err := t.Marshal()
 	if err != nil {
 		panic(fmt.Errorf("failed to marshal time: %w", err))
 	}
 
-	if err = t.Unmarshal(bytes); err != nil {
+	if err = t.Unmarshal(b); err != nil {
 		panic(fmt.Errorf("failed to unmarshal time: %w", err))
 	}
 
@@ -78,10 +78,10 @@ func EqualPointers[T comparable](p1, p2 *T) bool {
 }
 
 // MustExecuteTemplate executes the template with the given data.
-func MustExecuteTemplate(template *template.Template, data interface{}) []byte {
+func MustExecuteTemplate(templ *template.Template, data interface{}) []byte {
 	var buf bytes.Buffer
 
-	if err := template.Execute(&buf, data); err != nil {
+	if err := templ.Execute(&buf, data); err != nil {
 		panic(err)
 	}
 

internal/framework/status/leader_aware_group_updater_test.go

@@ -59,7 +59,8 @@ var _ = Describe("LeaderAwareGroupUpdater", func() {
 
 			if updateNeeded {
 				setter = func(obj client.Object) bool {
-					gc := obj.(*v1.GatewayClass)
+					gc, ok := obj.(*v1.GatewayClass)
+					Expect(ok).To(BeTrue(), "obj is not a *v1.GatewayClass")
 					gc.Status = createGCStatus(condType)
 					return true
 				}

internal/framework/status/updater.go

@@ -54,9 +54,9 @@ type Updater struct {
 }
 
 // NewUpdater creates a new Updater.
-func NewUpdater(client client.Client, logger logr.Logger) *Updater {
+func NewUpdater(c client.Client, logger logr.Logger) *Updater {
 	return &Updater{
-		client: client,
+		client: c,
 		logger: logger,
 	}
 }

internal/framework/status/updater_test.go

@@ -69,7 +69,8 @@ func prepareReq(name string, condType string, updateNeeded bool) UpdateRequest {
 	var setter Setter
 	if updateNeeded {
 		setter = func(obj client.Object) bool {
-			gc := obj.(*v1.GatewayClass)
+			gc, ok := obj.(*v1.GatewayClass)
+			Expect(ok).To(BeTrue(), "obj is not a *v1.GatewayClass")
 			gc.Status = createGCStatus(condType)
 			return true
 		}

internal/mode/static/handler.go

@@ -181,7 +181,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 		h.parseAndCaptureEvent(ctx, logger, event)
 	}
 
-	changeType, graph := h.cfg.processor.Process()
+	changeType, gr := h.cfg.processor.Process()
 
 	var err error
 	switch changeType {
@@ -193,7 +193,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 		return
 	case state.EndpointsOnlyChange:
 		h.version++
-		cfg := dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.version)
+		cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
 
 		h.setLatestConfiguration(&cfg)
 
@@ -204,7 +204,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 		)
 	case state.ClusterStateChange:
 		h.version++
-		cfg := dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.version)
+		cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
 
 		h.setLatestConfiguration(&cfg)
 
@@ -230,10 +230,10 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 
 	h.latestReloadResult = nginxReloadRes
 
-	h.updateStatuses(ctx, logger, graph)
+	h.updateStatuses(ctx, logger, gr)
 }
 
-func (h *eventHandlerImpl) updateStatuses(ctx context.Context, logger logr.Logger, graph *graph.Graph) {
+func (h *eventHandlerImpl) updateStatuses(ctx context.Context, logger logr.Logger, gr *graph.Graph) {
 	gwAddresses, err := getGatewayAddresses(ctx, h.cfg.k8sClient, nil, h.cfg.gatewayPodConfig)
 	if err != nil {
 		logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address")
@@ -243,18 +243,18 @@ func (h *eventHandlerImpl) updateStatuses(ctx context.Context, logger logr.Logge
 
 	var gcReqs []frameworkStatus.UpdateRequest
 	if h.cfg.updateGatewayClassStatus {
-		gcReqs = status.PrepareGatewayClassRequests(graph.GatewayClass, graph.IgnoredGatewayClasses, transitionTime)
+		gcReqs = status.PrepareGatewayClassRequests(gr.GatewayClass, gr.IgnoredGatewayClasses, transitionTime)
 	}
 	routeReqs := status.PrepareRouteRequests(
-		graph.L4Routes,
-		graph.Routes,
+		gr.L4Routes,
+		gr.Routes,
 		transitionTime,
 		h.latestReloadResult,
 		h.cfg.gatewayCtlrName,
 	)
 
-	polReqs := status.PrepareBackendTLSPolicyRequests(graph.BackendTLSPolicies, transitionTime, h.cfg.gatewayCtlrName)
-	ngfPolReqs := status.PrepareNGFPolicyRequests(graph.NGFPolicies, transitionTime, h.cfg.gatewayCtlrName)
+	polReqs := status.PrepareBackendTLSPolicyRequests(gr.BackendTLSPolicies, transitionTime, h.cfg.gatewayCtlrName)
+	ngfPolReqs := status.PrepareNGFPolicyRequests(gr.NGFPolicies, transitionTime, h.cfg.gatewayCtlrName)
 
 	reqs := make([]frameworkStatus.UpdateRequest, 0, len(gcReqs)+len(routeReqs)+len(polReqs)+len(ngfPolReqs))
 	reqs = append(reqs, gcReqs...)
@@ -267,8 +267,8 @@ func (h *eventHandlerImpl) updateStatuses(ctx context.Context, logger logr.Logge
 	// We put Gateway status updates separately from the rest of the statuses because we want to be able
 	// to update them separately from the rest of the graph whenever the public IP of NGF changes.
 	gwReqs := status.PrepareGatewayRequests(
-		graph.Gateway,
-		graph.IgnoredGateways,
+		gr.Gateway,
+		gr.IgnoredGateways,
 		transitionTime,
 		gwAddresses,
 		h.latestReloadResult,
@@ -558,15 +558,15 @@ func (h *eventHandlerImpl) nginxGatewayServiceUpsert(ctx context.Context, logger
 		logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address")
 	}
 
-	graph := h.cfg.processor.GetLatestGraph()
-	if graph == nil {
+	gr := h.cfg.processor.GetLatestGraph()
+	if gr == nil {
 		return
 	}
 
 	transitionTime := metav1.Now()
 	gatewayStatuses := status.PrepareGatewayRequests(
-		graph.Gateway,
-		graph.IgnoredGateways,
+		gr.Gateway,
+		gr.IgnoredGateways,
 		transitionTime,
 		gwAddresses,
 		h.latestReloadResult,
@@ -584,15 +584,15 @@ func (h *eventHandlerImpl) nginxGatewayServiceDelete(
 		logger.Error(err, "Setting GatewayStatusAddress to Pod IP Address")
 	}
 
-	graph := h.cfg.processor.GetLatestGraph()
-	if graph == nil {
+	gr := h.cfg.processor.GetLatestGraph()
+	if gr == nil {
 		return
 	}
 
 	transitionTime := metav1.Now()
 	gatewayStatuses := status.PrepareGatewayRequests(
-		graph.Gateway,
-		graph.IgnoredGateways,
+		gr.Gateway,
+		gr.IgnoredGateways,
 		transitionTime,
 		gwAddresses,
 		h.latestReloadResult,

internal/mode/static/manager.go

@@ -715,14 +715,14 @@ func setInitialConfig(
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()
 
-	var config ngfAPI.NginxGateway
+	var conf ngfAPI.NginxGateway
 	// Polling to wait for CRD to exist if the Deployment is created first.
 	if err := wait.PollUntilContextCancel(
 		ctx,
 		500*time.Millisecond,
 		true, /* poll immediately */
 		func(ctx context.Context) (bool, error) {
-			if err := reader.Get(ctx, configName, &config); err != nil {
+			if err := reader.Get(ctx, configName, &conf); err != nil {
 				if !apierrors.IsNotFound(err) {
 					return false, err
 				}
@@ -736,7 +736,7 @@ func setInitialConfig(
 
 	// status is not updated until the status updater's cache is started and the
 	// resource is processed by the controller
-	return updateControlPlane(&config, logger, eventRecorder, configName, logLevelSetter)
+	return updateControlPlane(&conf, logger, eventRecorder, configName, logLevelSetter)
 }
 
 func getMetricsOptions(cfg config.MetricsConfig) metricsserver.Options {

internal/mode/static/state/dataplane/configuration.go

@@ -607,7 +607,7 @@ func (hpr *hostPathRules) maxServerCount() int {
 func buildUpstreams(
 	ctx context.Context,
 	listeners []*graph.Listener,
-	resolver resolver.ServiceResolver,
+	svcResolver resolver.ServiceResolver,
 	ipFamily IPFamilyType,
 ) []Upstream {
 	// There can be duplicate upstreams if multiple routes reference the same upstream.
@@ -643,7 +643,7 @@ func buildUpstreams(
 
 						var errMsg string
 
-						eps, err := resolver.Resolve(ctx, br.SvcNsName, br.ServicePort, allowedAddressType)
+						eps, err := svcResolver.Resolve(ctx, br.SvcNsName, br.ServicePort, allowedAddressType)
 						if err != nil {
 							errMsg = err.Error()
 						}

internal/mode/static/state/graph/gateway_listener.go

@@ -306,8 +306,8 @@ func validateListenerAllowedRouteKind(listener v1.Listener) (conds []conditions.
 }
 
 func getListenerSupportedKinds(listener v1.Listener) []v1.RouteGroupKind {
-	_, kinds := getAndValidateListenerSupportedKinds(listener)
-	return kinds
+	_, sk := getAndValidateListenerSupportedKinds(listener)
+	return sk
 }
 
 func validateListenerLabelSelector(listener v1.Listener) (conds []conditions.Condition, attachable bool) {

internal/mode/static/state/graph/policies.go

@@ -154,19 +154,19 @@ func attachPolicyToGateway(
 }
 
 func processPolicies(
-	policies map[PolicyKey]policies.Policy,
+	pols map[PolicyKey]policies.Policy,
 	validator validation.PolicyValidator,
 	gateways processedGateways,
 	routes map[RouteKey]*L7Route,
 	globalSettings *policies.GlobalSettings,
 ) map[PolicyKey]*Policy {
-	if len(policies) == 0 || gateways.Winner == nil {
+	if len(pols) == 0 || gateways.Winner == nil {
 		return nil
 	}
 
 	processedPolicies := make(map[PolicyKey]*Policy)
 
-	for key, policy := range policies {
+	for key, policy := range pols {
 		var conds []conditions.Condition
 
 		targetRefs := make([]PolicyTargetRef, 0, len(policy.GetTargetRefs()))
@@ -280,7 +280,7 @@ func buildHostPortPaths(route *L7Route) map[string]string {
 
 // markConflictedPolicies marks policies that conflict with a policy of greater precedence as invalid.
 // Policies are sorted by timestamp and then alphabetically.
-func markConflictedPolicies(policies map[PolicyKey]*Policy, validator validation.PolicyValidator) {
+func markConflictedPolicies(pols map[PolicyKey]*Policy, validator validation.PolicyValidator) {
 	// Policies can only conflict if they are the same policy type (gvk) and they target the same resource(s).
 	type key struct {
 		policyGVK schema.GroupVersionKind
@@ -289,7 +289,7 @@ func markConflictedPolicies(policies map[PolicyKey]*Policy, validator validation
 
 	possibles := make(map[key][]*Policy)
 
-	for policyKey, policy := range policies {
+	for policyKey, policy := range pols {
 		// If a policy is invalid, it cannot conflict with another policy.
 		if policy.Valid {
 			for _, ref := range policy.TargetRefs {

internal/mode/static/state/resolver/resolver.go

@@ -44,8 +44,8 @@ type ServiceResolverImpl struct {
 }
 
 // NewServiceResolverImpl creates a new instance of a ServiceResolverImpl.
-func NewServiceResolverImpl(client client.Client) *ServiceResolverImpl {
-	return &ServiceResolverImpl{client: client}
+func NewServiceResolverImpl(c client.Client) *ServiceResolverImpl {
+	return &ServiceResolverImpl{client: c}
 }
 
 // Resolve resolves a Service's NamespacedName and ServicePort to a list of Endpoints.

internal/mode/static/state/store.go

@@ -36,11 +36,11 @@ type ngfPolicyObjectStore struct {
 
 // newNGFPolicyObjectStore returns a new ngfPolicyObjectStore.
 func newNGFPolicyObjectStore(
-	policies map[graph.PolicyKey]policies.Policy,
+	pols map[graph.PolicyKey]policies.Policy,
 	gvkFunc kinds.MustExtractGVK,
 ) *ngfPolicyObjectStore {
 	return &ngfPolicyObjectStore{
-		policies:       policies,
+		policies:       pols,
 		extractGVKFunc: gvkFunc,
 	}
 }