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

[WIP] Add access logging support #10506

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

npolshakova
Copy link
Contributor

@npolshakova npolshakova commented Jan 24, 2025

Description

#10507

API changes

Code changes

CI changes

Docs changes

Context

Interesting decisions

Testing steps

Notes for reviewers

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

LogName string `json:"logName,omitempty"`

// The static cluster defined in bootstrap config to route to
StaticClusterName string `json:"staticClusterName,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

why does it need to be static?

@nfuden
Copy link
Contributor

nfuden commented Jan 28, 2025

any chance we can do something like https://github.com/solo-io/gloo/blob/main/projects/gloo/pkg/plugins/als/converter.go#L56-L64 to validate the contents of the access logging itself?

@npolshakova
Copy link
Contributor Author

any chance we can do something like https://github.com/solo-io/gloo/blob/main/projects/gloo/pkg/plugins/als/converter.go#L56-L64 to validate the contents of the access logging itself?

I'm hesitant to include those initially for a couple of reasons:

  1. envoy go control plane doesn't have those defined anywhere so we'd need to hard code the "unuseful" values
  2. It's unclear how Gloo identified the "unuseful" commands from the envoy list. The original issue from when this code was introduced (Support listener access logging #8438) didn't seem to have it in the requirements. A bad entry shouldn't be the end of the world as long as the syntax is enclosed correctly.

Most of the validation should be happening as part of the kubebuilder validation lines. I'm not sure we want to validate on the content of access logs, just the configuration.

@npolshakova npolshakova force-pushed the add-access-logging-support branch from 7bf1610 to 9827e53 Compare January 30, 2025 14:27
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