Limit label selector to Ingress factory. #3137
Conversation
timoreimann
added
status/2-needs-review
area/provider/k8s/ingress
kind/bug/fix
|
@timoreimann could you rebase on 1.6. |
timoreimann
force-pushed
the
kubernetes-limit-labelselector-to-ingress-factory
branch
from
2a7fbb9 to
75856d0
Compare
|
@ldez done. |
provider/kubernetes/client.go
Outdated
| @@ -160,7 +171,7 @@ func (c *clientImpl) WatchAll(namespaces Namespaces, labelSelector string, stopC | |||
| // GetIngresses returns all Ingresses for observed namespaces in the cluster. | |||
| func (c *clientImpl) GetIngresses() []*extensionsv1beta1.Ingress { | |||
| var result []*extensionsv1beta1.Ingress | |||
| for ns, factory := range c.factories { | |||
| for ns, factory := range c.ingressFactories { | |||
| ings, err := factory.Extensions().V1beta1().Ingresses().Lister().List(labels.Everything()) | |||
There was a problem hiding this comment.
@timoreimann It looks odd to me to keep 2 factory instances at the same time in clientImpl. How do you think about setting tweakListOptions to nil for all resources including ingresses but filtering them by selector here?
There was a problem hiding this comment.
Ah yeah, that'd be much better (and pretty much what I was longing for but failed to see).
Let me update the PR...
There was a problem hiding this comment.
@timoreimann Umm I think we should also use cache.FilteringResourceEventHandler to specify a filter func to perform label selector matching for ingress resources. Or some extra ingress events will be captured by eventHandler which may trigger some unnecessary reloads. See examples here :)
https://github.com/kubernetes/kubernetes/blob/dfd6f581b5c8830c760d2778b299c3fd5e7aa242/pkg/scheduler/factory/factory.go#L186
There was a problem hiding this comment.
Updated the PR.
PTAL again.
|
I didn't really notice that this labelSelector should only be applied for ingress. How about renaming the parameter to |
|
Code looks good, but do you think we should have a test for this? since we merged a regression without it being caught? |
I agree with you that we should have a test (in fact, tests for most of our low-level Kubernetes integration functionality). However, I don't think that unit tests would be really valuable in this case: The code in What I think we need are proper integration tests for Kubernetes. I've been working on #1889 for quite a while; it's not quite done, but I hope to finish it rather soon. Personally, I'd rather invest into these when it comes to validating our integration logic. |
|
I manually tested the PR both with and without a configured label selector. Looks like it's working as expected. |
provider/kubernetes/kubernetes.go
Outdated
| func (p *Provider) newK8sClient(ingressLabelSelector string) (Client, error) { | ||
| ingLabelSel, err := labels.Parse(ingressLabelSelector) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid ingress label selector: %s", ingressLabelSelector) |
There was a problem hiding this comment.
WDYT about using %q instead of %s ?
provider/kubernetes/kubernetes.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("invalid ingress label selector: %s", ingressLabelSelector) | ||
| } | ||
| log.Infof("ingress label selector is: %s", ingLabelSel) |
There was a problem hiding this comment.
WDYT about using %q instead of %s ?
This fixes a regression when we switched to the newer shared informer factory.
This reverts commit 75856d006cf526203121b36f1c31cc40ad6cf4be.
ldez
force-pushed
the
kubernetes-limit-labelselector-to-ingress-factory
branch
from
1685ff9 to
2ef023c
Compare
What does this PR do?
Limit the Kubernetes label selector to the Ingress factory.
Motivation
Fix a regression when we switched to the newer shared informer factory.
Reviewer Notes
@yue9944882 could you please take a look and let me know if this seems good to you?
I'm also open to any suggestions how we could possibly implement this more elegantly.
Thanks!
Refs #3128