Skip to content

Adds Access loggers to admin interface#15461

Merged
mattklein123 merged 33 commits intoenvoyproxy:mainfrom
davinci26:addAccessLogToAdmin
Mar 25, 2021
Merged

Adds Access loggers to admin interface#15461
mattklein123 merged 33 commits intoenvoyproxy:mainfrom
davinci26:addAccessLogToAdmin

Conversation

@davinci26
Copy link
Copy Markdown
Member

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com
Commit Message:

Splitting the work of #15202 into two pieces for easier reviewing.

Additional Description:
Risk Level: Medium
Testing: Pending
Docs Changes: Pending
Release Notes: Pending
Platform Specific Features: N/A

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15461 was opened by davinci26.

see: more, trace.

@davinci26
Copy link
Copy Markdown
Member Author

First commit to check the CI. I have some pending items still, I will tag when its ready for a review

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Sotiris Nanopoulos added 2 commits March 12, 2021 11:40
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Copy link
Copy Markdown
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 api

Copy link
Copy Markdown
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.

This is great. Thanks for working on this. A few high level comments to get started.

/wait

Copy link
Copy Markdown
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 remaining comment. Thanks so much for working on this.

/wait

Comment on lines +7 to +10
access_log:
- name: envoy.access_loggers.stdoutput
typed_config:
"@type": type.googleapis.com/envoy.extensions.access_loggers.stdoutput.v3.StdoutputAccessLog
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that you made it not required, how about just removing access logging entirely from all the examples? It will make them cleaner?

Signed-off-by: davinci26 <sotirisnan@gmail.com>
Signed-off-by: davinci26 <sotirisnan@gmail.com>
Signed-off-by: davinci26 <sotirisnan@gmail.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Copy link
Copy Markdown
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 looks great. Sorry one more test comment. Do you have test coverage of the deprecated path? I think you want a DEPRECATED_FEATURE_TEST somewhere? Thank you!

/wait

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

I think you want a DEPRECATED_FEATURE_TEST somewhere?

I did not know about this, first time contributing an API change. I added two tests to validate what happens:

  1. When the deprecated property is used on its own
  2. When the new property and the deprecated property are used at the same time.

Copy link
Copy Markdown
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.

Thank you!

@mattklein123 mattklein123 merged commit 0564107 into envoyproxy:main Mar 25, 2021
codefromthecrypt added a commit to tetratelabs-attic/getenvoy.io that referenced this pull request Jun 10, 2021
`/dev/stdout` wasn't semantically ported to windows so will crash.
Also, `access_log_path` is ignored in envoy 1.19

See tetratelabs/func-e#273
See envoyproxy/envoy#15461

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit to tetratelabs-attic/getenvoy.io that referenced this pull request Jun 10, 2021
`/dev/stdout` wasn't semantically ported to windows so will crash.
Also, `access_log_path` is ignored in envoy 1.19

See tetratelabs/func-e#273
See envoyproxy/envoy#15461

Co-authored-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants