Skip to content
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

Merged
merged 1 commit into from
May 3, 2022

Conversation

luksa
Copy link
Contributor

@luksa luksa commented Apr 29, 2022

No description provided.

@luksa luksa force-pushed the OSSM-1420 branch 3 times, most recently from 0fc87a3 to 99d3f32 Compare April 29, 2022 13:02
Copy link
Contributor

@dgn dgn left a 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)
Copy link
Contributor

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

Copy link
Contributor Author

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()
Copy link
Contributor

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

Copy link
Contributor Author

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 {
Copy link
Contributor

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"
Copy link
Member

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

Copy link
Contributor Author

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.

@dgn
Copy link
Contributor

dgn commented Apr 29, 2022

integration test failure is likely a flake. seen this before

@rcernich
Copy link
Contributor

rcernich commented May 2, 2022

Looks like pilot/pkg/serviceregistry/kube/controller/filter/discoverynamespaces.go needs formatting.

@luksa
Copy link
Contributor Author

luksa commented May 3, 2022

/test integration

@dgn
Copy link
Contributor

dgn commented May 3, 2022

@luksa is this ready to merge? I would like to update #509 once it is in

@maistra-bot maistra-bot merged commit 106d88d into maistra:maistra-2.2 May 3, 2022
maistra-bot pushed a commit that referenced this pull request Jun 24, 2022
…) (#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Aug 23, 2022
jewertow added a commit to jewertow/istio that referenced this pull request Aug 24, 2022
jewertow added a commit to jewertow/istio that referenced this pull request Aug 25, 2022
jewertow added a commit to jewertow/istio that referenced this pull request Aug 25, 2022
jewertow added a commit to jewertow/istio that referenced this pull request Aug 25, 2022
jewertow added a commit to jewertow/istio that referenced this pull request Aug 29, 2022
jewertow added a commit to jewertow/istio that referenced this pull request Aug 29, 2022
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
maistra-bot pushed a commit that referenced this pull request Aug 30, 2022
…oped privileges (#513) (#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
maistra-bot pushed a commit that referenced this pull request Nov 9, 2022
…oped privileges (#513) (#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Jul 11, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Jul 11, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Jul 11, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Jul 17, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Jul 17, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Aug 8, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Aug 9, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Aug 9, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Aug 9, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Aug 9, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Aug 9, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Aug 10, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
jewertow added a commit to jewertow/istio that referenced this pull request Aug 10, 2023
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
openshift-merge-robot pushed a commit that referenced this pull request Aug 10, 2023
…oped privileges (#513) (#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
yannuil added a commit to yannuil/maistra-istio that referenced this pull request Feb 19, 2024
…oped privileges (maistra#513) (maistra#548)

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
Signed-off-by: Yann Liu <yannliu@redhat.com>
yannuil added a commit to yannuil/maistra-istio that referenced this pull request Mar 21, 2024
…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>
yannuil added a commit to yannuil/maistra-istio that referenced this pull request Mar 25, 2024
…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>
yannuil added a commit to yannuil/maistra-istio that referenced this pull request Mar 25, 2024
…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>
yannuil added a commit to yannuil/maistra-istio that referenced this pull request Mar 25, 2024
…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>
yannuil added a commit to yannuil/maistra-istio that referenced this pull request Mar 25, 2024
…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>
yannuil added a commit to yannuil/maistra-istio that referenced this pull request Mar 25, 2024
…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>
yannuil added a commit to yannuil/maistra-istio that referenced this pull request Mar 25, 2024
…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>
yannuil added a commit to yannuil/maistra-istio that referenced this pull request Mar 26, 2024
…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>
yannuil added a commit to yannuil/maistra-istio that referenced this pull request Mar 26, 2024
…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>
openshift-merge-bot bot pushed a commit that referenced this pull request Mar 26, 2024
…oped privileges (#513) (#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants