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

formatter: add custom date formatting to downstream cert start and end dates #14502

Merged
merged 10 commits into from
Jan 6, 2021
Merged

formatter: add custom date formatting to downstream cert start and end dates #14502

merged 10 commits into from
Jan 6, 2021

Conversation

esmet
Copy link
Contributor

@esmet esmet commented Dec 22, 2020 •
edited
Loading

Commit Message: formatter: add custom date formatting to downstream cert start and end dates
Additional Description: refactor and reuse existing behavior from START_TIME
Risk Level: low (new behavior, though existing code is refactored)
Testing: refactored existing unit test, added test cases for existing behaviors to new fields
Docs Changes: Updated docs/root/configuration/observability/access_log/usage.rst to mention formatting, refers back to START_TIME
Release Notes: formatter: added support for custom date formatting to %DOWNSTREAM_PEER_CERT_V_START% and %DOWNSTREAM_PEER_CERT_V_END%, similar to %START_TIME%.
Fixes: #14501

@esmet
Add custom date formatting to downstream cert start and end dates
c8e7cac 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet esmet changed the title Add custom date formatting to downstream cert start and end dates formatter: add custom date formatting to downstream cert start and end dates Dec 22, 2020
@mattklein123 mattklein123 self-assigned this Dec 28, 2020
mattklein123
mattklein123 requested changes Dec 28, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

/wait

source/common/formatter/substitution_formatter.cc Show resolved Hide resolved
@esmet
Refactor to use a generic SystemTimeFormatter with a FieldExtractor c…
ac5f69a 
…allback

Signed-off-by: John Esmet <john.esmet@gmail.com>
esmet added 2 commits January 4, 2021 21:16
@esmet
Add docs
b1ba7dd 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Add changelog
5c98483 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet esmet marked this pull request as ready for review January 4, 2021 21:21
esmet
esmet commented Jan 4, 2021
View reviewed changes
source/common/formatter/substitution_formatter.cc Outdated
// Matches newline pattern in a StartTimeFormatter format string.
const std::regex& getStartTimeNewlinePattern() {
// Matches newline pattern in a system time format string (e.g. start time)
const std::regex& getDateFormatNewlinePattern() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency I should probably rename this to getSystemTimeNewlinePattern

@esmet
More test cases
55fa349 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet esmet requested a review from mattklein123 January 4, 2021 22:14
esmet added 2 commits January 4, 2021 22:20
@esmet
Clarify variable names
42cb290 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Clarify a function name
21bb32a 
Signed-off-by: John Esmet <john.esmet@gmail.com>
mattklein123
mattklein123 requested changes Jan 5, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM with small comments.

/wait

docs/root/version_history/current.rst Outdated
@@ -67,6 +67,7 @@ New Features
* config: added ability to flush stats when the admin's :ref:`/stats endpoint <operations_admin_interface_stats>` is hit instead of on a timer via :ref:`stats_flush_on_admin <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.stats_flush_on_admin>`.
* config: added new runtime feature `envoy.features.enable_all_deprecated_features` that allows the use of all deprecated features.
* formatter: added new :ref:`text_format_source <envoy_v3_api_field_config.core.v3.SubstitutionFormatString.text_format_source>` field to support format strings both inline and from a file.
* formatter: added support for custom date formatting to %DOWNSTREAM_PEER_CERT_V_START% and %DOWNSTREAM_PEER_CERT_V_END%, similar to %START_TIME%.
Copy link
Member

Choose a reason for hiding this comment

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

Please ref link to the relevant fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

source/common/formatter/substitution_formatter.cc Outdated
Comment on lines 379 to 383
const size_t parameters_length = token.length() - (DownstreamPeerCertVStartParamStart + 1);
const std::string args =
token[DownstreamPeerCertVStartParamStart - 1] == '('
? token.substr(DownstreamPeerCertVStartParamStart, parameters_length)
: "";
Copy link
Member

Choose a reason for hiding this comment

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

Can this part be somehow pushed down into the shared parser/formatter? It's pretty confusing and could use more comments anyway. Until this code is rewritten to use an actual parser grammar I would err on the side of more comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds right. I'll add a shared format parsing function and push the logic into each constructor, which should allow this to become an easy one liner.

@esmet
Add links to docs
091e135 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Improve how we parse system time formats across implementations
7b5326d 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@mattklein123
Copy link
Member

Thanks this looks good. Can you fix the docs build?

/wait

@esmet
Fix docs links
d73b727 
Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Copy link
Contributor Author

esmet commented Jan 5, 2021

I found and fixed the docs issue locally so CI should be happy this time around.

@esmet esmet requested a review from mattklein123 January 5, 2021 22:07
mattklein123
mattklein123 approved these changes Jan 6, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 1bf77f3 into envoyproxy:master Jan 6, 2021
mpuncel added a commit to mpuncel/envoy that referenced this pull request Jan 8, 2021
@mpuncel
Merge branch 'master' into mpuncel/http2-hc-goaway
eacd1f4 
* master: (48 commits)
  Resolve 14506, avoid libidn2 for our curl dependency (envoyproxy#14601)
  fix new/free mismatch in Mainthread utility (envoyproxy#14596)
  opencensus: deprecate Zipkin configuration. (envoyproxy#14576)
  upstream: clean up code location (envoyproxy#14580)
  configuration impl: add cast for ios compilation (envoyproxy#14590)
  buffer impl: add cast for android compilation (envoyproxy#14589)
  ratelimit: add dynamic metadata to ratelimit response (envoyproxy#14508)
  tcp_proxy: wait for CONNECT response before start streaming data (envoyproxy#14317)
  stream info: cleanup address handling (envoyproxy#14432)
  [deps] update upb to latest commit (envoyproxy#14582)
  Add utility to check whether the execution is in main thread. (envoyproxy#14457)
  listener: undeprecate bind_to_port (envoyproxy#14480)
  Fix data race in overload integration test (envoyproxy#14586)
  deps: update PGV (envoyproxy#14571)
  dependencies: update cve_scan.py for some libcurl 7.74.0 false positives. (envoyproxy#14572)
  Network::Connection: Add L4 crash dumping support (envoyproxy#14509)
  ssl: remember stat names for configured ciphers. (envoyproxy#14534)
  formatter: add custom date formatting to downstream cert start and end dates (envoyproxy#14502)
  feat(lua): allow setting response body when the upstream response body is empty (envoyproxy#14486)
  Generalize the gRPC access logger base classes (envoyproxy#14469)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
@esmet esmet deleted the downstream-cert-formatter branch January 11, 2021 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support configurable date formats for DOWNSTREAM_PEER_CERT_V_START/END
2 participants
@esmet @mattklein123