Re: [PR] [helm] added httproute config for management endpoints [polaris]
adutra merged PR #3969: URL: https://github.com/apache/polaris/pull/3969 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
adutra commented on code in PR #3969: URL: https://github.com/apache/polaris/pull/3969#discussion_r2913322530 ## helm/polaris/values.yaml: ## @@ -472,6 +472,9 @@ httproute: # @section -- HTTPRoute hosts: - chart-example.local + # -- Enable to expose the health endpoints on your httproute. This is required if you want to use the management service for health checks and metrics scraping external to the cluster. Review Comment: ```suggestion # -- Enable to expose the management endpoints on your httproute. This is required if you want to use the management service for health checks and metrics scraping external to the cluster. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
adutra commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2913317465
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
Review Comment:
OK, I thought it was more expressive if we declared the `/q` prefix always,
but I won't die on this hill 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
cccs-cat001 commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2913294599
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
Review Comment:
I don't think that's really necessary. a /q request going to the regular
service results in a 404 anyways
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
adutra commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2913247308
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
Review Comment:
Maybe a safer approach is to just expose a boolean to enable the feature. If
it's true then `/q` -> management port, otherwise `/q` requests are dropped:
```yaml
rules:
# Always intercept /q — either route to mgmt service or drop
- matches:
- path:
type: PathPrefix
value: "/q"
{{- if .Values.httproute.exposeManagementEndpoints }}
backendRefs:
- name: {{ include "polaris.fullnameWithSuffix" (list . "mgmt") }}
port: {{ get (first .Values.managementService.ports) "port" }}
{{- end }}
# Catch-all: everything else goes to the main service
- backendRefs:
- name: {{ $fullName }}
port: {{ get (first .Values.service.ports) "port" }}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
adutra commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2913221346
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
Review Comment:
That's what I was thinking too 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
cccs-cat001 commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2913207587
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
+ backendRefs:
+- name: {{ include "polaris.fullnameWithSuffix" (list . "mgmt") }}
+ port: {{ get (first .Values.managementService.ports) "port" }}
+ filters:
+- type: URLRewrite
+ urlRewrite:
+path:
+ type: ReplacePrefixMatch
+ replacePrefixMatch: "/"
Review Comment:
from the sounds of our use case they'll want to scrape the metrics too
eventually
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
cccs-cat001 commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2913210765
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
Review Comment:
right... I could just direct all `/q` endpoints to that service...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
adutra commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2913088911
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
+ backendRefs:
+- name: {{ include "polaris.fullnameWithSuffix" (list . "mgmt") }}
+ port: {{ get (first .Values.managementService.ports) "port" }}
+ filters:
+- type: URLRewrite
+ urlRewrite:
+path:
+ type: ReplacePrefixMatch
+ replacePrefixMatch: "/"
Review Comment:
But do we want to expose metrics to the outside world? Do you think there
could be a use case for that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
cccs-cat001 commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2913063990
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
+ backendRefs:
+- name: {{ include "polaris.fullnameWithSuffix" (list . "mgmt") }}
+ port: {{ get (first .Values.managementService.ports) "port" }}
+ filters:
+- type: URLRewrite
+ urlRewrite:
+path:
+ type: ReplacePrefixMatch
+ replacePrefixMatch: "/"
Review Comment:
updated the description to include metrics
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
cccs-cat001 commented on code in PR #3969: URL: https://github.com/apache/polaris/pull/3969#discussion_r2913050409 ## helm/polaris/values.yaml: ## @@ -472,6 +472,8 @@ httproute: # @section -- HTTPRoute hosts: - chart-example.local + + exposeManagementEndpoints: false Review Comment: Ah! That's why the docs are being uncooperative! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
adutra commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2913004344
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
+ backendRefs:
+- name: {{ include "polaris.fullnameWithSuffix" (list . "mgmt") }}
+ port: {{ get (first .Values.managementService.ports) "port" }}
+ filters:
+- type: URLRewrite
+ urlRewrite:
+path:
+ type: ReplacePrefixMatch
+ replacePrefixMatch: "/"
Review Comment:
If we want just healthchecks, maybe something like this is better:
```yaml
- matches:
- path:
type: PathPrefix
value: "/q/health"
backendRefs:
- name: {{ include "polaris.fullnameWithSuffix" (list . "mgmt") }}
port: {{ get (first .Values.managementService.ports) "port" }}
filters:
- type: URLRewrite
urlRewrite:
path:
type: ReplacePrefixMatch
replacePrefixMatch: "/q/health"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
adutra commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2913007116
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
Review Comment:
And actually, do we need a new prefix at all?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
adutra commented on code in PR #3969:
URL: https://github.com/apache/polaris/pull/3969#discussion_r2912959645
##
helm/polaris/values.yaml:
##
@@ -472,6 +472,8 @@ httproute:
# @section -- HTTPRoute
hosts:
- chart-example.local
+
+ exposeManagementEndpoints: false
Review Comment:
You are missing a description and a section declaration here:
```yaml
# -- description here
# @section -- HTTPRoute
exposeManagementEndpoints: false
```
Then please regenerate the docs and the schema by running `make helm`.
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
+ backendRefs:
+- name: {{ include "polaris.fullnameWithSuffix" (list . "mgmt") }}
+ port: {{ get (first .Values.managementService.ports) "port" }}
+ filters:
+- type: URLRewrite
+ urlRewrite:
+path:
+ type: ReplacePrefixMatch
+ replacePrefixMatch: "/"
Review Comment:
Note: this is also exposing metrics under `/q/metrics`, but the PR
description mentions only "liveness/readiness/health".
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
+- matches:
+ - path:
+ type: PathPrefix
+ value: "/mgmt"
Review Comment:
Should we make this prefix configurable?
##
helm/polaris/templates/httproute.yaml:
##
@@ -41,8 +41,21 @@ spec:
sectionName: {{ .Values.httproute.sectionName }}
{{- end }}
rules:
-# We don't specify a matches block here, so the default is a prefix path
match on "/" (match every HTTP request)
-# The backend (Service) to send matching requests to
+{{- if .Values.httproute.exposeManagementEndpoints }}
Review Comment:
Can you please add unit tests for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [helm] added httproute config for management endpoints [polaris]
cccs-cat001 commented on PR #3969: URL: https://github.com/apache/polaris/pull/3969#issuecomment-4032691219 Yeah... I'm not seeing why it keeps wanting to put this new config under `Other Values` when I run helm-doc-generate... any ideas? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
