kubernetes / cloud-provider-alibaba-cloud

CloudProvider for Alibaba Cloud
Apache License 2.0
360 stars 163 forks source link

ccm will update listener which is user manage but won't delete listener which is user manage #372

Closed OnlyPiglet closed 8 months ago

OnlyPiglet commented 1 year ago

What happened:

if I add a tcp protocol 80 listener on slb , and then I add a load balancer service with protocol tcp and port 80, this will update original , I think it's better not to update listener which is user manage is better. right?

What you expected to happen:

not to update listener which is user manage is better

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:

Environment:

OnlyPiglet commented 1 year ago

here is the related code,we may need to judge is this listener is user managed like delete action

pkg/controller/service/clbv1/listeners.go

for _, rlis := range remote.Listeners {
        found := false
        for _, llis := range local.Listeners {

            if rlis.ListenerPort == llis.ListenerPort && rlis.Protocol == llis.Protocol {
                found = true
                // listener and protocol match, do update here we may need to judge is this listener is user managed like delete action ?? 
                updateActions = append(updateActions,
                    UpdateAction{
                        lbId:   remote.LoadBalancerAttribute.LoadBalancerId,
                        local:  llis,
                        remote: rlis,
                    })
            }
        }
        // Do not delete any listener that no longer managed by my service
        // for safety. Only conflict case is taking care.
        if !found {
            if !isPortManagedByMyService(local, rlis) {
                reqCtx.Log.Info(fmt.Sprintf("update listener: port [%d] namekey [%s] is managed by user, skip reconcile", rlis.ListenerPort, rlis.NamedKey))
                continue
            }

            deleteActions = append(deleteActions, DeleteAction{
                lbId:     remote.LoadBalancerAttribute.LoadBalancerId,
                listener: rlis,
            })
        }
    }
gujingit commented 1 year ago

If the slb is created by a service with loadbalancer type, the SLB configuration will be synchronized based on the service configuration. And if you reuse a slb and set force-override to true, the slb also will be updated based on the service. If you reuse a slb and set force-override to false, ccm will not manage the listener.

OnlyPiglet commented 1 year ago

If the slb is created by a service with loadbalancer type, the SLB configuration will be synchronized based on the service configuration. And if you reuse a slb and set force-override to true, the slb also will be updated based on the service. If you reuse a slb and set force-override to false, ccm will not manage the listener.

I mean is if the listener is managed manually before, ccm should not update it even force-override is true,because the listener is managed by manual before . and service port may be misconfiguration.

OnlyPiglet commented 1 year ago

If the slb is created by a service with loadbalancer type, the SLB configuration will be synchronized based on the service configuration. And if you reuse a slb and set force-override to true, the slb also will be updated based on the service. If you reuse a slb and set force-override to false, ccm will not manage the listener.

I mean is if the listener is managed manually before, ccm should not update it even force-override is true,because the listener is managed by manual before . and service port may be misconfiguration.

because is I set force-override false, normal listener will not sync too

OnlyPiglet commented 1 year ago

I thinks this is better

for _, rlis := range remote.Listeners {
        found := false
        for _, llis := range local.Listeners {

            if rlis.ListenerPort == llis.ListenerPort && rlis.Protocol == llis.Protocol {
                found = true
                // listener and protocol match, don't update any listener that no longer managed by my service
                        if !isPortManagedByMyService(local, rlis) {
                reqCtx.Log.Info(fmt.Sprintf("update listener: port [%d] namekey [%s] is managed by user, skip reconcile", rlis.ListenerPort, rlis.NamedKey))
                continue
             }
                updateActions = append(updateActions,
                    UpdateAction{
                        lbId:   remote.LoadBalancerAttribute.LoadBalancerId,
                        local:  llis,
                        remote: rlis,
                    })
            }
        }
        // Do not delete any listener that no longer managed by my service
        // for safety. Only conflict case is taking care.
        if !found {
            if !isPortManagedByMyService(local, rlis) {
                reqCtx.Log.Info(fmt.Sprintf("delete listener: port [%d] namekey [%s] is managed by user, skip reconcile", rlis.ListenerPort, rlis.NamedKey))
                continue
            }

            deleteActions = append(deleteActions, DeleteAction{
                lbId:     remote.LoadBalancerAttribute.LoadBalancerId,
                listener: rlis,
            })
        }
    }
OnlyPiglet commented 1 year ago

@gujingit if there is any update,I will happy to commit a pr for this

k8s-triage-robot commented 8 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale