Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #4360 (2023-06-26 16:01:52)

:bug: compare string value of protocol in health check instead of pointer
<!-- Thanks for this PR! If this is your first PR please read the [contributing guide](../CONTRIBUTING.md) -->
<!-- If this PR is still work-in-progress and is being open for visibility please prefix the title with `WIP:` -->

**What type of PR is this?**
/kind bug 

<!--
Add one of the following kinds:
/kind feature
/kind bug
/kind api-change
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind flake
/kind regression
/kind support
-->

**What this PR does / why we need it**:

when a user specified healthCheck to be HTTPS or HTTP they would see an error like this in the logs 

```
failed to configure health check for classic load balancer: &{<redacted object>}: ValidationError: HealthCheck HTTPS Target must specify a port followed by a path that begins with a slash. e.g. HTTP:80/ping/this/path 
```

This was because the comparison on line 1441 of this file was comparing the addresses of the ELBHealthCheck, rather than the object. That comparison always failed and so the value from line 1445 was always returned. 

<!-- Enter a description of the change and why this change is needed -->

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Checklist**:
<!-- Put an "X" character inside the brackets of each completed task. Some may be optional depending on the PR in which case these can be deleted -->

- [x] squashed commits
- [ ] includes documentation
- [x] adds unit tests
- [ ] adds or updates e2e tests

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
fixes an error for HTTPS and HTTP health checks for classic ELBs 
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please assign [cecilerobertmichon](https://github.com/cecilerobertmichon) for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["cecilerobertmichon"]} -->
faiq: /test ?
faiq: /test pull-cluster-api-provider-aws-e2e
faiq: /test pull-cluster-api-provider-aws-e2e
enxebre: just one more comment https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/4360#discussion_r1246918395
then if yo can squash commits lgtm!
richardcase: @faiq - this also lgtm, can you squash the commits and then we can get this in.

Patch:
diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go
index 6785bc997..1dce896fa 100644
--- a/pkg/cloud/services/elb/loadbalancer.go
+++ b/pkg/cloud/services/elb/loadbalancer.go
@@ -1438,7 +1438,7 @@ func (s *Service) getHealthCheckTarget() string {
 	protocol := &infrav1.ELBProtocolSSL
 	if controlPlaneELB != nil && controlPlaneELB.HealthCheckProtocol != nil {
 		protocol = controlPlaneELB.HealthCheckProtocol
-		if protocol == &infrav1.ELBProtocolHTTP || protocol == &infrav1.ELBProtocolHTTPS {
+		if protocol.String() == infrav1.ELBProtocolHTTP.String() || protocol.String() == infrav1.ELBProtocolHTTPS.String() {
 			return fmt.Sprintf("%v:%d/readyz", protocol, infrav1.DefaultAPIServerPort)
 		}
 	}


Test patch:
diff --git a/pkg/cloud/services/elb/loadbalancer_test.go b/pkg/cloud/services/elb/loadbalancer_test.go
index 9fcbe8991..2cd031259 100644
--- a/pkg/cloud/services/elb/loadbalancer_test.go
+++ b/pkg/cloud/services/elb/loadbalancer_test.go
@@ -2317,6 +2317,10 @@ func TestChunkELBs(t *testing.T) {
 }
 
 func TestGetHealthCheckProtocol(t *testing.T) {
+	testHTTP := infrav1.ELBProtocol("HTTP")
+	testHTTPS := infrav1.ELBProtocol("HTTPS")
+	testTCP := infrav1.ELBProtocol("TCP")
+
 	tests := []struct {
 		testName                  string
 		lbSpec                    *infrav1.AWSLoadBalancerSpec
@@ -2330,21 +2334,21 @@ func TestGetHealthCheckProtocol(t *testing.T) {
 		{
 			"protocol http",
 			&infrav1.AWSLoadBalancerSpec{
-				HealthCheckProtocol: &infrav1.ELBProtocolHTTP,
+				HealthCheckProtocol: &testHTTP,
 			},
 			"HTTP:6443/readyz",
 		},
 		{
 			"protocol https",
 			&infrav1.AWSLoadBalancerSpec{
-				HealthCheckProtocol: &infrav1.ELBProtocolHTTPS,
+				HealthCheckProtocol: &testHTTPS,
 			},
 			"HTTPS:6443/readyz",
 		},
 		{
 			"protocol tcp",
 			&infrav1.AWSLoadBalancerSpec{
-				HealthCheckProtocol: &infrav1.ELBProtocolTCP,
+				HealthCheckProtocol: &testTCP,
 			},
 			"TCP:6443",
 		},