Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 14 additions & 1 deletion .govulncheck.yaml
Original file line number Diff line number Diff line change
@@ -1 +1,14 @@
ignored-vulnerabilities: []
ignored-vulnerabilities:
# Incorrect parsing of IPv6 host literals in net/url
# Found in: net/url@go1.24.13
# Fixed in: net/url@go1.25.8
- id: GO-2026-4601
info: https://pkg.go.dev/vuln/GO-2026-4601
silence-until: 2026-04-23
# FileInfo can escape from a Root in os
# Found in: os@go1.24.13
# Fixed in: os@go1.25.8
- id: GO-2026-4602
info: https://pkg.go.dev/vuln/GO-2026-4602
silence-until: 2026-04-23

72 changes: 62 additions & 10 deletions test/metrics/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,41 @@
awaitilities := WaitForDeployments(t)

t.Run("host-operator", func(t *testing.T) {

// given
hostAwait := awaitilities.Host()
// host metrics should be available at this point
hostAwait.InitMetrics(t, awaitilities.Member1().ClusterName, awaitilities.Member2().ClusterName)
_, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics")

Check failure on line 38 in test/metrics/metrics_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Define a constant instead of duplicating this literal "/metrics" 3 times.

See more on https://sonarcloud.io/project/issues?id=codeready-toolchain_toolchain-e2e&issues=AZ0gq5ppjb2ksXck6Nkp&open=AZ0gq5ppjb2ksXck6Nkp&pullRequest=1269

Check failure on line 38 in test/metrics/metrics_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Define a constant instead of duplicating this literal "host-operator-metrics-service" 3 times.

See more on https://sonarcloud.io/project/issues?id=codeready-toolchain_toolchain-e2e&issues=AZ0gq5ppjb2ksXck6Nkq&open=AZ0gq5ppjb2ksXck6Nkq&pullRequest=1269
require.NoError(t, err)
Comment on lines +38 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's just a detail, but you technically don't need this because WaitForRouteToBeAvailable is executed as part of SetupRouteForService

Suggested change
_, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics")
require.NoError(t, err)


// when
labels := hostAwait.GetMetricLabels(t, wait.HostOperatorVersionMetric)
t.Run("commit", func(t *testing.T) {
// when
labels := hostAwait.GetMetricLabels(t, hostAwait.MetricsURL, wait.HostOperatorCommitMetric)

// verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label
require.Len(t, labels, 1)
commit := labels[0]["commit"]
assert.Len(t, commit, 7)
// verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label
require.Len(t, labels, 1)
commit := labels[0]["commit"]
assert.Len(t, commit, len("e6a12a442a60dfd86d348a030ad2e789c79184b5")) // example value: 40 characters
})

t.Run("short commit", func(t *testing.T) {
// when
labels := hostAwait.GetMetricLabels(t, hostAwait.MetricsURL, wait.HostOperatorShortCommitMetric)

// verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label
require.Len(t, labels, 1)
commit := labels[0]["commit"]
assert.Len(t, commit, 7)
})

t.Run("version", func(t *testing.T) {
// when
labels := hostAwait.GetMetricLabels(t, hostAwait.MetricsURL, wait.HostOperatorVersionMetric)

// verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label
require.Len(t, labels, 1)
commit := labels[0]["commit"]
assert.Len(t, commit, 7)
})
})

t.Run("member-operators", func(t *testing.T) {
Expand All @@ -56,7 +79,7 @@

// --- member1 ---
// when
labels := member1Await.GetMetricLabels(t, wait.MemberOperatorVersionMetric)
labels := member1Await.GetMetricLabels(t, member1Await.MetricsURL, wait.MemberOperatorVersionMetric)

// verify that the "version" metric exists for the first Member Operator and that it has a non-empty `commit` label
require.Len(t, labels, 1)
Expand All @@ -65,7 +88,7 @@

// --- member2 ---
// when
labels = member2Await.GetMetricLabels(t, wait.MemberOperatorVersionMetric)
labels = member2Await.GetMetricLabels(t, member2Await.MetricsURL, wait.MemberOperatorVersionMetric)

// verify that the "version" metric exists for the second Member Operator and that it has a non-empty `commit` label
require.Len(t, labels, 1)
Expand All @@ -75,6 +98,35 @@
// expect the same version on member1 and member2
assert.Equal(t, commit1, commit2)
})

t.Run("registration-service", func(t *testing.T) {

// given
hostAwait := awaitilities.Host()
_, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics")
require.NoError(t, err)
Comment on lines +102 to +107
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Bug: Wrong service name in route availability check for registration-service metrics.

Line 106 waits for "host-operator-metrics-service" but this test is validating registration-service metrics from hostAwait.RegistrationServiceMetricsURL. The route was set up in init.go as "registration-service-metrics", so the wait should match.

🐛 Proposed fix
 	t.Run("registration-service", func(t *testing.T) {
 
 		// given
 		hostAwait := awaitilities.Host()
-		_, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics")
+		_, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.RegistrationServiceNs, "registration-service-metrics", "/metrics")
 		require.NoError(t, err)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
t.Run("registration-service", func(t *testing.T) {
// given
hostAwait := awaitilities.Host()
_, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics")
require.NoError(t, err)
t.Run("registration-service", func(t *testing.T) {
// given
hostAwait := awaitilities.Host()
_, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.RegistrationServiceNs, "registration-service-metrics", "/metrics")
require.NoError(t, err)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/metrics/metrics_test.go` around lines 102 - 107, The test uses the wrong
service name when waiting for the metrics route: change the call to
hostAwait.WaitForRouteToBeAvailable so it waits for
"registration-service-metrics" (the route created in init.go) instead of
"host-operator-metrics-service"; update the invocation in the
"registration-service" t.Run block that currently references hostAwait and
WaitForRouteToBeAvailable so it matches the route used by
hostAwait.RegistrationServiceMetricsURL.

Comment on lines +106 to +107
Copy link
Collaborator

Choose a reason for hiding this comment

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

the same

Suggested change
_, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics")
require.NoError(t, err)


t.Run("commit", func(t *testing.T) {
// when
labels := hostAwait.GetMetricLabels(t, hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceCommitMetric)

// verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label
require.Len(t, labels, 1)
commit := labels[0]["commit"]
assert.Len(t, commit, len("da3f54634cc65075d51d067a157831d44bf1413e")) // example value: 40 characters
})

t.Run("short commit", func(t *testing.T) {
// when
labels := hostAwait.GetMetricLabels(t, hostAwait.RegistrationServiceMetricsURL, wait.RegistrationServiceShortCommitMetric)

// verify that the "version" metric exists for Host Operator and that it has a non-empty `commit` label
require.Len(t, labels, 1)
commit := labels[0]["commit"]
assert.Len(t, commit, 7)
})
})

}

// TestMetricsWhenUsersManuallyApproved verifies that `UserSignupsApprovedMetric` and `UserSignupsApprovedWithMethodMetric` counters are increased when users are approved
Expand Down
34 changes: 32 additions & 2 deletions testsupport/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/tools/clientcmd/api"
Expand Down Expand Up @@ -205,14 +206,43 @@
require.NoError(t, err, "failed to find proxy metrics service")

// setup host metrics route for metrics verification in tests
hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", "/metrics")
hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", "/metrics",

Check failure on line 209 in testsupport/init.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Define a constant instead of duplicating this literal "/metrics" 3 times.

See more on https://sonarcloud.io/project/issues?id=codeready-toolchain_toolchain-e2e&issues=AZ0gq5uljb2ksXck6Nks&open=AZ0gq5uljb2ksXck6Nks&pullRequest=1269
&routev1.RoutePort{
TargetPort: intstr.FromString("https"),
},
&routev1.TLSConfig{
Termination: routev1.TLSTerminationPassthrough,
},
)
require.NoError(t, err)
initHostAwait.MetricsURL = "https://" + hostMetricsRoute.Status.Ingress[0].Host
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if SetupRouteForService guarantees non-empty Ingress on return
ast-grep --pattern 'func ($_ *Awaitility) SetupRouteForService($$$) ($_, error) {
  $$$
}'

Repository: codeready-toolchain/toolchain-e2e

Length of output: 2439


🏁 Script executed:

# First, let's locate and examine testsupport/init.go
fd init.go testsupport

# Also check the line count to understand file structure
wc -l testsupport/init.go 2>/dev/null || echo "File not found in expected path"

Repository: codeready-toolchain/toolchain-e2e

Length of output: 120


🏁 Script executed:

# Search for files that use SetupRouteForService and access Ingress[0].Host
rg "SetupRouteForService|Ingress\[0\]\.Host" --type-list | head -20

Repository: codeready-toolchain/toolchain-e2e

Length of output: 466


🏁 Script executed:

# Find WaitForRouteToBeAvailable implementation
ast-grep --pattern 'func ($_ *Awaitility) WaitForRouteToBeAvailable($$$) ($_, error) {
  $$$
}'

Repository: codeready-toolchain/toolchain-e2e

Length of output: 4243


🏁 Script executed:

# Examine testsupport/init.go around lines 218, 231, 244
sed -n '210,250p' testsupport/init.go

Repository: codeready-toolchain/toolchain-e2e

Length of output: 1936


🏁 Script executed:

# Also check for the specific string from the review comment snippet
rg "initHostAwait.MetricsURL" testsupport/

Repository: codeready-toolchain/toolchain-e2e

Length of output: 257


🏁 Script executed:

# Find all places accessing .Status.Ingress[0]
rg "Status\.Ingress\[0\]" testsupport/

Repository: codeready-toolchain/toolchain-e2e

Length of output: 772


🏁 Script executed:

# Check all occurrences of accessing Ingress in the codebase
rg "\.Status\.Ingress" --type go -B 2 -A 2 | head -100

Repository: codeready-toolchain/toolchain-e2e

Length of output: 2139


Add defensive checks before accessing route Ingress slice.

Lines 218, 231, and 244 access route.Status.Ingress[0].Host without validating that the slice is non-empty. Although SetupRouteForService internally calls WaitForRouteToBeAvailable which verifies the Ingress slice is populated before returning, the code in init.go relies on this implicit guarantee without a defensive check. Add validation before accessing the index to handle any edge cases:

if len(route.Status.Ingress) == 0 {
    t.Fatalf("route has no ingress status")
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testsupport/init.go` at line 218, The code assumes route.Status.Ingress is
non-empty when assigning initHostAwait.MetricsURL (using
hostMetricsRoute.Status.Ingress[0].Host) and similarly for the Logs and Health
assignments; add defensive checks before indexing each route's Ingress slice
(for hostMetricsRoute, hostLogsRoute, hostHealthRoute) to verify
len(route.Status.Ingress) > 0 and call t.Fatalf with a clear message if empty,
then proceed to read Ingress[0].Host for the URL assignments.

t.Logf("host metrics URL: %s", initHostAwait.MetricsURL)

// setup registration service metrics route for metrics verification in tests
registrationServiceMetricsRoute, err := initHostAwait.SetupRouteForService(t, "registration-service-metrics", "/metrics",
&routev1.RoutePort{
TargetPort: intstr.FromString("regsvc-metrics"),
},
&routev1.TLSConfig{
Termination: routev1.TLSTerminationEdge,
},
)
require.NoError(t, err)
initHostAwait.RegistrationServiceMetricsURL = "https://" + registrationServiceMetricsRoute.Status.Ingress[0].Host
t.Logf("registration service metrics URL: %s", initHostAwait.RegistrationServiceMetricsURL)
Comment on lines +222 to +232
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like the route creation is also done in the tests

route := &routev1.Route{
ObjectMeta: metav1.ObjectMeta{
Namespace: await.Host().Namespace,
Name: "registration-service-metrics",
},
Spec: routev1.RouteSpec{
To: routev1.RouteTargetReference{
Kind: "Service",
Name: "registration-service-metrics",
},
Port: &routev1.RoutePort{
TargetPort: intstr.FromString("regsvc-metrics"),
},
},
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, good catch! I only ran the tests in the test/metrics package so I missed the route creation in test/e2e. Let me refactor that


// setup member metrics route for metrics verification in tests
memberMetricsRoute, err := initMemberAwait.SetupRouteForService(t, "member-operator-metrics-service", "/metrics")
memberMetricsRoute, err := initMemberAwait.SetupRouteForService(t, "member-operator-metrics-service", "/metrics",
&routev1.RoutePort{
TargetPort: intstr.FromString("https"),
},
&routev1.TLSConfig{
Termination: routev1.TLSTerminationPassthrough,
},
)
require.NoError(t, err, "failed while setting up or waiting for the route to the 'member-operator-metrics' service to be available")
initMemberAwait.MetricsURL = "https://" + memberMetricsRoute.Status.Ingress[0].Host
t.Logf("member metrics URL: %s", initMemberAwait.MetricsURL)

// Wait for the webhooks in Member 1 only because we do not deploy webhooks for Member 2
// (we can't deploy the same webhook multiple times on the same cluster)
Expand Down
7 changes: 3 additions & 4 deletions testsupport/metrics/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,16 +129,14 @@ func getBuckets(t dto.MetricType, m *dto.Metric) (*[]*dto.Bucket, error) {

// GetMetricLabels return all labels (indexed by key) for all metrics of the given `family`
func GetMetricLabels(restConfig *rest.Config, baseURL string, family string) ([]map[string]string, error) {
uri := baseURL + "/metrics"
var metrics []byte

client := http.Client{
Timeout: time.Duration(30 * time.Second),
Transport: &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec
},
}
request, err := http.NewRequest("Get", uri, nil)
uri := baseURL + "/metrics"
request, err := http.NewRequest("GET", uri, nil)
if err != nil {
return nil, err
}
Expand All @@ -150,6 +148,7 @@ func GetMetricLabels(restConfig *rest.Config, baseURL string, family string) ([]
defer func() {
_ = resp.Body.Close()
}()
var metrics []byte
metrics, err = io.ReadAll(resp.Body)
if err != nil {
return nil, err
Expand Down
26 changes: 11 additions & 15 deletions testsupport/wait/awaitility.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/rest"
"k8s.io/kubectl/pkg/util/podutils"
Expand Down Expand Up @@ -210,8 +209,8 @@
// SetupRouteForService if needed, creates a route for the given service (with the same namespace/name)
// It waits until the route is available (or returns an error) by first checking the resource status
// and then making a call to the given endpoint
func (a *Awaitility) SetupRouteForService(t *testing.T, serviceName, endpoint string) (routev1.Route, error) {
t.Logf("setting up route for service '%s' with endpoint '%s'", serviceName, endpoint)
func (a *Awaitility) SetupRouteForService(t *testing.T, serviceName string, path string, port *routev1.RoutePort, tlsConfig *routev1.TLSConfig) (routev1.Route, error) {
t.Logf("setting up route for service '%s' with endpoint '%s'", serviceName, path)
service, err := a.WaitForService(t, serviceName)
if err != nil {
return routev1.Route{}, err
Expand All @@ -230,28 +229,24 @@
Name: service.Name,
},
Spec: routev1.RouteSpec{
Port: &routev1.RoutePort{
TargetPort: intstr.FromString("https"),
},
TLS: &routev1.TLSConfig{
Termination: routev1.TLSTerminationPassthrough,
},
To: routev1.RouteTargetReference{
Kind: service.Kind,
Name: service.Name,
},
Port: port,
TLS: tlsConfig,
},
}
if err = a.Client.Create(context.TODO(), &route); err != nil {
return route, err
}
}
return a.WaitForRouteToBeAvailable(t, route.Namespace, route.Name, endpoint)
return a.WaitForRouteToBeAvailable(t, route.Namespace, route.Name, path)
}

// WaitForRouteToBeAvailable waits until the given route is available, ie, it has an Ingress with a host configured
// and the endpoint is reachable (with a `200 OK` status response)
func (a *Awaitility) WaitForRouteToBeAvailable(t *testing.T, ns, name, endpoint string) (routev1.Route, error) {
func (a *Awaitility) WaitForRouteToBeAvailable(t *testing.T, ns, name, path string) (routev1.Route, error) {

Check failure on line 249 in testsupport/wait/awaitility.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 23 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=codeready-toolchain_toolchain-e2e&issues=AZ0gq5tkjb2ksXck6Nkr&open=AZ0gq5tkjb2ksXck6Nkr&pullRequest=1269
t.Logf("waiting for route '%s' in namespace '%s'", name, ns)
route := routev1.Route{}
// retrieve the route for the registration service
Expand Down Expand Up @@ -282,13 +277,13 @@
InsecureSkipVerify: true, // nolint:gosec
},
}
request, err = http.NewRequest("GET", "https://"+route.Status.Ingress[0].Host+endpoint, nil)
request, err = http.NewRequest("GET", "https://"+route.Status.Ingress[0].Host+path, nil)
if err != nil {
return false, err
}
request.Header.Add("Authorization", fmt.Sprintf("Bearer %s", a.RestConfig.BearerToken))
} else {
request, err = http.NewRequest("GET", "http://"+route.Status.Ingress[0].Host+endpoint, nil)
request, err = http.NewRequest("GET", "http://"+route.Status.Ingress[0].Host+path, nil)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -335,8 +330,9 @@

// GetMetricValue gets the value of the metric with the given family and label key-value pair
// fails if the metric with the given labelAndValues does not exist
func (a *Awaitility) GetMetricLabels(t *testing.T, family string) []map[string]string {
labels, err := metrics.GetMetricLabels(a.RestConfig, a.MetricsURL, family)
func (a *Awaitility) GetMetricLabels(t *testing.T, metricsURL, family string) []map[string]string {
t.Logf("getting labels for metric '%s' from '%s'", family, metricsURL)
labels, err := metrics.GetMetricLabels(a.RestConfig, metricsURL, family)
require.NoError(t, err)
return labels
}
Expand Down
23 changes: 15 additions & 8 deletions testsupport/wait/host.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,10 @@ var (
// HostAwaitility the Awaitility for the Host cluster
type HostAwaitility struct {
*Awaitility
RegistrationServiceNs string
RegistrationServiceURL string
APIProxyURL string
RegistrationServiceNs string
RegistrationServiceURL string
RegistrationServiceMetricsURL string
APIProxyURL string
}

// NewHostAwaitility initializes a HostAwaitility
Expand All @@ -81,10 +82,11 @@ func NewHostAwaitility(cfg *rest.Config, cl client.Client, ns string, registrati
// WithRetryOptions returns a new HostAwaitility with the given RetryOptions applied
func (a *HostAwaitility) WithRetryOptions(options ...RetryOption) *HostAwaitility {
return &HostAwaitility{
Awaitility: a.Awaitility.WithRetryOptions(options...),
RegistrationServiceNs: a.RegistrationServiceNs,
RegistrationServiceURL: a.RegistrationServiceURL,
APIProxyURL: a.APIProxyURL,
Awaitility: a.Awaitility.WithRetryOptions(options...),
RegistrationServiceNs: a.RegistrationServiceNs,
RegistrationServiceURL: a.RegistrationServiceURL,
RegistrationServiceMetricsURL: a.RegistrationServiceMetricsURL,
APIProxyURL: a.APIProxyURL,
}
}

Expand All @@ -111,7 +113,12 @@ const (

UsersPerActivationsAndDomainMetric = "sandbox_users_per_activations_and_domain"

HostOperatorVersionMetric = "sandbox_host_operator_version"
HostOperatorVersionMetric = "sandbox_host_operator_version" // DEPRECATED: use HostOperatorCommitMetric instead
HostOperatorCommitMetric = "sandbox_host_operator_commit"
HostOperatorShortCommitMetric = "sandbox_host_operator_short_commit"

RegistrationServiceCommitMetric = "sandbox_registration_service_commit"
RegistrationServiceShortCommitMetric = "sandbox_registration_service_short_commit"

SignupProvisionTimeMetric = "sandbox_user_signup_provision_time"
)
Expand Down
Loading