Analysis util functions for getting mesh config#19274
Analysis util functions for getting mesh config#19274istio-testing merged 11 commits intoistio:masterfrom
Conversation
|
Would it be more correct to assume that the namespace the MeshConfig configmap resides in is the control-plane namespace, rather than tying it to rootNamespace? |
There was a problem hiding this comment.
Why is this a utility function rather than a func on analysis.Context? It feels more explicit then that every analyzer should be running on exactly one control-plane/mesh configuration.
There was a problem hiding this comment.
I did fiddle with it briefly but didn't like how it was going, so I was thinking of settling for a smaller incremental improvement. But I agree, it does feel like it should probably end up there.
We can't assume that the mesh config is coming via configMap at all. e.g. in the Galley case, it's ingested as a yaml file. (Which, yes, is ultimately provided by a config map, but Galley doesn't know or care about that). |
0a21b88 to
5c4f0cd
Compare
|
/test unit-tests_istio |
|
/retest |
|
@ayj PTAL |
| // TODO: namespaces can in theory be anything, so we need to make this more configurable | ||
| if strings.HasPrefix(r.Metadata.Name.String(), "kube-") || strings.HasPrefix(r.Metadata.Name.String(), "istio-") { | ||
| ns := r.Metadata.Name.String() | ||
| if util.IsSystemNamespace(ns) { |
There was a problem hiding this comment.
The previous behavior skipped istio-system as well. I didn't see that handled in IsSystemNamespace.
There was a problem hiding this comment.
There's a slight behavior change here. Istio now installs with istio-system labeled istio-injection:disabled, so we aren't going to generate false positives in the normal case and excluding istio-system is redundant. And if someone manually changes that, then I don't feel bad alerting them about it.
The fewer namespace names we need to hardcode, the better.
| }) | ||
|
|
||
| if autoMtlsEnabled { | ||
| if mc.GetEnableAutoMtls().GetValue() { |
There was a problem hiding this comment.
Was this purely a refactor or does it change behavior? Some of the expected test output changed below.
There was a problem hiding this comment.
- A test was previously broken, and didn't fail even after it should have. Thus the
s/istio-policy/istio-telemetry. - There's a behavior change where for the mtls analyzer we look at the
istio=annotationto tell us what resources are control plane and should be skipped, instead of relying on the namespace. This has the dual benefit of avoiding needing to know theistio-systemnamespace, and also lets us detect errors for non-control-plane resources that are bundled in the namespace (like Grafana). - There's some minor test cleanup to add
{}to be consistent with what is valid when youkubectl applyit.
Signed-off-by: Naseem <naseemkullah@gmail.com>
Abstract getting mesh cfg into a util function.
Abstract getting system namespaces (including the istio control plane namespace) into a util function.Note that this assumes that meshcfg.rootNamespace is the control plane namespace, at least as far as analyzers are concerned. This should do the right thing for our current set of analyzers, and if we need to improve it in the future we now have a better abstraction boundary to do it behind.Update: Removed abstraction for system namespaces, in favor of refactoring to not have analyzers pay attention. It turns out that "control plane namespace" is not really well defined and best avoided if we can.
Kept other minor refactoring & cleanup.