-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OSSM-1420 Ensure istiod can run with no cluster-scoped privileges #513
Conversation
0fc87a3
to
99d3f32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. not 100% sure about the impact of leaving out the filters on the informers though. we should file a follow-up ticket
) | ||
}).AddEventHandler(handler) | ||
// TODO: filter deployments with label selector "gateway.istio.io/managed=istio.io-gateway-controller" | ||
client.KubeInformer().Apps().V1().Deployments().Informer().AddEventHandler(handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could consider using NewFilteredSecretInformer()
func of xns-informer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, the NewFilteredSecretInformer is a more low-level function. If we were to use it, we'd have to keep the list of namespaces in sync using the SMMR controller.
On the other hand, this filter here is only used for optimization.
}, | ||
) | ||
}) | ||
informer := client.KubeInformer().Core().V1().Secrets().Informer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, we could use NewFilteredSecretInformer(). let's create a follow-up ticket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, the filtering is only done as a best-effort optimization (according to the comment, the code behaves correctly without it). And since we're only watching the secrets in member namespaces, whereas the upstream code uses a cluster-scoped watch, this optimization may not do much at all.
namespaces sets.String | ||
} | ||
|
||
func NewMaistraDiscoveryNamespacesFilter(mrc memberroll.MemberRollController) DiscoveryNamespacesFilter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matches exactly what I have :) just called it FakeDiscoveryNamespacesFilter
}, | ||
) | ||
}).AddEventHandler(handler) | ||
// TODO: filter deployments with label selector "gateway.istio.io/managed=istio.io-gateway-controller" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth to create a Jira and link it here? We might thinks this is a TODO from upstream and never look back here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created OSSM-1448, but I have the feeling that this is something that we'll never actually implement. We'll either move away from xns-informers or upstream will remove the label selector.
integration test failure is likely a flake. seen this before |
Looks like pilot/pkg/serviceregistry/kube/controller/filter/discoverynamespaces.go needs formatting. |
/test integration |
…) (#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…istra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…istra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…istra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…istra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…istra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…istra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com> Signed-off-by: Yann Liu <yannliu@redhat.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com> Co-authored-by: Jacek Ewertowski <jewertow@redhat.com> Signed-off-by: Yann Liu <yannliu@redhat.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com> Co-authored-by: Jacek Ewertowski <jewertow@redhat.com> Signed-off-by: Yann Liu <yannliu@redhat.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com> Co-authored-by: Jacek Ewertowski <jewertow@redhat.com> Signed-off-by: Yann Liu <yannliu@redhat.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com> Co-authored-by: Jacek Ewertowski <jewertow@redhat.com> Signed-off-by: Yann Liu <yannliu@redhat.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com> Co-authored-by: Jacek Ewertowski <jewertow@redhat.com> Signed-off-by: Yann Liu <yannliu@redhat.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com> Co-authored-by: Jacek Ewertowski <jewertow@redhat.com> Signed-off-by: Yann Liu <yannliu@redhat.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com> Co-authored-by: Jacek Ewertowski <jewertow@redhat.com> Signed-off-by: Yann Liu <yannliu@redhat.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com> Co-authored-by: Jacek Ewertowski <jewertow@redhat.com> Signed-off-by: Yann Liu <yannliu@redhat.com>
…oped privileges (maistra#513) (maistra#548) Co-authored-by: Marko Lukša <marko.luksa@gmail.com> Co-authored-by: Jacek Ewertowski <jewertow@redhat.com> Signed-off-by: Yann Liu <yannliu@redhat.com>
No description provided.