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

Add support for custom Envoy JSON fields #3059

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

mike1808
Copy link
Contributor

Add support for custom Envoy JSON fields via = in the fields name. The specify custom field name and its Envoy format string use = in the field name. The syntax is the following: field_name=Envoy format string, where Envoy format string can have any command except DYNAMIC_METADATA and FILTER_STATE commands.

Example of custom field:

json-fields:
  - @timestamp
  - customer_id=%REQ(X-CUSTOMER-ID)%

Fixes #3032, Fixes #1507

cc @KauzClay

@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #3059 into main will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3059      +/-   ##
==========================================
- Coverage   73.22%   73.21%   -0.01%     
==========================================
  Files          95       95              
  Lines        6057     6056       -1     
==========================================
- Hits         4435     4434       -1     
  Misses       1521     1521              
  Partials      101      101              
Impacted Files Coverage Δ
internal/xdscache/v2/listener.go 95.10% <ø> (ø)
internal/envoy/v2/accesslog.go 100.00% <100.00%> (ø)

@youngnick youngnick requested review from jpeach, youngnick and stevesloka and removed request for youngnick October 25, 2020 22:39
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

Hi @mike1808,

Overally, I think this looks pretty good.

  1. We should document this in ./site/_site/docs/main/configuration.md. It's
    OK to do this in a separate PR if you prefer.

  2. I think that it would be clearer to split the validation regex to handle the
    simple case (e.g. %HOSTNAME%) and complex case (e.g. %REQ(foo?bar):baz%)
    separately. This would make the regex much more obvious and the code easier to
    follow. The REQ/RESP regex only needs to validate the capture from the simple
    case (i.e. REQ(foo?bar):baz)

  3. Once you do (2), there's an opportunity to have a single definition of the
    simple expressions. If you define EnvoyOperators to just be the simple
    operators, then you can derive the JSON field name from that, and add those
    mappings to JSONFields in an init function. The EnvoyOperators map should
    go into pkg/config/accesslog.go.

pkg/config/parameters_test.go Outdated Show resolved Hide resolved
pkg/config/parameters_test.go Outdated Show resolved Hide resolved
pkg/config/parameters_test.go Outdated Show resolved Hide resolved
pkg/config/parameters_test.go Outdated Show resolved Hide resolved
pkg/config/parameters.go Show resolved Hide resolved
@mike1808
Copy link
Contributor Author

@jpeach

  1. We'll add the docs in the separate PR.
  2. I don't think that's possible because Envoy can have multiple operators in one field. E.g. it can have a simple operator like %HOSTNAME% and parameterized one like %REQ()%. like req from %HOSTNAME% with %REQ(X-CONTNENT-ID)%
  3. Because the option (2) is not possible we cannot do that. But we can move EnvoyOperators to pkg/config/accesslog.go

@mike1808
Copy link
Contributor Author

mike1808 commented Oct 26, 2020

@jpeach we updated the PR and tried to address all review comments. One thing we're not sure about is our approach for keeping parsed access log fields in the Parameters structure.

cc @XanderStrike

@XanderStrike XanderStrike force-pushed the envoy-custom-json-fields branch 3 times, most recently from 4261478 to 2cadfa7 Compare October 27, 2020 00:08
@mike1808
Copy link
Contributor Author

@jpeach we renamed JSONFields to jsonFields and encapsulated all the translation logic in config package.

Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM, nice work. I'm looking forward to seeing the docs update as well, I think that's key for making this feature more usable. We should wait for @jpeach to approve as well though.

Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

Thanks @mike1808, this looks great. Could you just take care of the couple of nits, then we can merge.

pkg/config/parameters.go Outdated Show resolved Hide resolved
pkg/config/parameters.go Outdated Show resolved Hide resolved
pkg/config/parameters.go Outdated Show resolved Hide resolved
Add support for custon Envoy JSON fields via use of `=` in the field
name.

See [design doc](design/envoy-json-logging-custom-fields-design.md) for
more information.

Fixes projectcontour#3032, projectcontour#1507

Signed-off-by: Mikael Manukyan <mmanukyan@vmware.com>
Co-authored-by: Clay Kauzlaric <ckauzlaric@vmware.com>
Co-authored-by: Alexander Standke <astandke@vmware.com>
@XanderStrike
Copy link
Contributor

Latest comments addressed and fixed up into the commit, rebased against projectcontour:main. Ready for review again.

@jpeach jpeach merged commit 93d3016 into projectcontour:main Oct 27, 2020
@jpeach jpeach added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 27, 2020
@XanderStrike XanderStrike deleted the envoy-custom-json-fields branch October 28, 2020 18:36
XanderStrike pushed a commit to cf-routing/contour that referenced this pull request Oct 28, 2020
XanderStrike pushed a commit to cf-routing/contour that referenced this pull request Oct 28, 2020
XanderStrike pushed a commit to cf-routing/contour that referenced this pull request Oct 29, 2020
follow-up for projectcontour#3059

Co-authored-by: Leah Hanson <lhanson@pivotal.io>
XanderStrike pushed a commit to cf-routing/contour that referenced this pull request Oct 30, 2020
follow-up for projectcontour#3059

Co-authored-by: Leah Hanson <lhanson@pivotal.io>
Co-authored-by: Alex Standke <astandke@vmware.com>
jpeach pushed a commit that referenced this pull request Nov 2, 2020
follow-up for #3059

Co-authored-by: Leah Hanson <lhanson@pivotal.io>
Co-authored-by: Alex Standke <astandke@vmware.com>
mike1808 added a commit to cloudfoundry/cf-for-k8s that referenced this pull request Nov 3, 2020
* We'll use "main" branch release until [our access log changes](projectcontour/contour#3059) make into
an actual version

[#175505884](https://www.pivotaltracker.com/story/show/175505884)

Co-authored-by: Clay Kauzlaric <ckauzlaric@vmware.com>
tcdowney pushed a commit to cloudfoundry/cf-for-k8s that referenced this pull request Nov 24, 2020
* We'll use "main" branch release until [our access log changes](projectcontour/contour#3059) make into
an actual version

[#175505884](https://www.pivotaltracker.com/story/show/175505884)

Co-authored-by: Clay Kauzlaric <ckauzlaric@vmware.com>
tcdowney pushed a commit to cloudfoundry/cf-for-k8s that referenced this pull request Dec 3, 2020
* We'll use "main" branch release until [our access log changes](projectcontour/contour#3059) make into
an actual version

[#175505884](https://www.pivotaltracker.com/story/show/175505884)

Co-authored-by: Clay Kauzlaric <ckauzlaric@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support custom JSON fields for Envoy access logs JSON logging should report invalid headers
4 participants