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

Switch to JSON formatted access logs #44

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Conversation

dinofizz
Copy link
Contributor

@dinofizz dinofizz commented May 8, 2024

I'd like to emit access logs in JSON format so as to be able to improve their visibility in our OpenSearch index. In addition to this I made a number of changes:

  • I did a general update of direct dependencies (apologies for the vendor folder changes)
  • I removed references and flags related to legacy Kubernetes versions in our manifest/CRD generation
  • I've switched to using the latest version of controller-gen
  • I'm using docker buildx build with a platform flag specifying the architecture to be linux amd64
  • I've extended some of the RBAC roles to align with changes in our internal "deployment" repo

Example pod logs showing admin logs and cluster access logs (admin logs left as default - but we can move these to JSON to if we want):

image

JSON access log (prettified):

{
  "response_code": 0,
  "upstream_cluster": "example-www_TCP_443",
  "bytes_received": 1294,
  "bytes_sent": 5999,
  "downstream_local_address": "10.129.191.27:443",
  "duration": 445,
  "upstream_host": "93.184.215.14:443",
  "downstream_remote_address": "10.129.187.50:48990",
  "response_flags": "-",
  "start_time": "2025-01-08T14:12:19.041Z",
  "upstream_local_address": "10.129.191.27:37028"
}

@dinofizz dinofizz force-pushed the dino@improve-logging branch 2 times, most recently from c81fc70 to 5e28e72 Compare May 8, 2024 16:47
@dinofizz dinofizz force-pushed the dino@improve-logging branch from 5e28e72 to 9ee3d3c Compare May 8, 2024 16:48
@dinofizz dinofizz changed the title dino@improve logging Switch to JSON formatted access logs Jan 8, 2025
@dinofizz dinofizz marked this pull request as ready for review January 8, 2025 13:38
Makefile Outdated
@@ -70,7 +68,7 @@ ifeq (, $(shell which controller-gen))
CONTROLLER_GEN_TMP_DIR=$$(mktemp -d) ;\
cd $$CONTROLLER_GEN_TMP_DIR ;\
go mod init tmp ;\
go get sigs.k8s.io/controller-tools/cmd/controller-gen@v0.4.0 ;\
go get sigs.k8s.io/controller-tools/cmd/controller-gen@latest ;\
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should pin this to a specific hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pinned this with this commit b8a2e9c

domgoodwin
domgoodwin previously approved these changes Jan 8, 2025
@@ -35,6 +36,34 @@ import (

// +kubebuilder:rbac:namespace=egress-operator-system,groups=core,resources=configmaps,verbs=get;list;watch;create;patch

var (
logFields = &structpb.Struct{
Copy link
Contributor

@domgoodwin domgoodwin Jan 8, 2025

Choose a reason for hiding this comment

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

Do these roughly match our service mesh ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not exactly.

This is what we have for the service mesh

		"start_time":                     "%START_TIME%",
		"total_duration_ms":              "%DURATION%",
		"bytes_received":                 "%BYTES_RECEIVED%",
		"bytes_sent":                     "%BYTES_SENT%",
		"response_flags":                 "%RESPONSE_FLAGS%",
		"destination_host":               "%UPSTREAM_HOST%",
		"destination_service":            "%UPSTREAM_CLUSTER%",
		"source_address":                 "%DOWNSTREAM_REMOTE_ADDRESS%",
		"connection_id":                  "%CONNECTION_ID%",
		"connection_termination_details": "%CONNECTION_TERMINATION_DETAILS%",
		"requested_sni":                  "%REQUESTED_SERVER_NAME%",
		"hostname":                       "%HOSTNAME%",

vs what's in this PR:

          authority: '%REQ(:AUTHORITY)%'
          bytes_received: '%BYTES_RECEIVED%'
          bytes_sent: '%BYTES_SENT%'
          connection_termination_details: '%CONNECTION_TERMINATION_DETAILS%'
          downstream_local_address: '%DOWNSTREAM_LOCAL_ADDRESS%'
          downstream_remote_address: '%DOWNSTREAM_REMOTE_ADDRESS%'
          duration: '%DURATION%'
          method: '%REQ(:METHOD)%'
          path: '%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%'
          protocol: '%PROTOCOL%'
          requested_server_name: '%REQUESTED_SERVER_NAME%'
          response_code: '%RESPONSE_CODE%'
          response_code_details: '%RESPONSE_CODE_DETAILS%'
          response_flags: '%RESPONSE_FLAGS%'
          start_time: '%START_TIME%'
          upstream_cluster: '%UPSTREAM_CLUSTER%'
          upstream_host: '%UPSTREAM_HOST%'
          upstream_local_address: '%UPSTREAM_LOCAL_ADDRESS%'
          upstream_service_time: '%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)%'
          upstream_transport_failure_reason: '%UPSTREAM_TRANSPORT_FAILURE_REASON%'
          user_agent: '%REQ(USER-AGENT)%'

awnumar
awnumar previously approved these changes Jan 8, 2025
@dinofizz dinofizz requested review from awnumar and cottand January 9, 2025 08:18
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.

3 participants