HYPERFLEET-591, HYPERFLEET-593 - test: automate K8s Resources Check and Adapter Dependency Relationships Workflow Validation#27
Conversation
…nd Adapter Dependency Relationships Workflow Validation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR increases the adapter processing timeout in configs/config.yaml, adds two cluster adapters ( Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Suite as Test Suite (creation.go)
participant Helper as Helper (pkg/helper)
participant Kubectl as kubectl CLI
participant K8sAPI as Kubernetes API
Test->>Suite: trigger cluster creation
Suite->>Helper: request per-adapter verification
Helper->>Kubectl: kubectl get <resource> --selector=<labels> -o json
Kubectl->>K8sAPI: fetch resource
K8sAPI-->>Kubectl: JSON response
Kubectl-->>Helper: JSON output
Helper->>Helper: parse JSON, validate status/labels/annotations
alt cl-deployment depends on cl-job
Helper->>Kubectl: poll Job status (selector)
Kubectl->>K8sAPI: fetch job status
K8sAPI-->>Kubectl: job status (succeeded)
Kubectl-->>Helper: job complete
Helper->>Kubectl: verify Deployment available
Kubectl-->>Helper: deployment available
end
Helper-->>Suite: verification result
Suite-->>Test: test outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-design/testcases/cluster.md (1)
5-7:⚠️ Potential issue | 🟡 MinorUpdate the Table of Contents entry to match the renamed section.
The TOC still points to the old “Workflow Validation” anchor, which will break the link after the heading rename.
🛠️ Proposed fix
-1. [Clusters Resource Type - Workflow Validation](`#test-title-clusters-resource-type---workflow-validation`) +1. [Clusters Resource Type - Basic Workflow Validation](`#test-title-clusters-resource-type---basic-workflow-validation`)Also applies to: 10-10
🤖 Fix all issues with AI agents
In `@e2e/cluster/creation.go`:
- Around line 211-216: The loop over h.Cfg.Adapters.Cluster currently prints a
warning and continues when adapterResourceVerifiers lacks an entry, which hides
missing verifier errors; update the logic in the loop that inspects
adapterResourceVerifiers (using variables adapterName, verifier, exists) to fail
the test immediately instead of skipping: replace the ginkgo.GinkgoWriter.Printf
+ continue with a test-failing call (e.g., ginkgo.Fail or a Gomega Expect/Fail)
that includes the adapterName and clear context so the test run exits fast when
a verifier is missing.
- Around line 240-279: The boolean flag foundInitialState is declared outside
the Eventually poll closure and persists across retries; move its initialization
into the start of the polling closure so each poll sets foundInitialState =
false before calling h.Client.GetClusterStatuses and iterating adapters, then
keep the existing checks (HasAdapterCondition calls and
g.Expect(foundInitialState)) unchanged so the assertion reflects the adapter
presence/state for the current poll rather than stale state from prior retries.
In `@pkg/helper/k8s.go`:
- Around line 130-137: The current logic casts jobList["items"] to items and
unconditionally picks items[0], which is nondeterministic when multiple
resources match; update the code that handles items (the jobList/items/jobData
logic) to check for multiple matches and return a clear error if len(items) > 1
(or alternatively filter by a provided name/explicit identifier) instead of
silently using the first entry, and apply the same change to the other identical
items-handling block that currently casts and uses the first element.
| var clusterName string | ||
|
|
||
| ginkgo.BeforeEach(func() { | ||
| ginkgo.BeforeEach(func(ctx context.Context) { |
There was a problem hiding this comment.
How to delete the resources created by adapters? Like namespace, job ... Add it with cluster deletion action after MVP?
I think we can add a common function for every added adapter. Then it can call it directly after test.
There was a problem hiding this comment.
For MVP and E2E testing, we delete the namespace named clusterid in AfterEach. All created Kubernetes resources are scoped under this namespace, so deleting it removes all cluster-related resources.
For post-MVP, the created Kubernetes resources should be cleaned up when a DELETE cluster request is submitted. If the deletion fails, it indicates an issue that CLM needs to resolve.
There was a problem hiding this comment.
Do you add the AfterEach? I didn't find it
There was a problem hiding this comment.
These three test cases use the same BeforeEach and AfterEach
86254860
force-pushed
the
HYPERFLEET-591
branch
from
1e9517d to
eec6695
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
configs/config.yaml (1)
48-55:⚠️ Potential issue | 🟡 MinorUpdate the recommendation comment to match the new default.
The comment still says “Recommended: 1m” while the default is now 2m, which can mislead readers.📝 Suggested doc alignment
- # Recommended: 1m + # Recommended: 2m
🧹 Nitpick comments (1)
pkg/helper/validation.go (1)
59-79: Normalize adapter names before conversion.
Trimming whitespace (and short-circuiting empty values) avoids surprising condition types when config entries include accidental spaces.♻️ Proposed tweak
func (h *Helper) AdapterNameToClusterConditionType(adapterName string) string { + adapterName = strings.TrimSpace(adapterName) + if adapterName == "" { + return "" + } // Split adapter name by "-" (e.g., "cl-namespace" -> ["cl", "namespace"]) parts := strings.Split(adapterName, "-")
86254860
force-pushed
the
HYPERFLEET-591
branch
from
2bdc46c to
979a934
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/helper/validation.go`:
- Around line 11-30: ValidateK8sName currently only checks total length and
k8sNameRegex but doesn't enforce the DNS-1123 per-label (dot-separated)
63-character limit; update ValidateK8sName to split the name on '.' (use
strings.Split) and iterate labels to ensure each label is non-empty and
len(label) <= 63, returning an error like "label %q exceeds maximum length of 63
characters" if violated; keep the existing regex and overall 253-char check, and
reference k8sNameRegex and ValidateK8sName when making this change.
- Around line 32-41: The ValidateLabelSelector function currently uses
labelSelectorRegex and rejects many valid Kubernetes selectors; replace the
regex check with the Kubernetes parser by calling labels.Parse(selector) from
k8s.io/apimachinery/pkg/labels inside ValidateLabelSelector and return any parse
error (and remove labelSelectorRegex since it becomes unused). Keep the
empty-string guard, but instead of labelSelectorRegex.MatchString(selector) use
labels.Parse(selector) to validate formats like set-based, existence, != and
DNS-prefixed keys.
🧹 Nitpick comments (1)
pkg/helper/validation.go (1)
93-114: Guard against empty adapter names.
An empty or all--adapter name currently returns"Successful", which can silently mis-map conditions. Consider a defensive early return.🛡️ Defensive guard
func (h *Helper) AdapterNameToClusterConditionType(adapterName string) string { + if strings.Trim(adapterName, "-") == "" { + return "" + } // Split adapter name by "-" (e.g., "cl-namespace" -> ["cl", "namespace"]) parts := strings.Split(adapterName, "-")
| var ( | ||
| // k8sNameRegex validates Kubernetes resource names (DNS-1123 subdomain format) | ||
| k8sNameRegex = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`) | ||
| // labelSelectorRegex validates Kubernetes label selector format (key=value,key=value) | ||
| labelSelectorRegex = regexp.MustCompile(`^[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?=[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?(,[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?=[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?)*$`) | ||
| ) | ||
|
|
||
| // ValidateK8sName validates a Kubernetes resource name (namespace, resource name, etc.) | ||
| func ValidateK8sName(name string) error { | ||
| if name == "" { | ||
| return fmt.Errorf("name cannot be empty") | ||
| } | ||
| if len(name) > 253 { | ||
| return fmt.Errorf("name %q exceeds maximum length of 253 characters", name) | ||
| } | ||
| if !k8sNameRegex.MatchString(name) { | ||
| return fmt.Errorf("name %q is invalid: must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", name) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "validation.go" -path "*/helper/*"Repository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 105
🏁 Script executed:
cat -n pkg/helper/validation.go | head -40Repository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 1972
🌐 Web query:
DNS-1123 subdomain format Kubernetes resource naming label length limit
💡 Result:
-
DNS-1123 label (often called “DNS label” in Kubernetes)
- Max length: 63 characters
- Format: lowercase alphanumeric (
a-z0-9) or-; must start with a letter; must end with an alphanumeric. [1]
-
DNS-1123 subdomain (used by most
metadata.namefields)- Max length: 253 characters total
- Format: one or more DNS labels separated by
.; allowed chars are lowercase alphanumeric,-, and.; must start and end with an alphanumeric. [1] - Implied per-label limit: each label inside the subdomain is still limited to 63 characters (DNS rule). [3]
Sources: Kubernetes naming rules [1]; RFC 1123 label and total-length limits [3].
Enforce per-label 63-character limit for DNS-1123 compliance.
The current regex and validation accept names with labels longer than 63 characters, which violates DNS-1123 subdomain format. Each dot-separated label must not exceed 63 characters in addition to the total 253-character limit. This allows invalid resource names that the Kubernetes API will reject.
Add a per-label length check:
Fix to enforce per-label length limit
func ValidateK8sName(name string) error {
if name == "" {
return fmt.Errorf("name cannot be empty")
}
if len(name) > 253 {
return fmt.Errorf("name %q exceeds maximum length of 253 characters", name)
}
+ for _, label := range strings.Split(name, ".") {
+ if len(label) > 63 {
+ return fmt.Errorf("name %q has a label longer than 63 characters", name)
+ }
+ }
if !k8sNameRegex.MatchString(name) {
return fmt.Errorf("name %q is invalid: must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", name)
}
return nil
}📝 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.
| var ( | |
| // k8sNameRegex validates Kubernetes resource names (DNS-1123 subdomain format) | |
| k8sNameRegex = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`) | |
| // labelSelectorRegex validates Kubernetes label selector format (key=value,key=value) | |
| labelSelectorRegex = regexp.MustCompile(`^[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?=[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?(,[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?=[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?)*$`) | |
| ) | |
| // ValidateK8sName validates a Kubernetes resource name (namespace, resource name, etc.) | |
| func ValidateK8sName(name string) error { | |
| if name == "" { | |
| return fmt.Errorf("name cannot be empty") | |
| } | |
| if len(name) > 253 { | |
| return fmt.Errorf("name %q exceeds maximum length of 253 characters", name) | |
| } | |
| if !k8sNameRegex.MatchString(name) { | |
| return fmt.Errorf("name %q is invalid: must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", name) | |
| } | |
| return nil | |
| } | |
| var ( | |
| // k8sNameRegex validates Kubernetes resource names (DNS-1123 subdomain format) | |
| k8sNameRegex = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`) | |
| // labelSelectorRegex validates Kubernetes label selector format (key=value,key=value) | |
| labelSelectorRegex = regexp.MustCompile(`^[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?=[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?(,[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?=[a-zA-Z0-9]([-a-zA-Z0-9_.]*[a-zA-Z0-9])?)*$`) | |
| ) | |
| // ValidateK8sName validates a Kubernetes resource name (namespace, resource name, etc.) | |
| func ValidateK8sName(name string) error { | |
| if name == "" { | |
| return fmt.Errorf("name cannot be empty") | |
| } | |
| if len(name) > 253 { | |
| return fmt.Errorf("name %q exceeds maximum length of 253 characters", name) | |
| } | |
| for _, label := range strings.Split(name, ".") { | |
| if len(label) > 63 { | |
| return fmt.Errorf("name %q has a label longer than 63 characters", name) | |
| } | |
| } | |
| if !k8sNameRegex.MatchString(name) { | |
| return fmt.Errorf("name %q is invalid: must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", name) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@pkg/helper/validation.go` around lines 11 - 30, ValidateK8sName currently
only checks total length and k8sNameRegex but doesn't enforce the DNS-1123
per-label (dot-separated) 63-character limit; update ValidateK8sName to split
the name on '.' (use strings.Split) and iterate labels to ensure each label is
non-empty and len(label) <= 63, returning an error like "label %q exceeds
maximum length of 63 characters" if violated; keep the existing regex and
overall 253-char check, and reference k8sNameRegex and ValidateK8sName when
making this change.
| // ValidateLabelSelector validates a Kubernetes label selector string | ||
| func ValidateLabelSelector(selector string) error { | ||
| if selector == "" { | ||
| return fmt.Errorf("label selector cannot be empty") | ||
| } | ||
| if !labelSelectorRegex.MatchString(selector) { | ||
| return fmt.Errorf("label selector %q is invalid: must be in format 'key=value' or 'key1=value1,key2=value2'", selector) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/helper/validation.go | head -60Repository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 2940
🏁 Script executed:
cat go.mod | grep -i "k8s.io"Repository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 61
🏁 Script executed:
rg "ValidateLabelSelector" --type goRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 416
🏁 Script executed:
ls -la go.modRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 134
🏁 Script executed:
cat -n pkg/helper/k8s.go | grep -A 5 -B 5 "ValidateLabelSelector"Repository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 1037
🏁 Script executed:
cat go.modRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 1859
🌐 Web query:
Kubernetes label selector validation formats equality set-based existence
💡 Result:
Kubernetes label selector formats (and what gets validated)
1) Equality-based (string selectors: kubectl -l ..., ?labelSelector=...)
Operators: =, == (synonyms), !=
Examples: environment=production, tier!=frontend
Note: key!=value also matches objects without that key. Multiple requirements are comma-separated and are ANDed. No OR operator. [1]
2) Set-based (string selectors)
Operators / forms:
- In:
key in (v1,v2) - NotIn:
key notin (v1,v2)(also matches objects without that key) - Exists:
key - DoesNotExist:
!key
Multiple requirements are comma-separated and ANDed. [1]
3) Existence (explicitly)
- Exists:
partition - DoesNotExist:
!partition[1]
Validation rules you typically hit
A) Label key / value format validation (applies to labels used in selectors too)
- Key: optional
prefix/+ requirednamename: ≤ 63 chars; begins/ends with alphanumeric; may contain-,_,., alphanumerics in betweenprefix(if present): DNS subdomain (≤ 253 chars) followed by/[1]
- Value: ≤ 63 chars (can be empty); if non-empty must begin/end with alphanumeric; may contain
-,_,., alphanumerics in between [1]
B) API LabelSelector / matchExpressions validation (YAML/JSON selectors)
In structured selectors (spec.selector.matchExpressions):
operator∈{In, NotIn, Exists, DoesNotExist}valuesrules:- In / NotIn →
valuesmust be non-empty - Exists / DoesNotExist →
valuesmust be empty [2]
- In / NotIn →
Sources: Kubernetes “Labels and Selectors” docs [1]; Kubernetes API “LabelSelector” definition (matchExpressions rules) [2].
Citations: [1] (kubernetes.io) [2] (kubernetes.io)
Citations:
- 1: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/
- 2: https://kubernetes.io/zh/docs/reference/kubernetes-api/common-definitions/label-selector/?utm_source=openai
Replace regex validation with Kubernetes' official label selector parser.
The current regex only supports equality-based selectors (key=value), rejecting valid formats including set-based selectors (key in (v1,v2), key notin (v1,v2)), existence selectors (key, !key), inequality operators (!=), and DNS-prefixed keys (prefix/key). Use labels.Parse() from k8s.io/apimachinery/pkg/labels for correct validation.
✅ Use Kubernetes' parser for validation
+import (
+ ...
+ "k8s.io/apimachinery/pkg/labels"
+)
+
func ValidateLabelSelector(selector string) error {
if selector == "" {
return fmt.Errorf("label selector cannot be empty")
}
- if !labelSelectorRegex.MatchString(selector) {
- return fmt.Errorf("label selector %q is invalid: must be in format 'key=value' or 'key1=value1,key2=value2'", selector)
- }
+ if _, err := labels.Parse(selector); err != nil {
+ return fmt.Errorf("label selector %q is invalid: %w", selector, err)
+ }
return nil
}If you switch to labels.Parse, labelSelectorRegex becomes unnecessary and can be removed.
🤖 Prompt for AI Agents
In `@pkg/helper/validation.go` around lines 32 - 41, The ValidateLabelSelector
function currently uses labelSelectorRegex and rejects many valid Kubernetes
selectors; replace the regex check with the Kubernetes parser by calling
labels.Parse(selector) from k8s.io/apimachinery/pkg/labels inside
ValidateLabelSelector and return any parse error (and remove labelSelectorRegex
since it becomes unused). Keep the empty-string guard, but instead of
labelSelectorRegex.MatchString(selector) use labels.Parse(selector) to validate
formats like set-based, existence, != and DNS-prefixed keys.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yasun1 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit c41f56a
into
openshift-hyperfleet:main
3 participants
Summary
Testing
Summary by CodeRabbit
New Features
Configuration
Tests
Quality