-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
daemon: updates the CEP in-place #32574
base: main
Are you sure you want to change the base?
daemon: updates the CEP in-place #32574
Conversation
Adds the base logic for handling the dynamic label filter for limiting the set of identity relevant labels used for creating the CIDs. Many labels are not useful for policy enforcement or visibility, the objective is to create CIDs only for labels used in network policies to greatly reduce the number of CIDs. Signed-off-by: Ovidiu Tirla <otirla@google.com>
Add a daemon feature flag for the dynamic label filter which is disabled by default for now. The goal is to have it enabled by default and completely replace the need to manually set the identity relevant labels. Signed-off-by: Ovidiu Tirla <otirla@google.com>
Signed-off-by: Ovidiu Tirla <otirla@google.com>
Modifies the CEP to reflect the dynamic filter labels changes Signed-off-by: Ovidiu Tirla <otirla@google.com>
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.
Hi, thanks a bunch for the contribution! Thanks for taking the time and using hive/cell
.
I've asked some code level questions, but the biggest question on my mind is whether this can turn out to be a performance bottleneck. ModifyIdentityLabels
I'd assume is a pretty expensive operation - if things change, I think this regenerates all affected endpoints, correct? Do you have some insight in how this performs in a cluster?
// If the SOURCE is any the transformed value will be empty | ||
// k8s.Foo converts to k8s:Foo, any.Foo converts to :Foo | ||
func normalizeKeyLabel(labelWithSourcePrefix string) string { | ||
before, after, found := strings.Cut(labelWithSourcePrefix, ".") |
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 was under the impression that label keys can contain dots. If there's no intentional source but the label key contains a dot, does this still do the right thing?
return exitingLabels | ||
} | ||
|
||
// getRelevantKeyLabels extracts the label key from the MatchLabels and MatchExpressions. |
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.
nit: I'm a bit confused by this naming - what are key labels? To me, this seems to extract a set of label keys from both the matchLabels
and the matchExpressionLabels
. So should this be named extractLabelKeys
?
exitingLabels[l.Key] = l | ||
} | ||
// Merge with pod labels | ||
exitingLabels.MergeLabels(labels.Map2Labels(podLabels, labels.LabelSourceK8s)) |
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 have slight performance worries here - we're doing a lot of string/map allocs and deallocs shuffling these different label representations around. How often do you expect this code to run?
A more efficient representation could be to use sorted lists of immutable labels, to avoid the constant allocation and deallocation.
identityLabelsMap := identityLabels.StringMap() | ||
labelsToRemove := labels.Labels{} | ||
|
||
for _, label := range endpointLabels { |
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.
Will this break in interesting ways if endpoint labels have duplicates?
) | ||
|
||
type controller struct { | ||
|
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.
nit: spurious newline
|
||
// EnableDynamicLabelFilter enables to filter dynamically the label prefixes used to determine identities based | ||
// on network policy labels. | ||
EnableDynamicLabelFilter bool |
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.
Let's try to avoid adding new fields to the DaemonConfig
. You're creating a new cell, you can use cell.Config
to scope the configuration to your cell.
case <-ctx.Done(): | ||
l.Info("Cilium Dynamic Label Filter Controller shut down") | ||
return | ||
case <-c.Signal.Signal: |
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.
Should this be rate-limited in some way? There's the signal coalescing if triggers arrive faster than the reconciliation can occur, but in effect (assuming fast trigger rate) the bottleneck is the rate at which reconciliation can happen - it's not clear to me that that's the right choice, given (I believe) this can kick of identity/ep regenerations.
based on #32501
Adds the logic required to update the CEP in-place when dynamic label filter detect any changes to network policy.
The reconcile loop will fetch all the CEP and compute the labels that needs to be added and removed considering the pod labels, namespace labels and the existing CEP labels.
Fixes: #32576