Skip to content

fix the issue of regexp matching#1812

Merged
k8s-ci-robot merged 1 commit into
kubernetes-sigs:masterfrom
Martande8055:issue-1810-regexp-fix
Apr 23, 2025
Merged

fix the issue of regexp matching#1812
k8s-ci-robot merged 1 commit into
kubernetes-sigs:masterfrom
Martande8055:issue-1810-regexp-fix

Conversation

@Martande8055

@Martande8055 Martande8055 commented Apr 1, 2025 •
edited by saschagrunert
Loading

Copy link
Copy Markdown
Contributor

fork proposal for exact-string regexp match to scope search results more accurately. "--namespace=test" should not match "test1,test2,test3,test4,testtakersnamespace" --name=example" should not match "examplepod,examplehost..etc"

What type of PR is this?

/kind bug

What this PR does / why we need it:

Observed that crictl pods --namespace "name" --name "name" -q results in multiple pod id results:

$ crictl pods --namespace test --name msender -q
95d9a10438afa5d01a42c946e0247f96bd994ca22287ef0a550af2dbbe830ed4
ef3815c75a41f5498476ba4e87eefdca822abb407f2a0432b253e148ba735a55
5b0236afd6b2645f81d773c0e3d6bc588a75fe6ea486c4dc33a8a1f51aa079e6 


crictl pods | grep msender                                          
POD ID              CREATED              STATE               NAME                                                                      NAMESPACE                                ATTEMPT             RUNTIME
95d9a10438afa       29 seconds ago       Ready               msender                                                                   test3                                    0                   (default)
00fd513962cc0       36 minutes ago       Ready               mlistener1                                                                test                                     0                   (default)
ef3815c75a41f       2 hours ago          Ready               msender                                                                   test2                                    0                   (default)
5b0236afd6b26       2 hours ago          Ready               msender                                                                   test                                     0                   (default)

This is because it's doing an inexact regexp match on the strings for "name" and "namespace":

nameRegexp:         c.String("name"),
podNamespaceRegexp: c.String("namespace"),

I posit that if we're supplying the string values for this search function, having an exact match will resolve incorrect outputs and ensure we're only returning the exact results from the search that should match.

With the change we'd get the correct string:

$ chroot /host crictl pods --namespace test --name msender -q
5b0236afd6b2645f81d773c0e3d6bc588a75fe6ea486c4dc33a8a1f51aa079e6

## matching only:
5b0236afd6b26       2 hours ago          Ready               msender                                                                   test                                     0                   (default)

I believe (but will need some help testing) that the function here that we're calling at https://github.com/kubernetes-sigs/cri-tools/blob/de83eda06541a3712faa42b87958e6428c848aef/cmd/crictl/util.go#L535C1-L547C2

Should be able to handle the regexp options.

Which issue(s) this PR fixes:

Fixes #1896

Special notes for your reviewer:

Low urgency, appreciate your time.

Does this PR introduce a user-facing change?

Update modifies container.go handling of `--namespace="string"` and `--name="string"` to exact match regexp (^string$) to avoid errant results caused by multiple namespaces or pod name overlap.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Apr 1, 2025 •
edited
Loading

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Martande8055 / name: Saurabh Subhash Martande (bd70f72)

@k8s-ci-robot

k8s-ci-robot commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

Welcome @Martande8055!

It looks like this is your first PR to kubernetes-sigs/cri-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cri-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 1, 2025
@Martande8055 Martande8055 mentioned this pull request Apr 1, 2025
Closed
@saschagrunert

saschagrunert commented Apr 22, 2025

Copy link
Copy Markdown
Member

Please rebase to fix the CI.

@Martande8055
Martande8055 force-pushed the issue-1810-regexp-fix branch from e73901d to bd70f72 Compare April 23, 2025 07:40
@Martande8055

Martande8055 commented Apr 23, 2025

Copy link
Copy Markdown
Contributor Author

i have now rebased my PR @saschagrunert

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2025
@k8s-ci-robot

k8s-ci-robot commented Apr 23, 2025

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Martande8055, saschagrunert

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2025
@k8s-ci-robot
k8s-ci-robot merged commit eeb2b65 into kubernetes-sigs:master Apr 23, 2025
@saschagrunert saschagrunert added kind/feature Categorizes issue or PR as related to a new feature. kind/bug Categorizes issue or PR as related to a bug. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Aug 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saschagrunert saschagrunert saschagrunert approved these changes
@haircommander haircommander Awaiting requested review from haircommander
@klueska klueska Awaiting requested review from klueska

Assignees

@saschagrunert saschagrunert

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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Martande8055 @k8s-ci-robot @saschagrunert