fix update logic for BackendSets and Listeners

- For Listener sync, fix panic scenario when updating non-TLS configured Ingress to TLS-configured Ingress
- Split BackendSet update wrapper method into two - one for bulk updating backends and one for updating other details. This is done to enable updating to nil value for sslConfig.

Author: piyush-tiwari

pkg/controllers/ingress/ingress.go

@@ -522,7 +522,7 @@ func syncListener(ctx context.Context, namespace string, stateStore *state.State
 		}
 
 		if sslConfig != nil {
-			if !reflect.DeepEqual(listener.SslConfiguration.CertificateIds, sslConfig.CertificateIds) {
+			if listener.SslConfiguration == nil || !reflect.DeepEqual(listener.SslConfiguration.CertificateIds, sslConfig.CertificateIds) {
 				klog.Infof("SSL config for listener update is %s", util.PrettyPrint(sslConfig))
 				needsUpdate = true
 			}
@@ -579,11 +579,10 @@ func syncBackendSet(ctx context.Context, ingress *networkingv1.Ingress, lbID str
 	if err != nil {
 		return err
 	}
-	if sslConfig != nil {
-		if bs.SslConfiguration == nil || !reflect.DeepEqual(bs.SslConfiguration.TrustedCertificateAuthorityIds, sslConfig.TrustedCertificateAuthorityIds) {
-			klog.Infof("SSL config for backend set %s update is %s", *bs.Name, util.PrettyPrint(sslConfig))
-			needsUpdate = true
-		}
+
+	if backendSetSslConfigNeedsUpdate(sslConfig, &bs) {
+		klog.Infof("SSL config for backend set %s update is %s", *bs.Name, util.PrettyPrint(sslConfig))
+		needsUpdate = true
 	}
 
 	healthChecker := stateStore.GetBackendSetHealthChecker(*bs.Name)
@@ -601,7 +600,7 @@ func syncBackendSet(ctx context.Context, ingress *networkingv1.Ingress, lbID str
 	}
 
 	if needsUpdate {
-		err = wrapperClient.GetLbClient().UpdateBackendSet(context.TODO(), lb.Id, etag, bs, nil, sslConfig, healthChecker, &policy)
+		err = wrapperClient.GetLbClient().UpdateBackendSetDetails(context.TODO(), *lb.Id, etag, &bs, sslConfig, healthChecker, policy)
 		if err != nil {
 			return err
 		}

pkg/controllers/ingress/util.go

@@ -429,3 +429,17 @@ func CreateOrGetCaBundleForBackendSet(namespace string, secretName string, compa
 func isTrustAuthorityCaBundle(id string) bool {
 	return strings.Contains(id, "cabundle")
 }
+
+func backendSetSslConfigNeedsUpdate(calculatedConfig *ociloadbalancer.SslConfigurationDetails,
+	currentBackendSet *ociloadbalancer.BackendSet) bool {
+	if calculatedConfig == nil && currentBackendSet.SslConfiguration != nil {
+		return true
+	}
+
+	if calculatedConfig != nil && (currentBackendSet.SslConfiguration == nil ||
+		!reflect.DeepEqual(currentBackendSet.SslConfiguration.TrustedCertificateAuthorityIds, calculatedConfig.TrustedCertificateAuthorityIds)) {
+		return true
+	}
+
+	return false
+}

pkg/controllers/ingress/util_test.go

@@ -367,6 +367,34 @@ intermediatecert
 	Expect(err).To(HaveOccurred())
 }
 
+func TestBackendSetSslConfigNeedsUpdate(t *testing.T) {
+	RegisterTestingT(t)
+
+	caBundleId1 := []string{"caCert1"}
+	caBundleId2 := []string{"caCert2"}
+
+	backendSetWithNilSslConfig := &ociloadbalancer.BackendSet{}
+	presentBackendSet1 := &ociloadbalancer.BackendSet{
+		SslConfiguration: &ociloadbalancer.SslConfiguration{
+			TrustedCertificateAuthorityIds: caBundleId1,
+		},
+	}
+	presentBackendSet2 := &ociloadbalancer.BackendSet{
+		SslConfiguration: &ociloadbalancer.SslConfiguration{
+			TrustedCertificateAuthorityIds: caBundleId2,
+		},
+	}
+	calculatedConfig1 := &ociloadbalancer.SslConfigurationDetails{
+		TrustedCertificateAuthorityIds: caBundleId1,
+	}
+
+	Expect(backendSetSslConfigNeedsUpdate(nil, backendSetWithNilSslConfig)).To(BeFalse())
+	Expect(backendSetSslConfigNeedsUpdate(nil, presentBackendSet1)).To(BeTrue())
+	Expect(backendSetSslConfigNeedsUpdate(calculatedConfig1, presentBackendSet1)).To(BeFalse())
+	Expect(backendSetSslConfigNeedsUpdate(calculatedConfig1, presentBackendSet2)).To(BeTrue())
+	Expect(backendSetSslConfigNeedsUpdate(calculatedConfig1, backendSetWithNilSslConfig)).To(BeTrue())
+}
+
 func generateTestCertsAndKey() (string, string, string) {
 	caCert := &x509.Certificate{
 		SerialNumber: big.NewInt(1),

pkg/loadbalancer/loadbalancer.go

@@ -451,6 +451,7 @@ func (lbc *LoadBalancerClient) updateRoutingPolicyRules(ctx context.Context, lbI
 	return err
 }
 
+// UpdateBackends updates Backends for backendSetName, while preserving sslConfig, policy, and healthChecker details
 func (lbc *LoadBalancerClient) UpdateBackends(ctx context.Context, lbID string, backendSetName string, backends []loadbalancer.BackendDetails) error {
 	lb, etag, err := lbc.GetLoadBalancer(ctx, lbID)
 	if err != nil {
@@ -477,56 +478,60 @@ func (lbc *LoadBalancerClient) UpdateBackends(ctx context.Context, lbID string,
 		return nil
 	}
 
-	return lbc.UpdateBackendSet(ctx, lb.Id, etag, backendSet, backends, nil, nil, nil)
-}
-
-func (lbc *LoadBalancerClient) UpdateBackendSet(ctx context.Context, lbID *string, etag string, backendSet loadbalancer.BackendSet,
-	backends []loadbalancer.BackendDetails, sslConfig *loadbalancer.SslConfigurationDetails,
-	healthCheckerDetails *loadbalancer.HealthCheckerDetails, policy *string) error {
-
-	if backends == nil {
-		backends = make([]loadbalancer.BackendDetails, len(backendSet.Backends))
-		for i := range backendSet.Backends {
-			backend := backendSet.Backends[i]
-			backends[i] = loadbalancer.BackendDetails{
-				IpAddress: backend.IpAddress,
-				Port:      backend.Port,
-				Weight:    backend.Weight,
-				Drain:     backend.Drain,
-				Backup:    backend.Backup,
-				Offline:   backend.Offline,
-			}
+	var sslConfig *loadbalancer.SslConfigurationDetails
+	if backendSet.SslConfiguration != nil {
+		sslConfig = &loadbalancer.SslConfigurationDetails{
+			TrustedCertificateAuthorityIds: backendSet.SslConfiguration.TrustedCertificateAuthorityIds,
 		}
 	}
-	if sslConfig == nil && backendSet.SslConfiguration != nil {
-		sslConfig = &loadbalancer.SslConfigurationDetails{TrustedCertificateAuthorityIds: backendSet.SslConfiguration.TrustedCertificateAuthorityIds}
-	}
-
-	if healthCheckerDetails == nil {
-		healthChecker := backendSet.HealthChecker
-		healthCheckerDetails = &loadbalancer.HealthCheckerDetails{
-			Protocol:          healthChecker.Protocol,
-			UrlPath:           healthChecker.UrlPath,
-			Port:              healthChecker.Port,
-			ReturnCode:        healthChecker.ReturnCode,
-			Retries:           healthChecker.Retries,
-			TimeoutInMillis:   healthChecker.TimeoutInMillis,
-			IntervalInMillis:  healthChecker.IntervalInMillis,
-			ResponseBodyRegex: healthChecker.ResponseBodyRegex,
-			IsForcePlainText:  healthChecker.IsForcePlainText,
-		}
+
+	healthCheckerDetails := &loadbalancer.HealthCheckerDetails{
+		Protocol:          backendSet.HealthChecker.Protocol,
+		UrlPath:           backendSet.HealthChecker.UrlPath,
+		Port:              backendSet.HealthChecker.Port,
+		ReturnCode:        backendSet.HealthChecker.ReturnCode,
+		Retries:           backendSet.HealthChecker.Retries,
+		TimeoutInMillis:   backendSet.HealthChecker.TimeoutInMillis,
+		IntervalInMillis:  backendSet.HealthChecker.IntervalInMillis,
+		ResponseBodyRegex: backendSet.HealthChecker.ResponseBodyRegex,
+		IsForcePlainText:  backendSet.HealthChecker.IsForcePlainText,
 	}
 
-	if policy == nil {
-		policy = backendSet.Policy
+	policy := *backendSet.Policy
+
+	return lbc.UpdateBackendSet(ctx, lbID, etag, *backendSet.Name, policy, healthCheckerDetails, sslConfig, backends)
+}
+
+// UpdateBackendSetDetails updates sslConfig, policy, and healthChecker details for backendSet, while preserving individual backends
+func (lbc *LoadBalancerClient) UpdateBackendSetDetails(ctx context.Context, lbID string, etag string,
+	backendSet *loadbalancer.BackendSet, sslConfig *loadbalancer.SslConfigurationDetails,
+	healthCheckerDetails *loadbalancer.HealthCheckerDetails, policy string) error {
+
+	backends := make([]loadbalancer.BackendDetails, len(backendSet.Backends))
+	for i := range backendSet.Backends {
+		backend := backendSet.Backends[i]
+		backends[i] = loadbalancer.BackendDetails{
+			IpAddress: backend.IpAddress,
+			Port:      backend.Port,
+			Weight:    backend.Weight,
+			Drain:     backend.Drain,
+			Backup:    backend.Backup,
+			Offline:   backend.Offline,
+		}
 	}
 
+	return lbc.UpdateBackendSet(ctx, lbID, etag, *backendSet.Name, policy, healthCheckerDetails, sslConfig, backends)
+}
+
+func (lbc *LoadBalancerClient) UpdateBackendSet(ctx context.Context, lbID string, etag string, backendSetName string,
+	policy string, healthCheckerDetails *loadbalancer.HealthCheckerDetails, sslConfig *loadbalancer.SslConfigurationDetails,
+	backends []loadbalancer.BackendDetails) error {
 	updateBackendSetRequest := loadbalancer.UpdateBackendSetRequest{
 		IfMatch:        common.String(etag),
-		LoadBalancerId: lbID,
-		BackendSetName: common.String(*backendSet.Name),
+		LoadBalancerId: common.String(lbID),
+		BackendSetName: common.String(backendSetName),
 		UpdateBackendSetDetails: loadbalancer.UpdateBackendSetDetails{
-			Policy:           policy,
+			Policy:           common.String(policy),
 			HealthChecker:    healthCheckerDetails,
 			SslConfiguration: sslConfig,
 			Backends:         backends,
@@ -539,7 +544,8 @@ func (lbc *LoadBalancerClient) UpdateBackendSet(ctx context.Context, lbID *strin
 		return err
 	}
 
-	klog.Infof("Update backend set response: name: %s, work request id: %s, opc request id: %s.", *backendSet.Name, *resp.OpcWorkRequestId, *resp.OpcRequestId)
+	klog.Infof("Update backend set response: name: %s, work request id: %s, opc request id: %s.",
+		*updateBackendSetRequest.BackendSetName, *resp.OpcWorkRequestId, *resp.OpcRequestId)
 	_, err = lbc.waitForWorkRequest(ctx, *resp.OpcWorkRequestId)
 	return err
 }
@@ -643,7 +649,7 @@ func (lbc *LoadBalancerClient) CreateListener(ctx context.Context, lbID string,
 		if sslConfig == nil {
 			return fmt.Errorf("no TLS configuration provided for a HTTP2 listener at port %d", listenerPort)
 		}
-		
+
 		sslConfig.CipherSuiteName = common.String(util.ProtocolHTTP2DefaultCipherSuite)
 	}
 

pkg/loadbalancer/loadbalancer_test.go

@@ -119,6 +119,26 @@ func TestLoadBalancerClient_UpdateBackends(t *testing.T) {
 
 }
 
+func TestLoadBalancerClient_UpdateBackendSetDetails(t *testing.T) {
+	RegisterTestingT(t)
+	loadBalancerClient := setupLBClient()
+
+	lbId := "id"
+	etag := "etag"
+	policy := "policy"
+	bsName := util.GenerateBackendSetName("default", "testecho1", 80)
+
+	lb := util.SampleLoadBalancerResponse()
+	sslConfigDetails := ociloadbalancer.SslConfigurationDetails{TrustedCertificateAuthorityIds: []string{"trusted-cert"}}
+	healthCheckerDetails := ociloadbalancer.HealthCheckerDetails{}
+
+	bs := lb.BackendSets[bsName]
+
+	err := loadBalancerClient.UpdateBackendSetDetails(context.TODO(), lbId, etag, &bs, &sslConfigDetails,
+		&healthCheckerDetails, policy)
+	Expect(err).To(BeNil())
+}
+
 func TestLoadBalancerClient_DeleteBackendSet(t *testing.T) {
 	RegisterTestingT(t)
 	loadBalancerClient := setupLBClient()