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

router, formatter: make address formats consistent in substitution_formatter & header_formatter #19613

Merged
merged 3 commits into from
Jan 25, 2022
Merged

router, formatter: make address formats consistent in substitution_formatter & header_formatter #19613

merged 3 commits into from
Jan 25, 2022

Conversation

robbat2
Copy link
Contributor

@robbat2 robbat2 commented Jan 19, 2022

The substitution_formatter & header_formatter formatters do not cover
all 15 combinations of address & output type, so make it more
consistent.

Output formats:

  • _ADDRESS
  • _ADDRESS_WITHOUT_PORT
  • _PORT

For each each address in the set:

  • UPSTREAM_LOCAL_
  • UPSTREAM_REMOTE_
  • DOWNSTREAM_LOCAL_
  • DOWNSTREAM_REMOTE_
  • DOWNSTREAM_DIRECT_REMOTE_

Signed-off-by: Robin H. Johnson robbat2@gentoo.org
Signed-off-by: Robin H. Johnson rjohnson@digitalocean.com
Risk Level: Low
Testing: unit tests added & passed for all inputs
Docs Changes: Adds format specifiers
Release Notes: router: add upstream/downstream local/remote format specifiers; formatter: add upstream/downstream local/remote format specifiers
Platform Specific Features: N/A

router, formatter: make address formats consistent in substitution_fo…
b90f1c7 
…rmatter & header_formatter

The substitution_formatter & header_formatter formatters do not cover
all 15 combinations of address & output type, so make it more
consistent.

Output formats:
- _ADDRESS
- _ADDRESS_WITHOUT_PORT
- _PORT

For each each address in the set:
- UPSTREAM_LOCAL_
- UPSTREAM_REMOTE_
- DOWNSTREAM_LOCAL_
- DOWNSTREAM_REMOTE_
- DOWNSTREAM_DIRECT_REMOTE_

Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>
@repokitteh-read-only
Copy link

Hi @robbat2, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #19613 was opened by robbat2.

see: more, trace.

access_loggers: update test for mock tweaks of downstream_direct_remo…
9792659 
…te & upstream_local

The mocks were updated to use distinct addresses for
downstream_direct_remote (previously the same as downstream_remote) &
upstream_local (previously empty).

The http_grpc_access_log_impl_test suite needs matching updates to keep
passing.

Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>
@robbat2
Copy link
Contributor Author

robbat2 commented Jan 19, 2022

The OSX failure is AllWorkersAreHandlingLoad/IPv6, and it's not clear why it only seems to fail on OSX, when it works everywhere else.

test/integration/integration_test.cc:205
Value of: w1_ctr > 1
  Actual: false
Expected: true
Stack trace:
  0x10cb3170c: Envoy::IntegrationTest_AllWorkersAreHandlingLoad_Test::TestBody()

@wbpcode
Copy link
Member

wbpcode commented Jan 20, 2022 •
edited
Loading

/assign-from @envoyproxy/first-pass-reviewers

@repokitteh-read-only
Copy link

@envoyproxy/first-pass-reviewers cannot be assigned to this issue.

🐱

Caused by: a #19613 (comment) was created by @wbpcode.

see: more, trace.

@wbpcode
Copy link
Member

wbpcode commented Jan 20, 2022

/assign-from @envoyproxy/first-pass-reviewers

@repokitteh-read-only
Copy link

@envoyproxy/first-pass-reviewers assignee is @RyanTheOptimist

🐱

Caused by: a #19613 (comment) was created by @wbpcode.

see: more, trace.

@RyanTheOptimist
Copy link
Contributor

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19613 (comment) was created by @RyanTheOptimist.

see: more, trace.

RyanTheOptimist
RyanTheOptimist previously approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've re-kicked off the tests to see if that failure was a flake.

@RyanTheOptimist
Copy link
Contributor

/assign-from envoyproxy/senior-maintainers

@repokitteh-read-only
Copy link

envoyproxy/senior-maintainers assignee is @htuch

🐱

Caused by: a #19613 (comment) was created by @RyanTheOptimist.

see: more, trace.

http_conn_man/headers, access_log/usage: improve wording on address f…
1f6c7ef 
…ormat strings

As suggested by htuch in PR#19613 review, this removes confusing wording
for the address format strings, e.g. DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT.

The new wording is applied consistently to all of the address format
strings in documentation for headers and usage logs.

Reference: envoyproxy#19613 (comment)
Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>
Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
@robbat2 robbat2 requested a review from htuch January 25, 2022 05:15
htuch
htuch approved these changes Jan 25, 2022
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch merged commit 4b38a99 into envoyproxy:main Jan 25, 2022
@mattklein123
Copy link
Member

@robbat2 I think this change should have an actual release note and not just doc updates? Can you do a follow up PR to add a release note about this? Thank you.

@robbat2 robbat2 mentioned this pull request Jan 26, 2022
Merged
robbat2 pushed a commit to robbat2/envoy that referenced this pull request Jan 27, 2022
router, formatter: release notes for address formats in substitution_…
6c3af22 
…formatter & header_formatter

PR#19613 missed the release note addition, which is added in this PR.

Risk Level: Zero
Testing: N/A. Documentation.
Docs Changes: Release notes
Release Notes: this change
Platform Specific Features: N/A
Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>
Fixes: envoyproxy#19613
Reference: envoyproxy#19613 (comment)
Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>
mattklein123 pushed a commit that referenced this pull request Jan 27, 2022
router, formatter: release notes for address formats in substitution_…
5233d32 
…formatter & header_formatter (#19708)

PR#19613 missed the release note addition, which is added in this PR.

Risk Level: Zero
Testing: N/A. Documentation.
Docs Changes: Release notes
Release Notes: this change
Platform Specific Features: N/A
Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>
Fixes: #19613
Reference: #19613 (comment)
Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>
joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
@robbat2
router, formatter: make address formats consistent in substitution_fo…
b78008f 
…rmatter & header_formatter (envoyproxy#19613)

The substitution_formatter & header_formatter formatters do not cover
all 15 combinations of address & output type, so make it more
consistent.

Output formats:

_ADDRESS
_ADDRESS_WITHOUT_PORT
_PORT
For each each address in the set:

UPSTREAM_LOCAL_
UPSTREAM_REMOTE_
DOWNSTREAM_LOCAL_
DOWNSTREAM_REMOTE_
DOWNSTREAM_DIRECT_REMOTE_

Risk Level: Low
Testing: unit tests added & passed for all inputs
Docs Changes: Adds format specifiers
Release Notes: router: add upstream/downstream local/remote format specifiers; formatter: add upstream/downstream local/remote format specifiers
Platform Specific Features: N/A

Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>
Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
Signed-off-by: Josh Perry <josh.perry@mx.com>
joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
router, formatter: release notes for address formats in substitution_…
f3da06b 
…formatter & header_formatter (envoyproxy#19708)

PR#19613 missed the release note addition, which is added in this PR.

Risk Level: Zero
Testing: N/A. Documentation.
Docs Changes: Release notes
Release Notes: this change
Platform Specific Features: N/A
Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>
Fixes: envoyproxy#19613
Reference: envoyproxy#19613 (comment)
Signed-off-by: Robin H. Johnson <rjohnson@digitalocean.com>
Signed-off-by: Josh Perry <josh.perry@mx.com>
@GUI GUI mentioned this pull request Sep 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch approved these changes

@RyanTheOptimist RyanTheOptimist RyanTheOptimist left review comments

Assignees

@htuch htuch

@RyanTheOptimist RyanTheOptimist

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@robbat2 @wbpcode @RyanTheOptimist @mattklein123 @htuch