From 5bf46247bc4dc63312ca326ab4a75b82990b5ab6 Mon Sep 17 00:00:00 2001 From: Marcel Fest Date: Mon, 18 Mar 2024 18:14:06 +0100 Subject: [PATCH 1/5] Descriptions get a new format controller-gen@0.14.0 --- ...elekom.de_layer2networkconfigurations.yaml | 48 ++++++++++--------- ...twork.schiff.telekom.de_routingtables.yaml | 19 +++++--- ...iff.telekom.de_vrfrouteconfigurations.yaml | 19 +++++--- 3 files changed, 50 insertions(+), 36 deletions(-) diff --git a/config/crd/bases/network.schiff.telekom.de_layer2networkconfigurations.yaml b/config/crd/bases/network.schiff.telekom.de_layer2networkconfigurations.yaml index feec7d51..9d8d6ea3 100644 --- a/config/crd/bases/network.schiff.telekom.de_layer2networkconfigurations.yaml +++ b/config/crd/bases/network.schiff.telekom.de_layer2networkconfigurations.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.12.0 + controller-gen.kubebuilder.io/version: v0.14.0 name: layer2networkconfigurations.network.schiff.telekom.de spec: group: network.schiff.telekom.de @@ -37,14 +37,19 @@ spec: API. properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -86,24 +91,24 @@ spec: description: matchExpressions is a list of label selector requirements. The requirements are ANDed. items: - description: A label selector requirement is a selector that - contains values, a key, and an operator that relates the key - and values. + description: |- + A label selector requirement is a selector that contains values, a key, and an operator that + relates the key and values. properties: key: description: key is the label key that the selector applies to. type: string operator: - description: operator represents a key's relationship to - a set of values. Valid operators are In, NotIn, Exists - and DoesNotExist. + description: |- + operator represents a key's relationship to a set of values. + Valid operators are In, NotIn, Exists and DoesNotExist. type: string values: - description: values is an array of string values. If the - operator is In or NotIn, the values array must be non-empty. - If the operator is Exists or DoesNotExist, the values - array must be empty. This array is replaced during a strategic + description: |- + values is an array of string values. If the operator is In or NotIn, + the values array must be non-empty. If the operator is Exists or DoesNotExist, + the values array must be empty. This array is replaced during a strategic merge patch. items: type: string @@ -116,11 +121,10 @@ spec: matchLabels: additionalProperties: type: string - description: matchLabels is a map of {key,value} pairs. A single - {key,value} in the matchLabels map is equivalent to an element - of matchExpressions, whose key field is "key", the operator - is "In", and the values array contains only "value". The requirements - are ANDed. + description: |- + matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels + map is equivalent to an element of matchExpressions, whose key field is "key", the + operator is "In", and the values array contains only "value". The requirements are ANDed. type: object type: object x-kubernetes-map-type: atomic diff --git a/config/crd/bases/network.schiff.telekom.de_routingtables.yaml b/config/crd/bases/network.schiff.telekom.de_routingtables.yaml index 2a4d7d1c..caff4038 100644 --- a/config/crd/bases/network.schiff.telekom.de_routingtables.yaml +++ b/config/crd/bases/network.schiff.telekom.de_routingtables.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.12.0 + controller-gen.kubebuilder.io/version: v0.14.0 name: routingtables.network.schiff.telekom.de spec: group: network.schiff.telekom.de @@ -26,14 +26,19 @@ spec: description: RoutingTable is the Schema for the routingtables API. properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object diff --git a/config/crd/bases/network.schiff.telekom.de_vrfrouteconfigurations.yaml b/config/crd/bases/network.schiff.telekom.de_vrfrouteconfigurations.yaml index 12ba94fe..9cfb0749 100644 --- a/config/crd/bases/network.schiff.telekom.de_vrfrouteconfigurations.yaml +++ b/config/crd/bases/network.schiff.telekom.de_vrfrouteconfigurations.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.12.0 + controller-gen.kubebuilder.io/version: v0.14.0 name: vrfrouteconfigurations.network.schiff.telekom.de spec: group: network.schiff.telekom.de @@ -33,14 +33,19 @@ spec: API. properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object From 7f668299b1cbbf768dc74bc18139a3804d0a71cf Mon Sep 17 00:00:00 2001 From: Marcel Fest Date: Mon, 18 Mar 2024 18:17:48 +0100 Subject: [PATCH 2/5] Update go comment --- api/v1alpha1/zz_generated.deepcopy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index c923095a..ecaa9148 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -1,5 +1,4 @@ //go:build !ignore_autogenerated -// +build !ignore_autogenerated /* Copyright 2022. From faa5e7c13f3881cab90373ba6d3f4aa5f84fd0fd Mon Sep 17 00:00:00 2001 From: Marcel Fest Date: Mon, 18 Mar 2024 18:27:13 +0100 Subject: [PATCH 3/5] Fix emptyString check from linter suggestion --- cmd/manager/main.go | 2 +- pkg/nl/layer2.go | 4 ++-- pkg/reconciler/layer2.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index a1ca9cbc..44c44e90 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -151,7 +151,7 @@ func main() { os.Exit(1) } - if len(interfacePrefix) > 0 { + if interfacePrefix != "" { setupLog.Info("start macvlan sync") macvlan.RunMACSync(interfacePrefix) } diff --git a/pkg/nl/layer2.go b/pkg/nl/layer2.go index a604e186..6e31c34c 100644 --- a/pkg/nl/layer2.go +++ b/pkg/nl/layer2.go @@ -108,7 +108,7 @@ func (*NetlinkManager) ParseIPAddresses(addresses []string) ([]*netlink.Addr, er func (n *NetlinkManager) CreateL2(info *Layer2Information) error { masterIdx := -1 - if len(info.VRF) > 0 { + if info.VRF != "" { l3Info, err := n.GetL3ByName(info.VRF) if err != nil { return err @@ -376,7 +376,7 @@ func (n *NetlinkManager) isL2VNIreattachRequired(current, desired *Layer2Informa // Reconcile VRF if current.VRF != desired.VRF { shouldReattachL2VNI = true - if len(desired.VRF) > 0 { + if desired.VRF != "" { l3Info, err := n.GetL3ByName(desired.VRF) if err != nil { return shouldReattachL2VNI, fmt.Errorf("error while getting L3 by name: %w", err) diff --git a/pkg/reconciler/layer2.go b/pkg/reconciler/layer2.go index 49760c33..941b28f6 100644 --- a/pkg/reconciler/layer2.go +++ b/pkg/reconciler/layer2.go @@ -166,7 +166,7 @@ func (r *reconcile) getDesired(l2vnis []networkv1alpha1.Layer2NetworkConfigurati return nil, fmt.Errorf("error parsing anycast gateways: %w", err) } - if len(spec.VRF) > 0 { + if spec.VRF != "" { vrfAvailable := false for _, info := range availableVrfs { if info.Name == spec.VRF { From 2bb949bfeeae53677cdcc0fb9f6bcf064a124076 Mon Sep 17 00:00:00 2001 From: Marcel Fest Date: Tue, 19 Mar 2024 07:29:46 +0100 Subject: [PATCH 4/5] Fix typecasts and added exclusions --- .golangci.yml | 16 +++++--- .../layer2networkconfiguration_controller.go | 6 +-- pkg/frr/manager.go | 12 +++++- pkg/healthcheck/healthcheck.go | 4 +- pkg/healthcheck/healthcheck_test.go | 10 +++-- pkg/nl/list.go | 38 +++++++++++++++---- 6 files changed, 62 insertions(+), 24 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 1df08e53..4cffff70 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -66,12 +66,11 @@ linters-settings: alias: ctrl nolintlint: allow-unused: false - allow-leading-space: false require-specific: true staticcheck: - go: "1.19" + go: "1.22" stylecheck: - go: "1.19" + go: "1.22" gocritic: enabled-tags: - diagnostic @@ -82,8 +81,6 @@ linters-settings: disabled-checks: - regexpSimplify - whyNoLint - unused: - go: "1.19" cyclop: max-complexity: 13 revive: @@ -154,6 +151,15 @@ issues: text: "ST1003: should not use ALL_CAPS in Go names; use CamelCase instead" - path: '(.+)/unix/(coil|frr|nl)\.go' text: "var-naming: don't use ALL_CAPS in Go names; use CamelCase" + - path: '(.+)/nl/(route|layer2)\.go' + text: 'import-alias-naming:.+\(schiff_unix\).*$' + # Test file exclusions + - path: '(.+)_test\.go' + text: 'unchecked-type-assertion:' + - path: '(.+)_test\.go' + text: 'import-alias-naming:' + - path: '(.+)_test\.go' + text: 'dot-imports:' # fix issues i currently can't fix. - path: '(.+)\.go' text: "string `vrf` has (\\d+) occurrences, make it a constant" diff --git a/controllers/layer2networkconfiguration_controller.go b/controllers/layer2networkconfiguration_controller.go index 03076684..54396c27 100644 --- a/controllers/layer2networkconfiguration_controller.go +++ b/controllers/layer2networkconfiguration_controller.go @@ -71,12 +71,12 @@ func (r *Layer2NetworkConfigurationReconciler) SetupWithManager(mgr ctrl.Manager // Create empty request for changes to node nodesMapFn := handler.EnqueueRequestsFromMapFunc(func(_ context.Context, _ client.Object) []reconcile.Request { return []reconcile.Request{{}} }) nodePredicates := predicate.Funcs{ - CreateFunc: func(e event.CreateEvent) bool { return false }, + CreateFunc: func(_ event.CreateEvent) bool { return false }, UpdateFunc: func(e event.UpdateEvent) bool { return os.Getenv(healthcheck.NodenameEnv) == e.ObjectNew.GetName() && !cmp.Equal(e.ObjectNew.GetLabels(), e.ObjectOld.GetLabels()) }, - DeleteFunc: func(e event.DeleteEvent) bool { return false }, - GenericFunc: func(e event.GenericEvent) bool { return false }, + DeleteFunc: func(_ event.DeleteEvent) bool { return false }, + GenericFunc: func(_ event.GenericEvent) bool { return false }, } err := ctrl.NewControllerManagedBy(mgr). diff --git a/pkg/frr/manager.go b/pkg/frr/manager.go index 1cc55668..6a1e7fc9 100644 --- a/pkg/frr/manager.go +++ b/pkg/frr/manager.go @@ -134,8 +134,16 @@ func (*Manager) GetStatusFRR() (activeState, subState string, err error) { if err != nil { return "", "", fmt.Errorf("error getting unit %s properties: %w", frrUnit, err) } - - return prop["ActiveState"].(string), prop["SubState"].(string), nil + var ok bool + activeState, ok = prop["ActiveState"].(string) + if !ok { + return "", "", fmt.Errorf("error casting property %v [\"ActiveState\"] as string", prop) + } + subState, ok = prop["SubState"].(string) + if !ok { + return activeState, "", fmt.Errorf("error casting property %v [\"SubState\"] as string", prop) + } + return activeState, subState, nil } func (v *VRFConfiguration) ShouldTemplateVRF() bool { diff --git a/pkg/healthcheck/healthcheck.go b/pkg/healthcheck/healthcheck.go index 06187a79..b3084b4c 100644 --- a/pkg/healthcheck/healthcheck.go +++ b/pkg/healthcheck/healthcheck.go @@ -157,9 +157,7 @@ func (hc *HealthChecker) CheckReachability() error { // just actively refuses connections (e.g. port is blocked) continue } - if err != nil { - return err - } + return err } } return nil diff --git a/pkg/healthcheck/healthcheck_test.go b/pkg/healthcheck/healthcheck_test.go index 1de05ae1..4326397f 100644 --- a/pkg/healthcheck/healthcheck_test.go +++ b/pkg/healthcheck/healthcheck_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "net" "os" "testing" @@ -346,10 +347,13 @@ func (*updateErrorClient) Update( } func (*updateErrorClient) Get(_ context.Context, _ types.NamespacedName, o client.Object, _ ...client.GetOption) error { - a := o.(*corev1.Node) - a.Name = testHostname + node, ok := o.(*corev1.Node) + if !ok { + return fmt.Errorf("error casting object %v as corev1.Node", o) + } + node.Name = testHostname for _, t := range InitTaints { - a.Spec.Taints = append(a.Spec.Taints, corev1.Taint{ + node.Spec.Taints = append(node.Spec.Taints, corev1.Taint{ Key: t, Effect: corev1.TaintEffectNoSchedule, }) diff --git a/pkg/nl/list.go b/pkg/nl/list.go index f188c08a..0d2b5228 100644 --- a/pkg/nl/list.go +++ b/pkg/nl/list.go @@ -40,15 +40,18 @@ func (*NetlinkManager) ListVRFInterfaces() ([]VRFInformation, error) { links, err := netlink.LinkList() if err != nil { - return nil, fmt.Errorf("cannot get links from netlink: %w", err) + return nil, fmt.Errorf("error listing links: %w", err) } for _, link := range links { if link.Type() != "vrf" { continue } - vrf := link.(*netlink.Vrf) + vrf, ok := link.(*netlink.Vrf) + if !ok { + return nil, fmt.Errorf("error casting link %v as netlink.Vrf", link) + } info := VRFInformation{} info.table = int(vrf.Table) info.Name = link.Attrs().Name @@ -71,8 +74,10 @@ func (n *NetlinkManager) ListL3() ([]VRFInformation, error) { if !(link.Type() == "vrf" && strings.HasPrefix(link.Attrs().Name, vrfPrefix)) { continue } - vrf := link.(*netlink.Vrf) - + vrf, ok := link.(*netlink.Vrf) + if !ok { + return nil, fmt.Errorf("error casting link %v as netlink.Vrf", link) + } info := VRFInformation{} info.table = int(vrf.Table) info.Name = link.Attrs().Name[3:] @@ -146,13 +151,21 @@ func (*NetlinkManager) updateL2Indices(info *Layer2Information, links []netlink. func updateLink(info *Layer2Information, link netlink.Link) error { // If subinterface is VXLAN if link.Type() == "vxlan" && strings.HasPrefix(link.Attrs().Name, vxlanPrefix) { - info.vxlan = link.(*netlink.Vxlan) + vxlan, ok := link.(*netlink.Vxlan) + if !ok { + return fmt.Errorf("error casting link %v as netlink.Vxlan", link) + } + info.vxlan = vxlan info.VNI = info.vxlan.VxlanId } // If subinterface is VETH if link.Type() == "veth" && strings.HasPrefix(link.Attrs().Name, vethL2Prefix) { - info.macvlanBridge = link.(*netlink.Veth) + macvlanBridge, ok := link.(*netlink.Veth) + if !ok { + return fmt.Errorf("error casting link %v as netlink.Veth", link) + } + info.macvlanBridge = macvlanBridge peerIdx, err := netlink.VethPeerIndex(info.macvlanBridge) if err != nil { return fmt.Errorf("error getting veth perr by index: %w", err) @@ -161,7 +174,11 @@ func updateLink(info *Layer2Information, link netlink.Link) error { if err != nil { return fmt.Errorf("error getting link by index: %w", err) } - info.macvlanHost = peerInterface.(*netlink.Veth) + macvlanHost, ok := peerInterface.(*netlink.Veth) + if !ok { + return fmt.Errorf("error casting link %v as netlink.Veth", link) + } + info.macvlanHost = macvlanHost info.CreateMACVLANInterface = true } @@ -181,7 +198,12 @@ func (n *NetlinkManager) ListL2() ([]Layer2Information, error) { continue } info := Layer2Information{} - info.bridge = link.(*netlink.Bridge) + + bridge, ok := link.(*netlink.Bridge) + if !ok { + return nil, fmt.Errorf("cannot cast link %v as netlink.Bridge", link) + } + info.bridge = bridge info.AnycastMAC = &info.bridge.HardwareAddr info.MTU = info.bridge.MTU vlanID, err := strconv.Atoi(info.bridge.Name[3:]) From 100c33d61074e2bfec88031fa6969446315081d9 Mon Sep 17 00:00:00 2001 From: Marcel Fest Date: Tue, 19 Mar 2024 07:37:10 +0100 Subject: [PATCH 5/5] Use a newer golang ci lint version --- .github/workflows/pullrequests.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pullrequests.yaml b/.github/workflows/pullrequests.yaml index 2681cc11..bd984a28 100644 --- a/.github/workflows/pullrequests.yaml +++ b/.github/workflows/pullrequests.yaml @@ -23,7 +23,7 @@ jobs: - name: golangci-lint uses: golangci/golangci-lint-action@v4.0.0 with: - version: v1.53.2 + version: v1.56.2 args: --timeout=10m test: