8000 rewrite probe only when the port needs to be intercepted by psikka1 · Pull Request #56387 · istio/istio · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

rewrite probe only when the port needs to be intercepted #56387

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
Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

rewrite probe only when the port needs to be intercepted #56387

wants to merge 13 commits into from
+1,182 −16

Conversation

psikka1
Copy link
@psikka1 psikka1 commented May 22, 2025 •
edited by istio-policy-bot
Loading

Please provide a description of this PR:
This fixes #53759

@psikka1 psikka1 requested review from a team as code owners May 22, 2025 21:46
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link
linux-foundation-easycla bot commented May 22, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@istio-policy-bot
Copy link

😊 Welcome @psikka1! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test labels May 22, 2025
@istio-testing
Copy link
Collaborator

Hi @psikka1. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@aattuluri
Copy link
Member

/ok-to-test

1 similar comment
@keithmattix
Copy link
Contributor

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels May 27, 2025
@istio-testing istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 28, 2025
dhawton
dhawton reviewed May 29, 2025
View reviewed changes
howardjohn
howardjohn requested changes May 31, 2025
View reviewed changes
Copy link
Member
@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

the probe being removed in ~all the tests seems definitely wrong?

I'm not entirely sure we need this complexity either, a user can always opt out of the probe rewrite with an annotation today. I'm worried about this accidently causing unintentional chanes

@psikka1
Copy link
Author
psikka1 commented Jun 3, 2025

the probe being removed in ~all the tests seems definitely wrong?

I'm not entirely sure we need this complexity either, a user can always opt out of the probe rewrite with an annotation today. I'm worried about this accidently causing unintentional chanes

@howardjohn - I am making modifications to retain the original behavior as much as possible. I had a discussion with @aattuluri regarding the same.

aattuluri
aattuluri suggested changes Jun 18, 2025
View reviewed changes
Copy link
Member
@aattuluri aattuluri left a comment

Choose a reason for hiding this comment

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

Please see comments.

@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 18, 2025
@istio-testing istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 26, 2025
dhawton
dhawton requested changes Jun 26, 2025
View reviewed changes
Copy link
Member
@dhawton dhawton left a comment

Choose a reason for hiding this comment

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

See my comment about release notes.

@psikka1 psikka1 requested a review from dhawton June 26, 2025 17:36
dhawton
dhawton reviewed Jun 27, 2025
View reviewed changes
@psikka1 @dhawton
Update releasenotes/notes/56387.yaml
81852b8 
Co-authored-by: Daniel Hawton <daniel@hawton.org>
@psikka1 psikka1 requested a review from dhawton June 27, 2025 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn Awaiting requested review from howardjohn

@aattuluri aattuluri Awaiting requested review from aattuluri

@dhawton dhawton Awaiting requested review from dhawton

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
area/networking area/user experience ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

istio rewrites app probers irrespective of the ports mentioned in includeInboundPorts annotation
7 participants
@psikka1 @istio-policy-bot @istio-testing @aattuluri @keithmattix @howardjohn @dhawton
0