Fix E2E panic due to invalid nil check #1447
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This code results in a panic if the underlying value of 'result'
is nil:
func(result interface{}) (bool, string, error) {
if result == nil {
return false, "gateway not found yet", nil
}
gw := result.(*unstructured.Unstructured)
haStatus := NestedString(gw.Object, "status", "haStatus") => PANIC
...
})
But it first checks if 'result' is nil so how can this be? A simple nil check
does not work because interfaces in Go contain both a type and a value. In
this case, we want to check for nil after casting.
Signed-off-by: Tom Pantelis <[email protected]>
tpantelis
requested review from
Oats87,
skitt and
sridhargaddam
as code owners
|
🤖 Created branch: z_pr1447/tpantelis/fix_nil_check |
tpantelis
changed the title
tpantelis
changed the title
skitt
approved these changes
Oct 31, 2023
Signed-off-by: Stephen Kitt <[email protected]>
sridhargaddam
approved these changes
Oct 31, 2023
|
🤖 Closed branches: [z_pr1447/tpantelis/fix_nil_check] |
tpantelis
added a commit
to tpantelis/shipyard
that referenced
this pull request
Nov 6, 2023
PR submariner-io#1447 changed places where we check for nil interface{} in the callback functions for AwaitUntil to casting 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, eg var s string var i interface{} = s 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 submariner-io#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 fucntion yields false. This resulted in the panic when 'result' was later casted and accessed. So we basically need to undo submariner-io#1447. For AwaitGatewayWithStatus and AwaitGatewayFullyConnected, check the error from 'findGateway' and return a nil interface{} value. Signed-off-by: Tom Pantelis <[email protected]>
tpantelis
added a commit
to tpantelis/shipyard
that referenced
this pull request
Nov 6, 2023
PR submariner-io#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 submariner-io#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 submariner-io#1447. For AwaitGatewayWithStatus and AwaitGatewayFullyConnected, check the error from 'findGateway' and return a nil interface{} value. Signed-off-by: Tom Pantelis <[email protected]>
tpantelis
added a commit
to tpantelis/shipyard
that referenced
this pull request
Nov 6, 2023
PR submariner-io#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 submariner-io#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 submariner-io#1447. For AwaitGatewayWithStatus and AwaitGatewayFullyConnected, check the error from 'findGateway' and return a nil interface{} value. Signed-off-by: Tom Pantelis <[email protected]>
tpantelis
added a commit
that referenced
this pull request
Nov 6, 2023
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]>
tpantelis
added a commit
to tpantelis/shipyard
that referenced
this pull request
Nov 6, 2023
PR submariner-io#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 submariner-io#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 submariner-io#1447. For AwaitGatewayWithStatus and AwaitGatewayFullyConnected, check the error from 'findGateway' and return a nil interface{} value. Signed-off-by: Tom Pantelis <[email protected]>
tpantelis
added a commit
that referenced
this pull request
Nov 6, 2023
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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This code results in a panic if the underlying value of
resultis nil:But it first checks if
resultis nil so how can this be? A simple nil check does not work because interfaces in Go contain both a type and a value. In this case, we want to check for nil after casting.