Remove potential nil ptr dereferences

[gargipanatula]

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Fixes a nil pointer dereference introduced by the SDK Go V2 migration in #1157.

The SDK migration's nil pointer dereference issue is in pkg/providers/v1/aws_loadbalancer.go:ensureSSLNegotiationPolicy, and is due to the SDK Go V2 returning nil on error rather than an empty struct. So, when the code didn't return an error and continued into the code, the nil pointer dereference surfaced.
V1 code: https://github.com/aws/aws-sdk-go/blob/main/service/elb/api.go#L1530-L1564
V2 code: https://github.com/aws/aws-sdk-go-v2/blob/service/elasticloadbalancing/v1.31.0/service/elasticloadbalancing/api_op_DescribeLoadBalancerPolicies.go#L29

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

This issue is currently awaiting triage.

If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-ci-robot]

Hi @gargipanatula. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[kmala]

we shouldn't return here as PolicyNotFoundException would mean we have to create the policy

[gargipanatula]

Ah thank you - pushed up a fix so that the code changes from

       result, err := c.elb.DescribeLoadBalancerPolicies(...)
	if err != nil {
		var notFoundErr *elbtypes.PolicyNotFoundException
		if !errors.As(err, &notFoundErr) {
			return fmt.Errorf("error describing security policies on load balancer: %q", err)
		}
	}

	if len(result.PolicyDescriptions) > 0 {
		return nil
	}

to

         result, err := c.elb.DescribeLoadBalancerPolicies(...)
	policyNotFoundError := false
	if err != nil {
		var notFoundErr *elbtypes.PolicyNotFoundException
		if !errors.As(err, &notFoundErr) {
			return fmt.Errorf("error describing security policies on load balancer: %q", err)
		}
		policyNotFoundError = true
	}

	if (!policyNotFoundError) && (result != nil && len(result.PolicyDescriptions) > 0) {
		// if policynotfound, would return false
		return nil
	}

This maintains the behavior that was present in V1, where the following happened:

• If DescribeLoadBalancerPolicies returns PolicyNotFoundException, the check will return false (result will be an empty struct so result.PolicyDescriptions will be 0)
• If DescribeLoadBalancerPolicies returns any other error, the check will return false (result will be an empty struct so result.PolicyDescriptions will be 0)
• If DescribeLoadBalancerPolicies does not return an error, the check will be evaluated as normal.

In V2, with these changes, it would look like this:

• If DescribeLoadBalancerPolicies returns PolicyNotFoundException, the check will return false (policyNotFoundError check)
• If DescribeLoadBalancerPolicies returns any other error, the check will return false (result will be nil)
• If DescribeLoadBalancerPolicies does not return an error, the check will be evaluated as normal (not a policyNotFoundError and result != nil).

[kmala]

can this happen where err is nil and instance is also nil?

[gargipanatula]

Yeah, getInstanceByID does

return instances[instanceID], nil

and instances[instanceID] can be nil if instanceID doesn't exist in the instances map

[JoelSpeed]

When is it ok for the instance to be nil, and for there to be no error?

[gargipanatula]

I don't think it's ever ok for instance to be nil.

This is because if instance is nil and not caught, it will immediately get passed into getInstanceZone (line 1010) where it will produce a nil ptr dereference issue because getInstanceZone dereferences instance without checking if it's nil.

getInstanceZone code:

func (c *Cloud) getInstanceZone(instance *ec2.Instance) cloudprovider.Zone {
	return cloudprovider.Zone{
		FailureDomain: *(instance.Placement.AvailabilityZone),
		Region:        c.region,
	}
}

[gargipanatula]

I think I misunderstood your question, if we're wondering when we get an output like nil, nil, getInstanceByID can produce this output, hence the need for if err != nil || instance == nil {.

[gargipanatula]

For context, the only case where this would've been true is if we had hit the
return instances[instanceID], nil
case in getInstanceByID, and instances[instanceID] was nil.

With these changes, if instances[instanceID] is nil, we now throw an InstanceNotFound error. So, instead of checking if err == nil && instance == nil, we can just check if err != nil and throw the resulting error from the function.

[kmala]

is there a need for policyNotFoundError? we can just have (result != nil && len(result.PolicyDescriptions) > 0) right if the result != nil anyway means we don't have any error?

[kmala]

/ok-to-test

[kmala]

/lgtm

[kmala]

/approve

[kmala]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kmala

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [kmala]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment