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.

Sign up for GitHub

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

Jump to bottom

feat: allow field KV general matching #2067

Merged
merged 1 commit into from
Aug 29, 2023
Merged

feat: allow field KV general matching #2067

merged 1 commit into from
Aug 29, 2023

Conversation

rexagod
Copy link
Member

@rexagod rexagod commented May 15, 2023

What this PR does / why we need it: Currently all matching support is limited to map-list representations; this adds support for a more general "key:value" based matching expression for any interface{} field (non-map-list-like).

How does this change affect the cardinality of KSM: No change.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 15, 2023
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 15, 2023
mrueg
mrueg reviewed May 16, 2023
View reviewed changes
Copy link
Member

@mrueg mrueg left a comment

Choose a reason for hiding this comment

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

Do you an example use case for this?

docs/customresourcestate-metrics.md Outdated Show resolved Hide resolved
@rexagod
Copy link
Member Author

rexagod commented May 16, 2023

Do you an example use case for this?

So right now we don't support generating metrics from a map for a key:value match. This is useful in cases where there are multiple instances (say, deployments) with key-value pairs that may or may not be present on them, in which case we'd want to make sure that even through the key is same as the one specified in the path, the value matches as well.

# instance A
# ...
labels:
 foo: "bar"
# ...
---
# instance B
# ...
labels:
 foo: "baz"
# ...
---
# config
# ...
labelsFromPath:
  foo: [metadata, labels, "foo:bar"]
# ...

@rexagod rexagod requested a review from mrueg June 12, 2023 09:27
@rexagod rexagod mentioned this pull request Jun 14, 2023
Closed
@rexagod
Copy link
Member Author

rexagod commented Jun 15, 2023

Closing in favor of the newer model.

@rexagod rexagod closed this Jun 15, 2023
@rexagod
Copy link
Member Author

rexagod commented Jun 20, 2023

Reopening in favor of #2068 (comment).

@rexagod rexagod reopened this Jun 20, 2023
dgrisonnet
dgrisonnet reviewed Jun 28, 2023
View reviewed changes
docs/customresourcestate-metrics.md Outdated
@@ -480,4 +480,7 @@ Examples:

# if the value to be matched is a number or boolean, the value is compared as a number or boolean
[status, conditions, "[value=66]", name] # status.conditions[1].name = "b"

# For generally matching against a field in an object schema, use the following syntax:
[metadata, annotations, "bar:baz"] # metadata.annotations["bar:baz"] = "baz" (if present, ignored otherwise)
Copy link
Member

Choose a reason for hiding this comment

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

To be in line with how we specify values, what would you think about:

[metadata, annotations, "[key=bar]", "[value=baz]"]

I feel like it would be easier for users if we stick to the existing UX we have for values and just extend it to keys.

@dgrisonnet
Copy link
Member

/triage accepted
/assign

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 29, 2023
@rexagod rexagod changed the title Allow field KV general matching feat: allow field KV general matching Jul 12, 2023
mrueg
mrueg reviewed Aug 28, 2023
View reviewed changes
docs/customresourcestate-metrics.md Outdated
@@ -480,4 +480,7 @@ Examples:

# if the value to be matched is a number or boolean, the value is compared as a number or boolean
[status, conditions, "[value=66]", name] # status.conditions[1].name = "b"

# For generally matching against a field in an object schema, use the following syntax:
[metadata, annotations, "bar=baz"] # metadata.annotations["bar=baz"] = "baz" (if present, ignored otherwise)
Copy link
Member

Choose a reason for hiding this comment

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

Can we do a "name=a" here similar to a couple of lines above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed, PTAL.

@rexagod
Allow field KV general matching
e52fbce 
Currently all matching support is limited to map-list representations;
this adds support for a more general "key:value" based matching
expression for any `interface{}` field (non-map-list-like).
@mrueg
Copy link
Member

mrueg commented Aug 29, 2023 •
edited
Loading

/approve
/lgtm
/hold
for @dgrisonnet to take a look

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2023
dgrisonnet
dgrisonnet approved these changes Aug 29, 2023
View reviewed changes
Copy link
Member

@dgrisonnet dgrisonnet left a comment

Choose a reason for hiding this comment

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

/lgtm
/unhold

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 29, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, mrueg, rexagod

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [dgrisonnet,mrueg,rexagod]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 5ce46d5 into kubernetes:main Aug 29, 2023
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrueg mrueg mrueg left review comments

@dgrisonnet dgrisonnet dgrisonnet approved these changes

@fpetkovski fpetkovski Awaiting requested review from fpetkovski

Assignees

@dgrisonnet dgrisonnet

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rexagod @dgrisonnet @mrueg @k8s-ci-robot