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

Stats: RDS_ROUTE_CONFIG tag extraction #32976

Closed
kb000 opened this issue Mar 19, 2024 · 5 comments · Fixed by #33734
Closed

Stats: RDS_ROUTE_CONFIG tag extraction #32976

kb000 opened this issue Mar 19, 2024 · 5 comments · Fixed by #33734

Comments

@kb000
Copy link
Contributor

kb000 commented Mar 19, 2024

Title: Some route_config_names fail tag extraction

Description:
RDS routes with legal names not expected by the <ROUTE_CONFIG_NAME> re2 regex fail tag extraction into prometheus stats.
The use case I've run across includes / in route names, though from the xds spec I'm not sure there's actually any restriction on route naming. You can configure the HttpConnectionManager with "route_config": {"name": "🚀🙏:)", ... } and it's accepted.

Repro steps:
Configure an xds route named ingressgateway/api-tess-io-1-gw-443 on http connection manager ingress_http

Admin and Stats Output:
/stats:

http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.config_reload: 1
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.config_reload_time_ms: 1710814314824
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.control_plane.connected_state: 1
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.control_plane.pending_requests: 0
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.control_plane.rate_limit_enforced: 0
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.init_fetch_timeout: 0
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.update_attempt: 13
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.update_empty: 0
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.update_failure: 0
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.update_rejected: 0
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.update_success: 12
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.update_time: 1710851152272
http.ingress_http.rds.ingressgateway/api-tess-io-1-gw-443.version: 17181926294437511708

/stats/prometheus:

# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_config_reload counter
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_config_reload{envoy_http_conn_manager_prefix="ingress_http"} 1
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_control_plane_rate_limit_enforced counter
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_control_plane_rate_limit_enforced{envoy_http_conn_manager_prefix="ingress_http"} 0
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_init_fetch_timeout counter
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_init_fetch_timeout{envoy_http_conn_manager_prefix="ingress_http"} 0
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_attempt counter
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_attempt{envoy_http_conn_manager_prefix="ingress_http"} 13
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_empty counter
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_empty{envoy_http_conn_manager_prefix="ingress_http"} 0
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_failure counter
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_failure{envoy_http_conn_manager_prefix="ingress_http"} 0
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_rejected counter
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_rejected{envoy_http_conn_manager_prefix="ingress_http"} 0
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_success counter
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_success{envoy_http_conn_manager_prefix="ingress_http"} 12
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_config_reload_time_ms gauge
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_config_reload_time_ms{envoy_http_conn_manager_prefix="ingress_http"} 1710814314824
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_control_plane_connected_state gauge
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_control_plane_connected_state{envoy_http_conn_manager_prefix="ingress_http"} 1
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_control_plane_pending_requests gauge
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_control_plane_pending_requests{envoy_http_conn_manager_prefix="ingress_http"} 0
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_time gauge
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_update_time{envoy_http_conn_manager_prefix="ingress_http"} 1710851152272
# TYPE envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_version gauge
envoy_http_rds_ingressgateway_api_tess_io_1_gw_443_version{envoy_http_conn_manager_prefix="ingress_http"} 17181926294437511708

Prometheus stats should be:

envoy_http_rds_config_reload{envoy_http_conn_manager_prefix="ingress_http",envoy_rds_route_config="ingressgateway/api-tess-io-1-gw-443"} 1
etc.
@kb000 kb000 added bug triage Issue requires triage labels Mar 19, 2024
@kb000
Copy link
Contributor Author

kb000 commented Mar 19, 2024

Looks to be introduced by #14519

@htuch
Copy link
Member

htuch commented Mar 21, 2024

@rojkov @jmarantz

@htuch htuch added area/stats and removed triage Issue requires triage labels Mar 21, 2024
@kb000
Copy link
Contributor Author

kb000 commented Mar 22, 2024

If we decide what the behavior should be (how much to allow in this regex), I can make the change.

Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 21, 2024
@kb000
Copy link
Contributor Author

kb000 commented Apr 22, 2024

Ok I'll make a PR and we can talk about what characters to allow in comments.

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 22, 2024
jmarantz pushed a commit that referenced this issue Apr 30, 2024
Commit Message: Extract route names with forward-slash.
Additional Description: RDS protocol allows almost any route name, and stats are lightly sanitized before emitting to statsd and prometheus endpoints. This change slightly relaxes the capture group for RDS route names in stats, instead of opening the floodgates to everything, because YAGNI.
Risk Level: Low
Testing: Updated unit test
Docs Changes: None: No restrictions on the route name in RDS were documented before (anything goes, as far as I can tell), so there's nothing to change there.
The minimal documentation on route name in RDS stats don't mention allowing, disallowing, or special handling for anything but ":".
Release Notes: N/A
Platform Specific Features: None
Fixes: #32976

Signed-off-by: Kevin Burek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants