generated from kedacore/github-template
-
Notifications
You must be signed in to change notification settings - Fork 141
feat: expose and secure controller-runtime metrics #1369
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
Open
khauser
wants to merge
10
commits into
kedacore:main
Choose a base branch
from
khauser:copilot/update-http-addon-rbac-permissions-again
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
9738f6a
Initial plan
Copilot 36ffd11
Update operator metrics to use port 8443 with authentication and auth…
Copilot 45f6783
Fix code review comments - update service targetPort and test improve…
Copilot 550c5ab
chore: Add changelog entry for version 0.12.0
khauser 4b142ba
fix: operator metrics test does work
khauser 5874b67
fix: copilot issues and stabilize zipkin
khauser 6410fc3
fix: metrics secure and auth should be configurable
khauser e0ba971
fix: metrics secure certifacte folder configurable
khauser 6a20d9a
feat: use port 8080 defaultwise for operator metrics
khauser e3f4364
fix: some review issues
khauser File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -362,3 +362,4 @@ admin/Cargo.lock | |
| *.csr | ||
| *.srl | ||
| *.ext | ||
| *.test | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRole | ||
| metadata: | ||
| name: operator-metrics-reader | ||
| rules: | ||
| - nonResourceURLs: | ||
| - "/metrics" | ||
| verbs: | ||
| - get |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: operator-metrics | ||
| spec: | ||
| type: ClusterIP | ||
| ports: | ||
| - name: metrics-http | ||
| protocol: TCP | ||
| port: 8080 | ||
| targetPort: 8080 | ||
| - name: metrics-https | ||
| protocol: TCP | ||
| port: 8443 | ||
| targetPort: 8443 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| //go:build e2e | ||
| // +build e2e | ||
|
|
||
| package operator_metrics_test | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
|
|
||
| . "github.com/kedacore/http-add-on/tests/helper" | ||
| ) | ||
|
|
||
| const ( | ||
| testName = "operator-metrics-test" | ||
| ) | ||
|
|
||
| var ( | ||
| testNamespace = fmt.Sprintf("%s-ns", testName) | ||
| clientName = fmt.Sprintf("%s-client", testName) | ||
| kedaOperatorMetricsURL = "http://keda-add-ons-http-operator-metrics.keda:8080/metrics" | ||
| operatorPodSelector = "app.kubernetes.io/instance=operator" | ||
| ) | ||
|
|
||
| type templateData struct { | ||
| TestNamespace string | ||
| ClientName string | ||
| } | ||
|
|
||
| const ( | ||
| clientTemplate = ` | ||
| apiVersion: v1 | ||
| kind: Pod | ||
| metadata: | ||
| name: {{.ClientName}} | ||
| namespace: {{.TestNamespace}} | ||
| spec: | ||
| serviceAccountName: {{.ClientName}} | ||
| containers: | ||
| - name: {{.ClientName}} | ||
| image: curlimages/curl | ||
| command: | ||
| - sh | ||
| - -c | ||
| - "exec tail -f /dev/null"` | ||
|
|
||
| serviceAccountTemplate = ` | ||
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: {{.ClientName}} | ||
| namespace: {{.TestNamespace}}` | ||
|
|
||
| clusterRoleBindingTemplate = ` | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| metadata: | ||
| name: {{.ClientName}}-metrics-reader | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: keda-add-ons-http-operator-metrics-reader | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{.ClientName}} | ||
| namespace: {{.TestNamespace}}` | ||
| ) | ||
|
|
||
| func TestOperatorMetrics(t *testing.T) { | ||
| // setup | ||
| t.Log("--- setting up ---") | ||
| // Create kubernetes resources | ||
| kc := GetKubernetesClient(t) | ||
| data, templates := getTemplateData() | ||
| CreateKubernetesResources(t, kc, testNamespace, data, templates) | ||
|
|
||
| // Wait for client pod to be ready | ||
| assert.True(t, WaitForAllPodRunningInNamespace(t, kc, testNamespace, 6, 10), | ||
| "client pod should be running") | ||
|
|
||
| t.Log("--- testing operator metrics endpoint ---") | ||
|
|
||
| // Test 1: HTTPS endpoint should be accessible (will fail cert validation but should return metrics) | ||
| t.Log("Test 1: Verify HTTP endpoint is available") | ||
| testHTTPEndpoint(t) | ||
|
|
||
| // Test 2: Verify metrics are returned | ||
| t.Log("Test 2: Verify metrics content") | ||
| testMetricsContent(t) | ||
|
|
||
| // cleanup | ||
| DeleteKubernetesResources(t, testNamespace, data, templates) | ||
| } | ||
|
|
||
| func testHTTPEndpoint(t *testing.T) { | ||
| cmd := fmt.Sprintf("curl --max-time 10 %s", kedaOperatorMetricsURL) | ||
| out, errOut, err := ExecCommandOnSpecificPod(t, clientName, testNamespace, cmd) | ||
|
|
||
| // The endpoint should return something (even if authentication fails, it should respond) | ||
| assert.True(t, err == nil || strings.Contains(errOut, "Forbidden") || strings.Contains(out, "Forbidden"), | ||
| "HTTP endpoint should respond (either with metrics or authentication error)") | ||
| } | ||
|
|
||
| func testMetricsContent(t *testing.T) { | ||
| // Access metrics from the client pod using the service endpoint | ||
| // The client pod uses a ServiceAccount with the operator-metrics-reader ClusterRole | ||
| // This allows it to access the metrics endpoint with proper RBAC permissions | ||
|
|
||
| // Get the ServiceAccount token to authenticate | ||
| cmd := fmt.Sprintf("curl -H \"Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)\" --max-time 10 %s", kedaOperatorMetricsURL) | ||
| out, errOut, err := ExecCommandOnSpecificPod(t, clientName, testNamespace, cmd) | ||
|
|
||
| if err != nil { | ||
| t.Logf("Metrics content test - Output: %s, Error output: %s, Error: %v", out, errOut, err) | ||
| } | ||
|
|
||
| // Verify that metrics are returned in Prometheus format | ||
| assert.NoError(t, err, "should be able to access metrics from client pod with RBAC permissions. Output: %s, Error: %s", out, errOut) | ||
| assert.True(t, strings.Contains(out, "# HELP") || strings.Contains(out, "# TYPE"), | ||
| "metrics should contain Prometheus format. Output: %s", out) | ||
| } | ||
|
|
||
| func getTemplateData() (templateData, []Template) { | ||
| return templateData{ | ||
| TestNamespace: testNamespace, | ||
| ClientName: clientName, | ||
| }, []Template{ | ||
| {Name: "serviceAccountTemplate", Config: serviceAccountTemplate}, | ||
| {Name: "clusterRoleBindingTemplate", Config: clusterRoleBindingTemplate}, | ||
| {Name: "clientTemplate", Config: clientTemplate}, | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a separate port here and in config/operator/metrics_service.yaml ? And if, might it make sense to include
metricsin its name if it's for metrics?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end one of this ports would point to nowhere. In my understanding this config folder is just for integration/end2end testing. Then it wouldn't be a problem. I adjusted the names.