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

envoy: fix panic writing accesslog without L7 tags #27453

Merged

Conversation

mhofstetter
Copy link
Member

@mhofstetter mhofstetter commented Aug 11, 2023

PR #24649 removed the deprecated support for adding fallback HTTP log tags if the access log entry wasn't of type HTTP, Kafka or GenericL7.

This removal leads to panics when the l7Tags function (nil) is called while trying to enrich the log entry in cases where no L7 tags are available.

Therefore, this commit changes the behaviour by defaulting to an noop l7Tags function if no specific L7 information are present.

Needs backport to v1.14 & v1.13 (#26722)

Fixes: #27442

PR cilium#24649 removed the deprecated support for adding fallback
HTTP log tags if the access log entry wasn't of type HTTP,
Kafka or GenericL7.

This removal leads to panics when the l7Tags function (`nil`)
is called while trying to enrich the log entry.

Therefore, this commit changes the behaviour by defaulting to an
noop l7Tags function if no specific L7 information are present.

Fixes: cilium#27442

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter added kind/bug This is a bug in the Cilium logic. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Aug 11, 2023
@mhofstetter
Copy link
Member Author

/test

@mhofstetter mhofstetter added needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Aug 11, 2023
@mhofstetter mhofstetter marked this pull request as ready for review August 11, 2023 12:02
@mhofstetter mhofstetter requested a review from a team as a code owner August 11, 2023 12:02
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks and LGTM ✅

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 14, 2023
@christarazi christarazi merged commit e25bd05 into cilium:main Aug 14, 2023
60 checks passed
@mhofstetter mhofstetter deleted the pr/mhofstetter/envoy-accesslog-fix-l7tags branch August 15, 2023 08:23
@tklauser tklauser mentioned this pull request Aug 22, 2023
22 tasks
@tklauser tklauser added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Aug 22, 2023
@tklauser tklauser mentioned this pull request Aug 23, 2023
8 tasks
@tklauser tklauser added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Aug 23, 2023
@joestringer joestringer added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Daemonset panic after adding policy.cilium.io/proxy-visibility to some pod
5 participants