Skip to content

🐛 test/e2e/metrics_test.go: discover operator-controller namespace and improve logging #1547

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 7, 2025
Merged

🐛 test/e2e/metrics_test.go: discover operator-controller namespace and improve logging #1547

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 30 additions & 10 deletions test/e2e/metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package e2e

import (
"bytes"
"io"
"os/exec"
"testing"

Expand All @@ -20,11 +21,10 @@ import (
// 6. Cleans up all resources created during the test, such as the ClusterRoleBinding and curl pod.
func TestOperatorControllerMetricsExportedEndpoint(t *testing.T) {
var (
token string
curlPod = "curl-metrics"
namespace = "olmv1-system"
client = ""
clients = []string{"kubectl", "oc"}
token string
curlPod = "curl-metrics"
client = ""
clients = []string{"kubectl", "oc"}
)

t.Log("Looking for k8s client")
Expand All @@ -37,15 +37,25 @@ func TestOperatorControllerMetricsExportedEndpoint(t *testing.T) {
}
}
if client == "" {
t.Skip("k8s client not found, skipping test")
t.Fatal("k8s client not found")
}
t.Logf("Using %q as k8s client", client)

t.Log("Determining operator-controller namespace")
cmd := exec.Command(client, "get", "pods", "--all-namespaces", "--selector=control-plane=operator-controller-controller-manager", "--output=jsonpath={.items[0].metadata.namespace}")
output, err := cmd.CombinedOutput()
require.NoError(t, err, "Error creating determining operator-controller namespace: %s", string(output))
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there are multiple outputs (namespaces) found?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I thought about that briefly. Ultimately if there are multiple pods that match the label that exist in different namespaces, we'll pick the first one.

If that's the "correct" namespace (where the SA and metrics service/endpoint) exist, the test will pass.
If it's not the right namespace, the test will fail, likely because the token generation fails because the SA doesn't exist, but if the SA exists in the "incorrect" namespace, it could still fail if there's no service there.

TL;DR: the test will fail if we choose an "incorrect" namespace.

Copy link
Contributor

@tmshort tmshort Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

wouldn’t the output be along the lines of "namespace1\nnamespace2" which would mess up any use of output? I don’t think you’re just picking “one”.
Admittedly, this will totally mess up your --serviceaccount parameter, but we should error out early.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because the output specifically selects the namespace of the first item in the returned list

--output=jsonpath={.items[0].metadata.namespace}

Copy link
Contributor

@tmshort tmshort Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, yes, this was something you had also changed downstream... So you're going for the first one... hmm... But now I'm wondering how you did that downstream without this commit?
Edit: Ah... drop commit...

namespace := string(output)
if namespace == "" {
t.Fatal("No operator-controller namespace found")
}
t.Logf("Using %q as operator-controller namespace", namespace)

t.Log("Creating ClusterRoleBinding for operator controller metrics")
cmd := exec.Command(client, "create", "clusterrolebinding", "operator-controller-metrics-binding",
cmd = exec.Command(client, "create", "clusterrolebinding", "operator-controller-metrics-binding",
"--clusterrole=operator-controller-metrics-reader",
"--serviceaccount="+namespace+":operator-controller-controller-manager")
output, err := cmd.CombinedOutput()
output, err = cmd.CombinedOutput()
require.NoError(t, err, "Error creating ClusterRoleBinding: %s", string(output))

defer func() {
Expand All @@ -55,8 +65,8 @@ func TestOperatorControllerMetricsExportedEndpoint(t *testing.T) {

t.Log("Generating ServiceAccount token")
tokenCmd := exec.Command(client, "create", "token", "operator-controller-controller-manager", "-n", namespace)
tokenOutput, err := tokenCmd.Output()
require.NoError(t, err, "Error creating token: %s", string(tokenOutput))
tokenOutput, tokenCombinedOutput, err := stdoutAndCombined(tokenCmd)
require.NoError(t, err, "Error creating token: %s", string(tokenCombinedOutput))
token = string(bytes.TrimSpace(tokenOutput))

t.Log("Creating curl pod to validate the metrics endpoint")
Expand Down Expand Up @@ -105,3 +115,13 @@ func TestOperatorControllerMetricsExportedEndpoint(t *testing.T) {
require.NoError(t, err, "Error calling metrics endpoint: %s", string(output))
require.Contains(t, string(output), "200 OK", "Metrics endpoint did not return 200 OK")
}

func stdoutAndCombined(cmd *exec.Cmd) ([]byte, []byte, error) {
var outOnly bytes.Buffer
var outAndErr bytes.Buffer
allWriter := io.MultiWriter(&outOnly, &outAndErr)
cmd.Stderr = &outAndErr
cmd.Stdout = allWriter
err := cmd.Run()
return outOnly.Bytes(), outAndErr.Bytes(), err
}
Loading