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

fix unexpected behavior of multi accesslogging filters #43372

Merged
merged 3 commits into from Feb 24, 2023
Merged

fix unexpected behavior of multi accesslogging filters #43372

merged 3 commits into from Feb 24, 2023

Conversation

fatedier
Copy link
Contributor

@fatedier fatedier commented Feb 15, 2023 •
edited by istio-policy-bot

Please provide a description of this PR:
Fix #43371

@fatedier fatedier requested a review from a team as a code owner February 15, 2023 18:30
@istio-policy-bot istio-policy-bot added the release-notes-none Indicates a PR that does not require release notes. label Feb 15, 2023
@istio-policy-bot
Copy link

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

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 15, 2023
bleggett
bleggett approved these changes Feb 15, 2023
View reviewed changes
Copy link
Contributor

@bleggett bleggett left a comment

Choose a reason for hiding this comment

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

LGTM

@bleggett
Copy link
Contributor

/retest

@zirain zirain removed the release-notes-none Indicates a PR that does not require release notes. label Feb 16, 2023
zirain
zirain reviewed Feb 16, 2023
View reviewed changes
Copy link
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

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

good catch, please add release-notes for backport

@fatedier fatedier requested review from a team as code owners February 16, 2023 05:16
hzxuzhonghu
hzxuzhonghu approved these changes Feb 16, 2023
View reviewed changes
Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

LGTM

@fatedier
Copy link
Contributor Author

/retest

@istio-testing
Copy link
Collaborator

istio-testing commented Feb 20, 2023 •
edited

@fatedier: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
integ-ambient_istio 85401d8128ff30931db6c8db97349fef31fcb984 link true /test integ-ambient

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/test-infra repository. I understand the commands that are listed here.

@zirain
Copy link
Member

zirain commented Feb 20, 2023

wait for #43461

zirain
zirain reviewed Feb 20, 2023
View reviewed changes
pilot/pkg/networking/core/v1alpha3/accesslog.go
@@ -81,7 +81,7 @@ func (b *AccessLogBuilder) setTCPAccessLog(push *model.PushContext, proxy *model
mesh := push.Mesh
cfgs := push.Telemetry.AccessLogging(push, proxy, class)

if cfgs == nil {
if len(cfgs) == 0 {
Copy link
Member

@zirain zirain Feb 20, 2023 •
edited

Choose a reason for hiding this comment

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

why need this?
nil means there's no telemetry api

istio/pilot/pkg/model/telemetry.go

Line 240 in 57f3b2b

if len(ct.Logging) == 0 && len(t.meshConfig.GetDefaultProviders().GetAccessLogging()) == 0 {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix this case:

istio/pilot/pkg/networking/core/v1alpha3/accesslog_test.go

Line 459 in 7d00611

name: "log-selector-unmatched-telemetry",

described here: #43371

Copy link
Member

Choose a reason for hiding this comment

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

can you retest without this changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

I got this point, this's someting was talked in #39521 (comment).

thanks for catching this!

Copy link
Contributor Author

@fatedier fatedier Mar 3, 2023 •
edited

Choose a reason for hiding this comment

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

@zirain I found that there will be a new problem here, which will cause the disable access logs to fail. However, I am confused as to why this e2e test passed. https://github.com/istio/istio/blob/master/tests/integration/telemetry/api/accesslogs_test.go#L58

It failed when I tried to cherry-pick it to release 1.16.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zirain I have submitted a pull request to fix this issue: #43770

@zirain
Copy link
Member

zirain commented Feb 20, 2023

/retest

zirain
zirain approved these changes Feb 20, 2023
View reviewed changes
Copy link
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

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

LGTM

@zirain zirain added cherrypick/release-1.17 Set this label on a PR to auto-merge it to the release-1.17 branch cherrypick/release-1.16 Set this label on a PR to auto-merge it to the release-1.16 branch labels Feb 20, 2023
@zirain
Copy link
Member

zirain commented Feb 21, 2023

/test integ-ambient

@fatedier @ericvn
Update releasenotes/notes/43372.yaml
72eabfe 
Co-authored-by: Eric Van Norman <ericvn@us.ibm.com>
GregHanson
GregHanson approved these changes Feb 24, 2023
View reviewed changes
Copy link
Member

@GregHanson GregHanson left a comment

Choose a reason for hiding this comment

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

release note looks good

@istio-testing istio-testing merged commit ffddbbf into istio:master Feb 24, 2023
@istio-testing istio-testing mentioned this pull request Feb 24, 2023
Closed
@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new pull request created: #43590

@istio-testing istio-testing mentioned this pull request Feb 24, 2023
Merged
@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new pull request created: #43591

This was referenced Mar 5, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericvn ericvn ericvn left review comments

@bleggett bleggett bleggett approved these changes

@zirain zirain zirain approved these changes

@hzxuzhonghu hzxuzhonghu hzxuzhonghu approved these changes

@GregHanson GregHanson GregHanson approved these changes

Assignees
No one assigned
Labels
area/extensions and telemetry cherrypick/release-1.16 Set this label on a PR to auto-merge it to the release-1.16 branch cherrypick/release-1.17 Set this label on a PR to auto-merge it to the release-1.17 branch size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TelemetryAPI: unexpected behavior of multi accesslogging filters
8 participants
@fatedier @istio-policy-bot @bleggett @istio-testing @zirain @ericvn @hzxuzhonghu @GregHanson