From 3d9b703cabccb0d36b910bea6019abfd28805e9c Mon Sep 17 00:00:00 2001 From: Andreas Fritzler Date: Fri, 15 Nov 2024 16:50:25 +0100 Subject: [PATCH] Improve logging in route controller Improve logging in route controller. Addionally ensure that `envtest` is using the correct Kubernetes binaries. --- Makefile | 2 +- go.sum | 4 ++ pkg/cloudprovider/ironcore/routes.go | 75 +++++++++++++---------- pkg/cloudprovider/ironcore/routes_test.go | 8 --- pkg/cloudprovider/ironcore/suite_test.go | 13 +++- 5 files changed, 59 insertions(+), 43 deletions(-) diff --git a/Makefile b/Makefile index f7552d4..eae8e28 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,7 @@ BIN_NAME = "cloud-provider-ironcore" IMG ?= controller:latest # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary. -ENVTEST_K8S_VERSION = 1.30.3 +ENVTEST_K8S_VERSION = 1.30.0 ifeq (,$(shell go env GOBIN)) GOBIN=$(shell go env GOPATH)/bin diff --git a/go.sum b/go.sum index f3d7d1f..82ccf02 100644 --- a/go.sum +++ b/go.sum @@ -11,6 +11,8 @@ github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a h1:idn718Q4 github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= +github.com/bits-and-blooms/bitset v1.14.3 h1:Gd2c8lSNf9pKXom5JtD7AaKO8o7fGQ2LtFj1436qilA= +github.com/bits-and-blooms/bitset v1.14.3/go.mod h1:7hO7Gc7Pp1vODcmWvKMRA9BNmbv6a/7QIWpPxHddWR8= github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM= github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ= github.com/cenkalti/backoff/v4 v4.2.1 h1:y4OZtCnogmCPw98Zjyt5a6+QwPLGkiQsYW5oUqylYbM= @@ -129,6 +131,8 @@ github.com/modern-go/reflect2 v1.0.2 h1:xBagoLtFs94CBntxluKeaWgTMpvLxC4ur3nMaC9G github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= +github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f h1:y5//uYreIhSUg3J1GEMiLbxo1LJaP8RfCpH6pymGZus= +github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+o7JKHSa8/e818NopupXU1YMK5fe1lsApnBw= github.com/onsi/ginkgo/v2 v2.21.0 h1:7rg/4f3rB88pb5obDgNZrNHrQ4e6WpjonchcpuBRnZM= github.com/onsi/ginkgo/v2 v2.21.0/go.mod h1:7Du3c42kxCUegi0IImZ1wUQzMBVecgIHjR1C+NkhLQo= github.com/onsi/gomega v1.35.1 h1:Cwbd75ZBPxFSuZ6T+rN/WCb/gOc6YgFBXLlZLhC7Ds4= diff --git a/pkg/cloudprovider/ironcore/routes.go b/pkg/cloudprovider/ironcore/routes.go index 97d8e20..3e99d86 100644 --- a/pkg/cloudprovider/ironcore/routes.go +++ b/pkg/cloudprovider/ironcore/routes.go @@ -36,26 +36,29 @@ func newIroncoreRoutes(targetClient client.Client, ironcoreClient client.Client, } func (o ironcoreRoutes) ListRoutes(ctx context.Context, clusterName string) ([]*cloudprovider.Route, error) { - klog.V(2).InfoS("List Routes", "Cluster", clusterName) + klog.V(2).InfoS("Listing routes for cluster", "Cluster", clusterName) + // Retrieve network interfaces matching the given namespace, network, and cluster label networkInterfaces := &networkingv1alpha1.NetworkInterfaceList{} if err := o.ironcoreClient.List(ctx, networkInterfaces, client.InNamespace(o.ironcoreNamespace), client.MatchingFields{ networkInterfaceSpecNetworkRefNameField: o.cloudConfig.NetworkName, }, client.MatchingLabels{ LabelKeyClusterName: clusterName, }); err != nil { - return nil, err + return nil, fmt.Errorf("failed to list network interfaces for cluster %s: %w", clusterName, err) } var routes []*cloudprovider.Route - // iterate over all network interfaces and collect all the prefixes as Routes + // Iterate over each network interface and compile route information based on its prefixes for _, nic := range networkInterfaces.Items { + // Verify that the network interface is associated with a machine reference if nic.Spec.MachineRef != nil && nic.Spec.MachineRef.Name != "" { for _, prefix := range nic.Status.Prefixes { destinationCIDR := prefix.String() + // Retrieve node addresses based on the machine reference name targetNodeAddresses, err := o.getTargetNodeAddresses(ctx, nic.Spec.MachineRef.Name) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get target node addresses for machine %s: %w", nic.Spec.MachineRef.Name, err) } route := &cloudprovider.Route{ Name: clusterName + "-" + destinationCIDR, @@ -63,34 +66,34 @@ func (o ironcoreRoutes) ListRoutes(ctx context.Context, clusterName string) ([]* TargetNode: types.NodeName(nic.Spec.MachineRef.Name), TargetNodeAddresses: targetNodeAddresses, } - routes = append(routes, route) } } } - klog.V(2).InfoS("Current Routes", "Cluster", clusterName, "Network", o.cloudConfig.NetworkName, "Routes", routes) + klog.V(2).InfoS("Route listing completed", "Cluster", clusterName, "Network", o.cloudConfig.NetworkName, "RouteCount", len(routes)) return routes, nil } func (o ironcoreRoutes) CreateRoute(ctx context.Context, clusterName string, nameHint string, route *cloudprovider.Route) error { - klog.V(2).InfoS("Creating Route", "Cluster", clusterName, "Route", route, "NameHint", nameHint) + klog.V(2).InfoS("Initiating CreateRoute operation", "Cluster", clusterName, "Route", route, "NameHint", nameHint) - // get the machine object based on the node name + // Retrieve the machine object corresponding to the node name nodeName := string(route.TargetNode) machine := &computev1alpha1.Machine{} if err := o.ironcoreClient.Get(ctx, client.ObjectKey{Namespace: o.ironcoreNamespace, Name: nodeName}, machine); err != nil { if apierrors.IsNotFound(err) { + klog.V(2).InfoS("Machine object for node not found, returning InstanceNotFound", "Node", nodeName) return cloudprovider.InstanceNotFound } return fmt.Errorf("failed to get machine object for node %s: %w", nodeName, err) } + klog.V(3).InfoS("Machine object retrieval completed", "Node", nodeName) - // loop over all node addresses and find based on internal IP the matching network interface + // Iterate over all target node addresses and identify the matching network interface using internal IPs for _, address := range route.TargetNodeAddresses { - // only use internal IP addresses if address.Type == corev1.NodeInternalIP { - // loop over all network interfaces of the machine + klog.V(3).InfoS("Evaluating internal IP address", "Node", nodeName, "Address", address.Address) for _, networkInterface := range machine.Status.NetworkInterfaces { interfaceFound := false for _, p := range networkInterface.IPs { @@ -100,16 +103,18 @@ func (o ironcoreRoutes) CreateRoute(ctx context.Context, clusterName string, nam } } - // if the interface is found, add the prefix to the network interface + // If a matching network interface is found, proceed with prefix operations if interfaceFound { - // get the network interface object + klog.V(3).InfoS("Matching network interface identified", "Node", nodeName, "Address", address.Address) networkInterfaceName := getNetworkInterfaceName(machine, networkInterface) nic := &networkingv1alpha1.NetworkInterface{} if err := o.ironcoreClient.Get(ctx, client.ObjectKey{Namespace: o.ironcoreNamespace, Name: networkInterfaceName}, nic); err != nil { return err } - // check if the prefix is already added + klog.V(3).InfoS("NetworkInterface retrieval completed", "NetworkInterface", networkInterfaceName) + + // Check if the prefix already exists in the network interface prefixExists := false for _, prefix := range nic.Status.Prefixes { if prefix.Prefix.String() == route.DestinationCIDR { @@ -118,8 +123,9 @@ func (o ironcoreRoutes) CreateRoute(ctx context.Context, clusterName string, nam } } - // if the prefix is not added, add it + // If the prefix is not found, add it to the network interface specification if !prefixExists { + klog.V(3).InfoS("Prefix not detected, proceeding with addition", "NetworkInterface", networkInterfaceName, "Prefix", route.DestinationCIDR) nicBase := nic.DeepCopy() ipPrefix := commonv1alpha1.MustParseIPPrefix(route.DestinationCIDR) prefixSource := networkingv1alpha1.PrefixSource{ @@ -127,41 +133,43 @@ func (o ironcoreRoutes) CreateRoute(ctx context.Context, clusterName string, nam } nic.Spec.Prefixes = append(nic.Spec.Prefixes, prefixSource) - klog.V(2).InfoS("Updating NetworkInterface by adding prefix", "NetworkInterface", client.ObjectKeyFromObject(nic), "Node", nodeName, "Prefix", route.DestinationCIDR) + klog.V(2).InfoS("Applying patch to NetworkInterface to incorporate new prefix", "NetworkInterface", networkInterfaceName, "Node", nodeName, "Prefix", route.DestinationCIDR) if err := o.ironcoreClient.Patch(ctx, nic, client.MergeFrom(nicBase)); err != nil { - return fmt.Errorf("failed to patch NetworkInterface %s for Node %s: %w", client.ObjectKeyFromObject(nic), nodeName, err) + return fmt.Errorf("failed to patch NetworkInterface %s for Node %s: %w", networkInterfaceName, nodeName, err) } + klog.V(2).InfoS("Patch applied to NetworkInterface", "NetworkInterface", networkInterfaceName, "Prefix", route.DestinationCIDR) } else { - klog.V(2).InfoS("NetworkInterface prefix already exists", "NetworkInterface", client.ObjectKeyFromObject(nic), "Node", nodeName, "Prefix", route.DestinationCIDR) + klog.V(3).InfoS("Prefix already present on NetworkInterface", "NetworkInterface", networkInterfaceName, "Prefix", route.DestinationCIDR) } + } else { + klog.V(4).InfoS("No matching network interface identified for IP address", "Node", nodeName, "Address", address.Address) } } + } else { + klog.V(4).InfoS("Ignoring non-internal IP address", "Node", nodeName, "AddressType", address.Type, "Address", address.Address) } } - - klog.V(2).InfoS("Created Route", "Cluster", clusterName, "Route", route, "NameHint", nameHint) - + klog.V(2).InfoS("CreateRoute operation completed", "Cluster", clusterName, "Route", route, "NameHint", nameHint) return nil } func (o ironcoreRoutes) DeleteRoute(ctx context.Context, clusterName string, route *cloudprovider.Route) error { - klog.V(2).InfoS("Deleting Route", "Cluster", clusterName, "Route", route) + klog.V(2).InfoS("Initiating route deletion", "Cluster", clusterName, "Route", route) - // get the machine object based on the node name + // Retrieve the machine object based on the node name nodeName := string(route.TargetNode) machine := &computev1alpha1.Machine{} if err := o.ironcoreClient.Get(ctx, client.ObjectKey{Namespace: o.ironcoreNamespace, Name: nodeName}, machine); err != nil { if apierrors.IsNotFound(err) { return cloudprovider.InstanceNotFound } - return fmt.Errorf("failed to get machine object for node %s: %w", nodeName, err) + return fmt.Errorf("failed to retrieve machine object for node %s: %w", nodeName, err) } - // loop over all node addresses and find based on internal IP the matching network interface + // Iterate over all target node addresses and find matching network interfaces by internal IP for _, address := range route.TargetNodeAddresses { - // only use internal IP addresses if address.Type == corev1.NodeInternalIP { - // loop over all network interfaces of the machine + klog.V(3).InfoS("Evaluating internal IP address for network interface match", "Node", nodeName, "Address", address.Address) for _, networkInterface := range machine.Status.NetworkInterfaces { interfaceFound := false for _, p := range networkInterface.IPs { @@ -171,35 +179,36 @@ func (o ironcoreRoutes) DeleteRoute(ctx context.Context, clusterName string, rou } } - // if the interface is found, add the prefix to the network interface + // If a matching network interface is found, attempt to remove the prefix if interfaceFound { - // get the network interface object networkInterfaceName := getNetworkInterfaceName(machine, networkInterface) nic := &networkingv1alpha1.NetworkInterface{} if err := o.ironcoreClient.Get(ctx, client.ObjectKey{Namespace: o.ironcoreNamespace, Name: networkInterfaceName}, nic); err != nil { return err } - // check if the prefix exists + // Check for the prefix in the network interface's status and remove it if present for i, prefix := range nic.Status.Prefixes { if prefix.Prefix.String() == route.DestinationCIDR { nicBase := nic.DeepCopy() nic.Spec.Prefixes = append(nic.Spec.Prefixes[:i], nic.Spec.Prefixes[i+1:]...) - klog.V(2).InfoS("Prefix found and removed", "Prefix", prefix.Prefix.String(), "Prefixes after", nic.Spec.Prefixes) + klog.V(2).InfoS("Prefix identified and marked for removal", "Prefix", prefix.Prefix.String(), "UpdatedPrefixes", nic.Spec.Prefixes) if err := o.ironcoreClient.Patch(ctx, nic, client.MergeFrom(nicBase)); err != nil { return fmt.Errorf("failed to patch NetworkInterface %s for Node %s: %w", client.ObjectKeyFromObject(nic), nodeName, err) } - + klog.V(2).InfoS("Prefix removal patch applied", "NetworkInterface", networkInterfaceName, "Node", nodeName) break } } } } + } else { + klog.V(4).InfoS("Ignoring non-internal IP address", "Node", nodeName, "AddressType", address.Type, "Address", address.Address) } } - klog.V(2).InfoS("Deleted Route", "Cluster", clusterName, "Route", route) + klog.V(2).InfoS("Route deletion completed", "Cluster", clusterName, "Route", route) return nil } diff --git a/pkg/cloudprovider/ironcore/routes_test.go b/pkg/cloudprovider/ironcore/routes_test.go index 08fa952..da73890 100644 --- a/pkg/cloudprovider/ironcore/routes_test.go +++ b/pkg/cloudprovider/ironcore/routes_test.go @@ -376,14 +376,6 @@ var _ = Describe("Routes", func() { }, }, }, - { - Name: "ephemeral", - NetworkInterfaceSource: computev1alpha1.NetworkInterfaceSource{ - NetworkInterfaceRef: &corev1.LocalObjectReference{ - Name: fmt.Sprintf("%s-%s", machine.Name, "ephemeral"), - }, - }, - }, } machine.Status.State = computev1alpha1.MachineStateRunning machine.Status.NetworkInterfaces = []computev1alpha1.NetworkInterfaceStatus{ diff --git a/pkg/cloudprovider/ironcore/suite_test.go b/pkg/cloudprovider/ironcore/suite_test.go index 0f473b7..5d2f01c 100644 --- a/pkg/cloudprovider/ironcore/suite_test.go +++ b/pkg/cloudprovider/ironcore/suite_test.go @@ -5,7 +5,10 @@ package ironcore import ( "context" + "fmt" "os" + "path/filepath" + "runtime" "testing" "time" @@ -70,7 +73,15 @@ var _ = BeforeSuite(func() { var err error By("bootstrapping test environment") - testEnv = &envtest.Environment{} + testEnv = &envtest.Environment{ + // The BinaryAssetsDirectory is only required if you want to run the tests directly + // without call the makefile target test. If not informed it will look for the + // default path defined in controller-runtime which is /usr/local/kubebuilder/. + // Note that you must have the required binaries setup under the bin directory to perform + // the tests directly. When we run make test it will be setup and used automatically. + BinaryAssetsDirectory: filepath.Join("..", "..", "..", "bin", "k8s", + fmt.Sprintf("1.30.0-%s-%s", runtime.GOOS, runtime.GOARCH)), + } testEnvExt = &envtestext.EnvironmentExtensions{ APIServiceDirectoryPaths: []string{ modutils.Dir("github.com/ironcore-dev/ironcore", "config", "apiserver", "apiservice", "bases"),