Skip to content

grpc access logs: include passed in command parsers when validating commands#42915

Open
AntonKanug wants to merge 6 commits intoenvoyproxy:mainfrom
AntonKanug:antonk/dns-logs-on-init
Open

grpc access logs: include passed in command parsers when validating commands#42915
AntonKanug wants to merge 6 commits intoenvoyproxy:mainfrom
AntonKanug:antonk/dns-logs-on-init

Conversation

@AntonKanug
Copy link
Contributor

@AntonKanug AntonKanug commented Jan 8, 2026 •
edited
Loading

Fixes: #43396

Commit Message: grpc access logs: include passed in command parsers when validating commands
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:

Fixes listener initializing errors when using custom tag substitution for dns filter logs.

delta config for type.googleapis.com/envoy.config.listener.v3.Listener rejected: Error adding/updating listener(s) listener: Not supported field in StreamInfo: QUERY_NAME

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Jan 8, 2026

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #42915 was opened by AntonKanug.

see: more, trace.

@AntonKanug AntonKanug force-pushed the antonk/dns-logs-on-init branch 2 times, most recently from 18b765e to a903416 Compare January 8, 2026 23:48
@AntonKanug AntonKanug marked this pull request as ready for review January 9, 2026 10:25
@AntonKanug
Copy link
Contributor Author

AntonKanug commented Jan 9, 2026

cc @wbpcode

@AntonKanug AntonKanug force-pushed the antonk/dns-logs-on-init branch 2 times, most recently from 1d7ad8d to e3925b1 Compare January 9, 2026 12:39
@wbpcode
Copy link
Member

wbpcode commented Jan 11, 2026

Thanks for this contribution. But

  1. I think to configure DNS's related format in the listener make no sense anyway?
  2. We prefer only keep HTTP and TCP's format as built-in and to reduce the potential name conflict?

@AntonKanug
Copy link
Contributor Author

AntonKanug commented Jan 11, 2026 •
edited
Loading

  1. I think to configure DNS's related format in the listener make no sense anyway?

We use a udp listener with dns filter (similar to this example)

  1. We prefer only keep HTTP and TCP's format as built-in and to reduce the potential name conflict?

whats the suggestion to fix envoy rejecting listener config due to command not being recognized in this case?

delta config for type.googleapis.com/envoy.config.listener.v3.Listener rejected: Error adding/updating listener(s) listener: Not supported field in StreamInfo: QUERY_NAME

@wbpcode
Copy link
Member

wbpcode commented Jan 24, 2026

We use a udp listener with dns filter (similar to this example)

Sorry, I didn't make it clear. I mean, at the listener level, we cannot access the single DNS query attributes anyway, right? Then, we we need to configure the DNS command that? Could we remove the UDP listener's access logger or to update listener level format?

@wbpcode
Copy link
Member

wbpcode commented Jan 28, 2026

/wait

@AntonKanug AntonKanug force-pushed the antonk/dns-logs-on-init branch from a69afac to b406204 Compare February 9, 2026 08:14
@AntonKanug
dns_filter: make DNS format commands available at config initialization
a768a06 
Signed-off-by: Anton Kanugalawattage <antondilon@gmail.com>

u

Signed-off-by: Anton Kanugalawattage <antondilon@gmail.com>

format

Signed-off-by: Anton Kanugalawattage <antondilon@gmail.com>

test

Signed-off-by: Anton Kanugalawattage <antondilon@gmail.com>

update

Signed-off-by: Anton Kanugalawattage <antondilon@gmail.com>

u

Signed-off-by: Anton Kanugalawattage <antondilon@gmail.com>
@AntonKanug
pass command paraser
c99ca13 
Signed-off-by: Anton Kanugalawattage <antondilon@gmail.com>
@AntonKanug AntonKanug force-pushed the antonk/dns-logs-on-init branch from b406204 to c99ca13 Compare February 9, 2026 08:23
@AntonKanug
Copy link
Contributor Author

AntonKanug commented Feb 9, 2026

From slack conversation:

the issue seems to be that non built in command parsers are not taken into account when validating commands, updated PR to fix that

@AntonKanug
unused
a02328e 
Signed-off-by: Anton Kanugalawattage <antondilon@gmail.com>
@yanjunxiang-google
Copy link
Contributor

yanjunxiang-google commented Feb 11, 2026 •
edited
Loading

Could you please add some integration tests to reproduce the issue and verify the change?

yanjunxiang-google
yanjunxiang-google reviewed Feb 11, 2026
View reviewed changes
test/extensions/filters/udp/dns_filter/dns_filter_test_utils.cc

const std::string record_name = parseDnsNameRecord(buffer, available_bytes, offset);
uint64_t available_bytes_uint64 = available_bytes;
const std::string record_name = parseDnsNameRecord(buffer, available_bytes_uint64, offset);
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google Feb 11, 2026

Choose a reason for hiding this comment

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

emm, this change seems to be just a cleanup, not related to the issue, right?

Copy link
Contributor Author

@AntonKanug AntonKanug Feb 11, 2026

Choose a reason for hiding this comment

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

yeah unrelated to the issue, size_t vs uint64_t caused problems when running tests on my mac

unit test
f1b62b3 
Signed-off-by: Wesley Hung <whung@palantir.com>
@farmpiggie farmpiggie force-pushed the antonk/dns-logs-on-init branch 2 times, most recently from 8b2622e to 4417392 Compare February 12, 2026 17:47
add integ test
ae18ab9 
Signed-off-by: Wesley Hung <whung@palantir.com>
@farmpiggie farmpiggie force-pushed the antonk/dns-logs-on-init branch from 4417392 to ae18ab9 Compare February 12, 2026 18:57
yanjunxiang-google
yanjunxiang-google reviewed Feb 12, 2026
View reviewed changes
test/extensions/filters/udp/dns_filter/dns_filter_integration_test.cc Outdated
EXPECT_EQ(DNS_RESPONSE_CODE_NO_ERROR, response_ctx_->getQueryResponseCode());
}

class DnsFilterAccessLogIntegrationTest
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google Feb 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe put the new access log integration tests in a new file: dns_filter_access_log_integration_test.cc?

Otherwise LGTM!

Copy link
Contributor Author

@AntonKanug AntonKanug Feb 13, 2026

Choose a reason for hiding this comment

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

done in 20c6ad0

@AntonKanug
mv to file
20c6ad0 
Signed-off-by: Anton Kanugalawattage <antondilon@gmail.com>
@AntonKanug AntonKanug changed the title dns_filter: make DNS format commands available at config initialization grpc access logs: include passed in command parsers when validating commands Feb 13, 2026
@AntonKanug
Copy link
Contributor Author

AntonKanug commented Feb 14, 2026

@yanjunxiang-google @wbpcode
#43457 was opened after this pr with the same changes and was merged without the integration tests 😕

cc @farmpiggie

@AntonKanug AntonKanug mentioned this pull request Feb 14, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

@wbpcode wbpcode Awaiting requested review from wbpcode wbpcode is a code owner

@yanjunxiang-google yanjunxiang-google Awaiting requested review from yanjunxiang-google yanjunxiang-google is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@wbpcode wbpcode

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

access_logs: DNS filter access logs using custom tags in gRPC access logs fails validation

3 participants

@AntonKanug @wbpcode @yanjunxiang-google