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

feat: Support filtering annotations allowlist by "*" #2234

Merged
merged 1 commit into from
Nov 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/cli-arguments.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ Flags:
--log_file_max_size uint Defines the maximum size a log file can grow to (no effect when -logtostderr=true). Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)
--logtostderr log to standard error instead of files (default true)
--metric-allowlist string Comma-separated list of metrics to be exposed. This list comprises of exact metric names and/or regex patterns. The allowlist and denylist are mutually exclusive.
--metric-annotations-allowlist string Comma-separated list of Kubernetes annotations keys that will be used in the resource' labels metric. By default the annotations metrics are not exposed. To include them, provide a list of resource names in their plural form and Kubernetes annotation keys you would like to allow for them (Example: '=namespaces=[kubernetes.io/team,...],pods=[kubernetes.io/team],...)'. A single '*' can be provided per resource instead to allow any annotations, but that has severe performance implications (Example: '=pods=[*]').
--metric-annotations-allowlist string Comma-separated list of Kubernetes annotations keys that will be used in the resource' labels metric. By default the annotations metrics are not exposed. To include them, provide a list of resource names in their plural form and Kubernetes annotation keys you would like to allow for them (Example: '=namespaces=[kubernetes.io/team,...],pods=[kubernetes.io/team],...)'. A single '*' can be provided per resource instead to allow any annotations, but that has severe performance implications (Example: '=pods=[*]'). Additionally, an asterisk (*) can be provided as a key, which will resolve to all resources, i.e., assuming '--resources=deployments,pods', '=*=[*]' will resolve to '=deployments=[*],pods=[*]'.
--metric-denylist string Comma-separated list of metrics not to be enabled. This list comprises of exact metric names and/or regex patterns. The allowlist and denylist are mutually exclusive.
--metric-labels-allowlist string Comma-separated list of additional Kubernetes label keys that will be used in the resource' labels metric. By default the labels metrics are not exposed. To include them, provide a list of resource names in their plural form and Kubernetes label keys you would like to allow for them (Example: '=namespaces=[k8s-label-1,k8s-label-n,...],pods=[app],...)'. A single '*' can be provided per resource instead to allow any labels, but that has severe performance implications (Example: '=pods=[*]'). Additionally, an asterisk (*) can be provided as a key, which will resolve to all resources, i.e., assuming '--resources=deployments,pods', '=*=[*]' will resolve to '=deployments=[*],pods=[*]'.
--metric-opt-in-list string Comma-separated list of metrics which are opt-in and not enabled by default. This is in addition to the metric allow- and denylists
Expand Down
53 changes: 32 additions & 21 deletions internal/store/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,32 +219,43 @@ func (b *Builder) WithCustomResourceStoreFactories(fs ...customresource.Registry
}
}

// WithAllowAnnotations configures which annotations can be returned for metrics
func (b *Builder) WithAllowAnnotations(annotations map[string][]string) {
if len(annotations) > 0 {
b.allowAnnotationsList = annotations
// allowList validates the given map and checks if the resources exists.
// If there is a '*' as key, return new map with all enabled resources.
func (b *Builder) allowList(list map[string][]string) (map[string][]string, error) {
if len(list) == 0 {
return nil, nil
}

for l := range list {
if !resourceExists(l) && l != "*" {
return nil, fmt.Errorf("resource %s does not exist. Available resources: %s", l, strings.Join(availableResources(), ","))
}
}

// "*" takes precedence over other specifications
allowedList, ok := list["*"]
if !ok {
return list, nil
}
m := make(map[string][]string)
for _, resource := range b.enabledResources {
m[resource] = allowedList
}
return m, nil
}

// WithAllowAnnotations configures which annotations can be returned for metrics
func (b *Builder) WithAllowAnnotations(annotations map[string][]string) error {
var err error
b.allowAnnotationsList, err = b.allowList(annotations)
return err
}

// WithAllowLabels configures which labels can be returned for metrics
func (b *Builder) WithAllowLabels(labels map[string][]string) error {
if len(labels) > 0 {
for label := range labels {
if !resourceExists(label) && label != "*" {
return fmt.Errorf("resource %s does not exist. Available resources: %s", label, strings.Join(availableResources(), ","))
}
}
b.allowLabelsList = labels
// "*" takes precedence over other specifications
if allowedLabels, ok := labels["*"]; ok {
m := make(map[string][]string)
for _, resource := range b.enabledResources {
m[resource] = allowedLabels
}
b.allowLabelsList = m
}
}
return nil
var err error
b.allowLabelsList, err = b.allowList(labels)
return err
}

// Build initializes and registers all enabled stores.
Expand Down
83 changes: 83 additions & 0 deletions internal/store/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,86 @@ func TestWithAllowLabels(t *testing.T) {
}
}
}

func TestWithAllowAnnotations(t *testing.T) {
tests := []struct {
Desc string
AnnotationsAllowlist map[string][]string
EnabledResources []string
Wanted LabelsAllowList
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be

Suggested change
Wanted LabelsAllowList
Wanted AnnotationsAllowList

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, It reuses the same type from the test above here. I could declare a new type and point it to options.LabelsAllowList, while at it should I change this to a more generic also?

I noticed that these two tests test the same function in the end, I wrote tests first then went to implement, remove the test or keep it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I didn't know AnnotatiosAllowList don't exist. I think we can keep it for this change like this. :)

err expectedError
}{
{
Desc: "wildcard key-value as the only element",
AnnotationsAllowlist: map[string][]string{"*": {"*"}},
EnabledResources: []string{"cronjobs", "pods", "deployments"},
Wanted: LabelsAllowList(map[string][]string{
"deployments": {"*"},
"pods": {"*"},
"cronjobs": {"*"},
}),
},
{
Desc: "wildcard key-value as not the only element",
AnnotationsAllowlist: map[string][]string{"*": {"*"}, "pods": {"*"}, "cronjobs": {"*"}},
EnabledResources: []string{"cronjobs", "pods", "deployments"},
Wanted: LabelsAllowList(map[string][]string{
"deployments": {"*"},
"pods": {"*"},
"cronjobs": {"*"},
}),
},
{
Desc: "wildcard key-value as not the only element, with resource mismatch",
AnnotationsAllowlist: map[string][]string{"*": {"*"}, "pods": {"*"}, "cronjobs": {"*"}, "configmaps": {"*"}},
EnabledResources: []string{"cronjobs", "pods", "deployments"},
Wanted: LabelsAllowList{},
err: expectedError{
expectedNotEqual: true,
},
},
{
Desc: "wildcard key-value as not the only element, with other mutually-exclusive keys",
AnnotationsAllowlist: map[string][]string{"*": {"*"}, "foo": {"*"}, "bar": {"*"}, "cronjobs": {"*"}},
EnabledResources: []string{"cronjobs", "pods", "deployments"},
Wanted: LabelsAllowList(nil),
err: expectedError{
expectedLabelError: true,
},
},
{
Desc: "wildcard key-value as not the only element, with other resources that do not exist",
AnnotationsAllowlist: map[string][]string{"*": {"*"}, "cronjobs": {"*"}},
EnabledResources: []string{"cronjobs", "pods", "deployments", "foo", "bar"},
Wanted: LabelsAllowList{},
err: expectedError{
expectedResourceError: true,
},
},
}

for _, test := range tests {
b := NewBuilder()

// Set the enabled resources.
err := b.WithEnabledResources(test.EnabledResources)
if err != nil && !test.err.expectedResourceError {
t.Log("Did not expect error while setting resources (--resources).")
t.Errorf("Test error for Desc: %s. Got Error: %v", test.Desc, err)
}

// Resolve the allow list.
err = b.WithAllowAnnotations(test.AnnotationsAllowlist)
if err != nil && !test.err.expectedLabelError {
t.Log("Did not expect error while parsing allow list annotations (--metric-annotations-allowlist).")
t.Errorf("Test error for Desc: %s. Got Error: %v", test.Desc, err)
}
resolvedAllowAnnotations := LabelsAllowList(b.allowAnnotationsList)

// Evaluate.
if !reflect.DeepEqual(resolvedAllowAnnotations, test.Wanted) && !test.err.expectedNotEqual {
t.Log("Expected maps to be equal.")
t.Errorf("Test error for Desc: %s\n Want: \n%+v\n Got: \n%#+v", test.Desc, test.Wanted, resolvedAllowAnnotations)
}
}
}
4 changes: 3 additions & 1 deletion pkg/app/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,9 @@ func RunKubeStateMetrics(ctx context.Context, opts *options.Options) error {
storeBuilder.WithKubeClient(kubeClient)

storeBuilder.WithSharding(opts.Shard, opts.TotalShards)
storeBuilder.WithAllowAnnotations(opts.AnnotationsAllowList)
if err := storeBuilder.WithAllowAnnotations(opts.AnnotationsAllowList); err != nil {
return fmt.Errorf("failed to set up annotations allowlist: %v", err)
}
if err := storeBuilder.WithAllowLabels(opts.LabelsAllowList); err != nil {
return fmt.Errorf("failed to set up labels allowlist: %v", err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/builder/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ func (b *Builder) WithFamilyGeneratorFilter(l generator.FamilyGeneratorFilter) {
}

// WithAllowAnnotations configures which annotations can be returned for metrics
func (b *Builder) WithAllowAnnotations(annotations map[string][]string) {
b.internal.WithAllowAnnotations(annotations)
func (b *Builder) WithAllowAnnotations(annotations map[string][]string) error {
return b.internal.WithAllowAnnotations(annotations)
}

// WithAllowLabels configures which labels can be returned for metrics
Expand Down
2 changes: 1 addition & 1 deletion pkg/builder/types/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ type BuilderInterface interface {
WithCustomResourceClients(cs map[string]interface{})
WithUsingAPIServerCache(u bool)
WithFamilyGeneratorFilter(l generator.FamilyGeneratorFilter)
WithAllowAnnotations(a map[string][]string)
WithAllowAnnotations(a map[string][]string) error
WithAllowLabels(l map[string][]string) error
WithGenerateStoresFunc(f BuildStoresFunc)
DefaultGenerateStoresFunc() BuildStoresFunc
Expand Down