-
Notifications
You must be signed in to change notification settings - Fork 360
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
feat: implement egctl x access logs #1887
Conversation
Signed-off-by: sh2 <shawnhxh@outlook.com>
Signed-off-by: sh2 <shawnhxh@outlook.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1887 +/- ##
==========================================
+ Coverage 65.24% 65.34% +0.10%
==========================================
Files 86 87 +1
Lines 12480 13226 +746
==========================================
+ Hits 8142 8642 +500
- Misses 3821 4045 +224
- Partials 517 539 +22 ☔ View full report in Codecov by Sentry. |
internal/cmd/egctl/accesslogs.go
Outdated
} | ||
} | ||
|
||
func serveAccessLogServer(ctx context.Context, server *grpc.Server, addr string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im guessing you are running this in the client machine where egctl
is being run
it may not be possible for to create a grpc conn between the server and envoy
maybe the easiest way is to scrape pod logs of envoy using kubectl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the basic idea of this PR is to implement an access-log server which receives access-logs from envoy as its endpoint.
we can create a connection between server and envoy if we apply the envoy-patch-policy successfully (for example, like internal/cmd/egctl/testdata/accesslogs/test-access-logs.yaml
in this PR).
so the operations for egctl x access-logs
will be: start the access-log server locally, apply the envoy-patch-policy for envoy, and access-log server will start to receive access logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the basic implementation of this subcmd has done, just left some doc and unit test.
PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @shawnh2 lets assume im running this cmd from my macbook into a cluster that is running in GKE.
if you are running this access logs server locally on your Mac, how can you expect the envoy running in GKE to connect back ? routing rules will not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this cmd works if you are running this access logs server in the same place that runs the cluster (for example also run this cmd in GKE), but won't work if you are running this cmd from your local macbook (there's no routing rules for sure).
imo, this cmd should work in both scenario, but this PR only implement the first.
do you think the first scenario is necessary? or should we only focus on the second for now?
Signed-off-by: sh2 <shawnhxh@outlook.com>
Signed-off-by: sh2 <shawnhxh@outlook.com>
Signed-off-by: sh2 <shawnhxh@outlook.com>
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions! |
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions! |
Closing this one, due to staleness and wrong implementation direction. |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1812