search

vmware / cloud-provider-for-cloud-director

Kubernetes External Cloud Provider for VMware Cloud Director
Other
20 stars 30 forks source link

use protocol instead of app protocol for health monitor type #376

Closed anvddriesch closed 2 months ago

anvddriesch commented 4 months ago

Solves the following bug: https://github.com/vmware/cloud-provider-for-cloud-director/issues/281

Previously, any TCP health monitors defined for a load balancer pool would be removed during an update of the pool which occurs for example during scaling of workers.

The reason is as follows: The TCP type health monitor is added based on the ports Protocol field being TCP when the load balancer pool is first formed as part of load balancer creation here: https://github.com/vmware/cloud-provider-for-cloud-director/blob/2d6e9efaf23df037d4cb1d58484bf04b3f2b8935/pkg/vcdsdk/gateway.go#L1754-L1755 However, in subsequent update cycles we were using the appProtocol field instead of the protocol one: https://github.com/vmware/cloud-provider-for-cloud-director/blob/2d6e9efaf23df037d4cb1d58484bf04b3f2b8935/pkg/ccm/loadbalancer.go#L167 This results in the health check being removed since only TCP type health checks are included in the first place as can be seen here: https://github.com/vmware/cloud-provider-for-cloud-director/blob/2d6e9efaf23df037d4cb1d58484bf04b3f2b8935/pkg/vcdsdk/gateway.go#L751-L752

This PR changes the code to use the ports Protocol also in update cases. The value being changed here is not used in any other functions and deploying this change resulted in health monitors to no longer be removed on update.

This change is Reviewable

vmwclabot commented 4 months ago

@anvddriesch, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

vmwclabot commented 4 months ago

@anvddriesch, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

vmwclabot commented 3 months ago

@anvddriesch, VMware has approved your signed contributor license agreement.

arunmk commented 2 months ago

Thanks @anvddriesch this is now merged.