Add new fields to ThanosQuerierConfig#2835
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @danielmellado! Some important instructions when contributing to openshift/api: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded three optional fields to ThanosQuerierConfig: 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
Bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test verify-deps |
| // enableRequestLogging controls whether all incoming HTTP requests are logged by Thanos Querier. | ||
| // enableRequestLogging is optional. | ||
| // Valid values are "Enabled" and "Disabled". | ||
| // When set to "Enabled", every request received by Thanos Querier is logged with method, path, and response status. | ||
| // When set to "Disabled", request logging is turned off. | ||
| // When omitted, this means no opinion and the platform is left to choose a reasonable default, that is subject to change over time. | ||
| // The current default value is "Disabled". | ||
| // +optional | ||
| EnableRequestLogging ThanosQuerierToggle `json:"enableRequestLogging,omitempty"` |
There was a problem hiding this comment.
Looking at https://thanos.io/tip/thanos/logging.md/ it looks like HTTP request logging has the ability to be configured beyond just being turned on/off.
Do we anticipate needing to support the finer tuning that the thanos configuration options seem to allow?
There was a problem hiding this comment.
Thanks for the pointer to the Thanos logging docs. You're right that Thanos supports richer request logging config (per-protocol levels, log_start/log_end decisions, endpoint allowlists).
However, looking at how CMO currently uses this field, it's a simple on/off toggle — when enabled, it generates a hardcoded --request.logging-config block with log_start: false, log_end: true for both HTTP and gRPC, using the top-level logLevel for verbosity. No fine-grained options are exposed to users today.
For now, I've renamed this to requestLogging with values LogAll / None to match the current CMO behavior while avoiding the ambiguous "Enabled"/"Disabled" terminology. If we need finer-grained control in the future, we can evolve this field into a structured type (e.g., with per-protocol log levels and decision options) without breaking the existing API.
| // enableCORS controls whether Thanos Querier sets CORS headers allowing access from any origin. | ||
| // enableCORS is optional. | ||
| // Valid values are "Enabled" and "Disabled". | ||
| // When set to "Enabled", CORS headers are added to responses, allowing cross-origin requests from any domain. | ||
| // When set to "Disabled", no CORS headers are added. | ||
| // When omitted, this means no opinion and the platform is left to choose a reasonable default, that is subject to change over time. | ||
| // The current default value is "Disabled". | ||
| // +optional | ||
| EnableCORS ThanosQuerierToggle `json:"enableCORS,omitempty"` |
There was a problem hiding this comment.
We try to avoid the usage of the terms "enabled" and "disabled" where possible because they tend to be just as ambiguous as a true/false boolean value.
Also, enableCORS: Enabled reads a bit weird.
What do you think of making this something like crossOriginRequestHeaderPolicy: AllowAll | None ? This looks to satisfy the existing options allowed by the Thanos flag but gives you the flexibility to evolve to support additional configurations in the future if Thanos decides to support additional configuration in the future.
There was a problem hiding this comment.
Agree there... enableCORS: Enabled does read awkwardly, and I agree we should avoid "Enabled"/"Disabled" values.
I've renamed this to crossOriginRequestPolicy with enum values AllowAll / DenyAll, closely following your suggestion. This maps directly to what Thanos and CMO support today (--web.disable-cors flag), and leaves room for future values like AllowSpecific if Thanos ever supports per-origin configuration.
Port ThanosQuerierConfig fields from the CMO ConfigMap API to the ClusterMonitoring CRD. LogLevel reuses the existing LogLevel enum (Error, Warn, Info, Debug). EnableRequestLogging and EnableCORS use a new ThanosQuerierToggle string enum (Enabled, Disabled) following the Kubernetes API convention of avoiding *bool fields. Signed-off-by: Daniel Mellado <dmellado@fedoraproject.org>
danielmellado
force-pushed
the
mon_crd_thanosquerier_fields
branch
from
2d8d4d8 to
b6ab5b9
Compare
|
@danielmellado: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Port ThanosQuerierConfig fields from the CMO ConfigMap API to the
ClusterMonitoring CRD. LogLevel reuses the existing LogLevel enum
(Error, Warn, Info, Debug). EnableRequestLogging and EnableCORS use
a new ThanosQuerierToggle string enum (Enabled, Disabled) following
the Kubernetes API convention of avoiding *bool fields.
Signed-off-by: Daniel Mellado dmellado@fedoraproject.org