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

Make %UPSTREAM_LOCAL_ADDRESS% access-log format work for HTTP requests. #9362

Merged
merged 5 commits into from
Dec 20, 2019
Merged

Make %UPSTREAM_LOCAL_ADDRESS% access-log format work for HTTP requests. #9362

merged 5 commits into from
Dec 20, 2019

Conversation

ggreenway
Copy link
Contributor

Make %UPSTREAM_LOCAL_ADDRESS% access-log format work for HTTP requests. It was previously only implemented for tcp-proxy connections.

Risk Level: Low
Testing: Unit and integration tests
Docs Changes: None required; it was never documented to not work
Release Notes: Added

@ggreenway
Make %UPSTREAM_LOCAL_ADDRESS% access-log format work for HTTP requests.
c644834 
Signed-off-by: Greg Greenway <ggreenway@apple.com>
@asraa
Copy link
Contributor

asraa commented Dec 16, 2019

Just a drive-by: would you be able to add coverage for this in our access_log_formatter_fuzz_test? That would involve

  1. adding the field to the StreamInfo information we fuzz here:

    envoy/test/fuzz/common.proto

    Line 18 in 210acff

    message StreamInfo {
  2. setting it in the utility function that constructs the testStreamInfo with that value here:

    envoy/test/fuzz/utility.h

    Line 114 in 210acff

    inline TestStreamInfo fromStreamInfo(const test::fuzz::StreamInfo& stream_info) {
  3. adding a testcase with %UPSTREAM_LOCAL_ADDRESS% and a test value like this one https://github.com/envoyproxy/envoy/blob/master/test/common/access_log/access_log_formatter_corpus/response_code

@ggreenway
Add fuzz test case
1fe8b17 
Signed-off-by: Greg Greenway <ggreenway@apple.com>
@ggreenway
Copy link
Contributor Author

Just a drive-by: would you be able to add coverage for this in our access_log_formatter_fuzz_test?

Good idea. I made an attempt at it.

@ggreenway
fix_format
f043bd0 
Signed-off-by: Greg Greenway <ggreenway@apple.com>
@asraa
Copy link
Contributor

asraa commented Dec 17, 2019

Just a drive-by: would you be able to add coverage for this in our access_log_formatter_fuzz_test?

Good idea. I made an attempt at it.

Yay! Running the fuzz test as a bazel test target will run it against that corpus entry. Thank you so much!

@ggreenway
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #9362 (comment) was created by @ggreenway.

see: more, trace.

@ggreenway
empty commit to restart CI that is stuck
55ce16b 
Signed-off-by: Greg Greenway <ggreenway@apple.com>
asraa
asraa approved these changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thank you!! LGTM

@ggreenway
Copy link
Contributor Author

@asraa Any idea why CI isn't completing? I pushed an empty commit yesterday because envoy-linux was still waiting for status to be reported. Now many of the steps are stuck in that state.

@asraa
Copy link
Contributor

asraa commented Dec 20, 2019 •
edited
Loading

/retest
/azp run

Hmmm. It might be that envoy-linux is stuck, will get back to you on why. Retesting doesn't work for me either

@repokitteh-read-only
Copy link

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #9362 (comment) was created by @asraa.

see: more, trace.

@mattklein123 mattklein123 reopened this Dec 20, 2019
@mattklein123
Copy link
Member

/azp run envoy-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mattklein123 mattklein123 self-assigned this Dec 20, 2019
@ggreenway
Copy link
Contributor Author

/retest

==> Downloading https://github.com/bazelbuild/bazelisk/releases/download/v1.2.0/bazelisk-darwin-amd64
==> Downloading from https://github-production-release-asset-2e65be.s3.amazonaws.com/149661467/54792900-234a-11ea-8134-faf6a597f7d5?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20191220%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20191220T162857Z&X-Amz-Expires=300&X-Amz-Signature=3f10de6b240b718bde337439c5a3e5c165059b77636d553702cc5ca0bc6f8dd6&X-Amz-SignedHeaders=host&actor_id=0&response-content-disposition=attachment%3B%20filename%3Dbazelisk-darwin-amd64&response-content-type=application%2Foctet-stream
Error: An exception occurred within a child process:
  ChecksumMismatchError: SHA256 mismatch
Expected: 6bfca2304cc2258578658d95a6e8dbe43d3ae5dd089076128c726cf14b8c2b8f
  Actual: c9764d559b044a69ec860235d46c5cc7d28e98cb378dd449b7c9fe53f696aa07
 Archive: /Users/runner/Library/Caches/Homebrew/downloads/49bcf3ae6f2484900431af3c21d7d5f7d324dc128ac8be370cbaae117ffa0b4c--bazelisk-darwin-amd64
To retry an incomplete download, remove the file above.

Any other macOS builds seeing this error?

@repokitteh-read-only
Copy link

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #9362 (comment) was created by @ggreenway.

see: more, trace.

@ggreenway
Copy link
Contributor Author

According to https://github.com/bazelbuild/bazelisk/releases/tag/v1.2.0, this version was released 1 hour ago; I think they re-released it, which changed the hash.

@mattklein123 mattklein123 merged commit 9ad469d into envoyproxy:master Dec 20, 2019
spenceral added a commit to spenceral/envoy that referenced this pull request Dec 20, 2019
@spenceral
Merge branch 'master' into filter-config-dump
ade8dbe 
* master: (167 commits)
  stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, using the new MemBlock wrapper for memcpy (envoyproxy#8779)
  Make %UPSTREAM_LOCAL_ADDRESS% access-log format work for HTTP requests. (envoyproxy#9362)
  tools: API boosting support for using decls and enum constants. (envoyproxy#9418)
  Fix incorrect cluster InitializePhase type (envoyproxy#9379)
  build: fix merge race between envoyproxy#9241 and envoyproxy#9413. (envoyproxy#9427)
  fuzz: fix incorrect evaluator test (envoyproxy#9402)
  server: fix bogus startup log message (envoyproxy#9404)
  tools: Add protoxform tests (envoyproxy#9241)
  api: options after import (envoyproxy#9413)
  misc: use std::move instead of constructing a copy (envoyproxy#9415)
  tools: API boosting support for rewriting elaborated types. (envoyproxy#9375)
  docs: fix invalid transport_socket value (envoyproxy#9403)
  fix typo in docs (envoyproxy#9394)
  srds: remove to-de-removed scopes first and then apply additions to avoid scope key conflict. (envoyproxy#9366)
  api: generate whole directory and sync (envoyproxy#9382)
  bazel: Add load statements for proto_library (envoyproxy#9367)
  Fix typo (envoyproxy#9388)
  Correct test of OptionsImpl argc type (Was: Correct type for std::array size() result) (envoyproxy#9290)
  http1 encode trailers in chunk encoding (envoyproxy#8667)
  Add mode to PipeInstance (envoyproxy#8423)
  ...
prakhag1 pushed a commit to prakhag1/envoy that referenced this pull request Jan 3, 2020
@ggreenway @prakhag1
Make %UPSTREAM_LOCAL_ADDRESS% access-log format work for HTTP request…
c14f700 
…s. (envoyproxy#9362)

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Prakhar <prakhar_au@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asraa asraa asraa approved these changes

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@mattklein123 mattklein123 Awaiting requested review from mattklein123

Assignees

@asraa asraa

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ggreenway @asraa @mattklein123