Skip to content

Commit

Permalink
Fix nil interface{} checks
Browse files Browse the repository at this point in the history
PR #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 <[email protected]>
  • Loading branch information
tpantelis committed Nov 6, 2023
1 parent 61cd6c0 commit cc0d7db
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 24 deletions.
5 changes: 2 additions & 3 deletions test/e2e/framework/clusterglobalegressip.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
8 changes: 2 additions & 6 deletions test/e2e/framework/framework.go
Original file line number Diff line number Diff line change
Expand Up @@ -282,11 +282,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 {
Expand Down Expand Up @@ -339,7 +335,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
}

Expand Down
26 changes: 16 additions & 10 deletions test/e2e/framework/gateways.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down
5 changes: 2 additions & 3 deletions test/e2e/framework/globalingressips.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/framework/pods.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down

0 comments on commit cc0d7db

Please sign in to comment.