Fix: unused ingress resources cleanup for external listeners #954
Conversation
| for _, res := range externalListenerResources { | ||
| o := res(log, eListener, ingressConfig, name, defaultControllerName) |
There was a problem hiding this comment.
Naming here is once again a bit hard to understand with res and o.
There was a problem hiding this comment.
This is from the previous state of the repo.
There was a problem hiding this comment.
I know, it's more of a comment that I don't think it's the greatest naming scheme ever. :D
You don't need to change it (my approval doesn't depend on it at all), it just would be nicer I think.
| deletionCounter++ | ||
| } | ||
| } | ||
| if deletionCounter > 0 { | ||
| log.Info(fmt.Sprintf("Removed '%d' resources for envoy ingress", deletionCounter)) | ||
| } |
There was a problem hiding this comment.
Question: Wouldn't it be better to put a log message in the place of deleteCounter++ stating deleted envoy ingress resource %s where %s is the name of the object?
This way we will see removed 5 resources for envoy ingress, not sure if that is more useful than seeing removed resources for envoy ingress vs. the more informative listing of removed resources.
I can be convinced we don't need the resource level info, just curious.
Same for the Istio version.
There was a problem hiding this comment.
Then it would be 5 remove message in case envoy. I think it is a good idea to do this with lower log level like: log.V(1).Info(...) What do you think?
There was a problem hiding this comment.
Yeah, I think this one line would be great for info and maybe we could add a debug level message for the individual resources.
|
bartam1
force-pushed
the
fix_remove_unused_externel_res
branch
from
d90d972 to
2ce938d
Compare
What's in this PR?
Removing unused ingress resources (nodePort services, istio ingress, envoy ingress) for external listeners
Why?
Koperator uses envoy as the ingress controller by default. When the users want to expose their Kafka brokers via LoadBalancer, the Koperator deploys these necessary resources: . But it doesn’t seem to remove these resources when they are not needed, for example, the users switch from LoadBalancer to NodePort. Moreover, the brokers are still reachable via the stale loadbalancer service. Same applies when changing from NodePort to LoadBalancer and from istioingress ingresscontroller to envoy ingress controller and vice-versa.
Additional context
Checklist