From 1fb8c378d31b22b698c0869b4e725dc9f211cd71 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Mon, 6 Nov 2023 09:38:52 -0500 Subject: [PATCH] Fix nil interface{} checks PR https://github.com/submariner-io/shipyard/pull/1447 changed places where we check for nil interface{} in the callback functions for AwaitUntil to cast first. However this can result in an invalid type conversion panic depending on how the value is returned from the operation function. In Go, an interface var contains both a type and value so there are 2 cases for a nil: 1) The type is non-nil and the value is nil 2) The type and value are nil For #1, the interface{} var is not actually nil so checking 'if result == nil' yields false. Eg, var s *string var i interface{} = s if i == nil { // yields false b/c i is typed } In this case, we can cast to the expected type and then check for nil. s := i.(*string) if s == nil { // yields true } For #2, the interface{} var is untyped so nil check yields true: var i interface{} = nil if i == nil { // yields true } So in cases where we directly return nil from an operation function, the nil check in the result function is valid. The actual reason for #1447 was b/c of a nil pointer panic in AwaitGatewaysWithStatus. This function was previously changed to call 'findGateway' and return its values. However 'findGateway' returns *unstructured.Unstructured so now the operation function returned a typed interface{} var. So if 'findGateway' returns a nil *unstructured.Unstructured value, the 'result == nil' check in the result function yields false. This resulted in the panic when 'result' was later casted and accessed. So we basically need to undo #1447. For AwaitGatewayWithStatus and AwaitGatewayFullyConnected, check the error from 'findGateway' and return a nil interface{} value. Signed-off-by: Tom Pantelis --- test/e2e/framework/clusterglobalegressip.go | 5 ++-- test/e2e/framework/framework.go | 8 ++----- test/e2e/framework/gateways.go | 26 +++++++++++++-------- test/e2e/framework/globalingressips.go | 5 ++-- test/e2e/framework/pods.go | 4 ++-- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/test/e2e/framework/clusterglobalegressip.go b/test/e2e/framework/clusterglobalegressip.go index d0a67aa13..66f231c7e 100644 --- a/test/e2e/framework/clusterglobalegressip.go +++ b/test/e2e/framework/clusterglobalegressip.go @@ -52,12 +52,11 @@ func AwaitAllocatedEgressIPs(client dynamic.ResourceInterface, name string) []st return resGip, err }, func(result interface{}) (bool, string, error) { - obj := result.(*unstructured.Unstructured) - if obj == nil { + if result == nil { return false, fmt.Sprintf("Egress IP resource %q not found yet", name), nil } - globalIPs := getGlobalIPs(obj) + globalIPs := getGlobalIPs(result.(*unstructured.Unstructured)) if len(globalIPs) == 0 { return false, fmt.Sprintf("Egress IP resource %q exists but allocatedIPs not available yet", name), nil } diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index 24cd2130a..ad10bc9bf 100644 --- a/test/e2e/framework/framework.go +++ b/test/e2e/framework/framework.go @@ -281,11 +281,7 @@ func DetectGlobalnet() { }).Namespace(TestContext.SubmarinerNamespace) AwaitUntil("find Clusters to detect if Globalnet is enabled", func() (interface{}, error) { - clusters, err := clusters.List(context.TODO(), metav1.ListOptions{}) - if apierrors.IsNotFound(err) { - return nil, nil //nolint:nilnil // We want to repeat but let the checker known that nothing was found. - } - return clusters, err + return clusters.List(context.TODO(), metav1.ListOptions{}) }, func(result interface{}) (bool, string, error) { clusterList := result.(*unstructured.UnstructuredList) if clusterList == nil || len(clusterList.Items) == 0 { @@ -338,7 +334,7 @@ func fetchClusterIDs() { return ds, err }, func(result interface{}) (bool, string, error) { - if result.(*appsv1.DaemonSet) == nil { + if result == nil { return false, "No DaemonSet found", nil } diff --git a/test/e2e/framework/gateways.go b/test/e2e/framework/gateways.go index 37ac23d45..b5e504e7a 100644 --- a/test/e2e/framework/gateways.go +++ b/test/e2e/framework/gateways.go @@ -46,24 +46,25 @@ func findGateway(cluster ClusterIndex, name string) (*unstructured.Unstructured, resGw, err = gwClient.Get(context.TODO(), strings.Split(name, ".")[0], metav1.GetOptions{}) } - if apierrors.IsNotFound(err) { - return nil, nil //nolint:nilnil // We want to repeat but let the checker known that nothing was found. - } - return resGw, err } func (f *Framework) AwaitGatewayWithStatus(cluster ClusterIndex, name, status string) *unstructured.Unstructured { obj := AwaitUntil(fmt.Sprintf("await Gateway on %q with status %q", name, status), func() (interface{}, error) { - return findGateway(cluster, name) + gw, err := findGateway(cluster, name) + if apierrors.IsNotFound(err) { + return nil, nil //nolint:nilnil // We want to repeat but let the checker known that nothing was found. + } + + return gw, err }, func(result interface{}) (bool, string, error) { - gw := result.(*unstructured.Unstructured) - if gw == nil { + if result == nil { return false, "gateway not found yet", nil } + gw := result.(*unstructured.Unstructured) haStatus := NestedString(gw.Object, "status", "haStatus") if haStatus != status { return false, fmt.Sprintf("gateway %q exists but has wrong status %q, expected %q", gw.GetName(), haStatus, status), nil @@ -115,14 +116,19 @@ func (f *Framework) AwaitGatewayRemoved(cluster ClusterIndex, name string) { func (f *Framework) AwaitGatewayFullyConnected(cluster ClusterIndex, name string) *unstructured.Unstructured { obj := AwaitUntil(fmt.Sprintf("await Gateway on %q with status active and connections UP", name), func() (interface{}, error) { - return findGateway(cluster, name) + gw, err := findGateway(cluster, name) + if apierrors.IsNotFound(err) { + return nil, nil //nolint:nilnil // We want to repeat but let the checker known that nothing was found. + } + + return gw, err }, func(result interface{}) (bool, string, error) { - gw := result.(*unstructured.Unstructured) - if gw == nil { + if result == nil { return false, "gateway not found yet", nil } + gw := result.(*unstructured.Unstructured) haStatus := NestedString(gw.Object, "status", "haStatus") if haStatus != "active" { return false, fmt.Sprintf("Gateway %q exists but not active yet", diff --git a/test/e2e/framework/globalingressips.go b/test/e2e/framework/globalingressips.go index 0ea49078f..f47675d3e 100644 --- a/test/e2e/framework/globalingressips.go +++ b/test/e2e/framework/globalingressips.go @@ -47,12 +47,11 @@ func (f *Framework) AwaitGlobalIngressIP(cluster ClusterIndex, name, namespace s return resGip, err }, func(result interface{}) (bool, string, error) { - obj := result.(*unstructured.Unstructured) - if obj == nil { + if result == nil { return false, fmt.Sprintf("GlobalEgressIP %s not found yet", name), nil } - globalIP := getGlobalIP(obj) + globalIP := getGlobalIP(result.(*unstructured.Unstructured)) if globalIP == "" { return false, fmt.Sprintf("GlobalIngress %q exists but allocatedIP not available yet", name), nil diff --git a/test/e2e/framework/pods.go b/test/e2e/framework/pods.go index 63b1a8c7d..eff6f01be 100644 --- a/test/e2e/framework/pods.go +++ b/test/e2e/framework/pods.go @@ -116,11 +116,11 @@ func (f *Framework) AwaitUntilAnnotationOnPod(cluster ClusterIndex, annotation, } return pod, err }, func(result interface{}) (bool, string, error) { - pod := result.(*v1.Pod) - if pod == nil { + if result == nil { return false, "No Pod found", nil } + pod := result.(*v1.Pod) if pod.GetAnnotations()[annotation] == "" { return false, fmt.Sprintf("Pod %q does not have annotation %q yet", podName, annotation), nil }