Skip to content

Conversation

@gangwgr
Copy link
Contributor

@gangwgr gangwgr commented Jan 30, 2026

Fixing oc client caes for microshift and metal ipv6 profliles

@coderabbitai
Copy link

coderabbitai bot commented Jan 30, 2026

Walkthrough

Adds comprehensive end-to-end test infrastructure for OpenShift CLI, including extensive test suites with multiple orchestrated scenarios, test utility functions and types supporting CLI command execution and Kubernetes operations, a fixture management system for embedded test data, and numerous YAML manifests defining test resources such as custom resource definitions, pods, deployments, and cluster configurations.

Changes

Cohort / File(s) Summary
E2E Test Suites
test/e2e/cli.go
Adds two test suites ([sig-cli] Workloads and [sig-cli] Additional Tests) with extensive e2e scenarios covering pod creation, image import, quota handling, debug workflows, release extraction, multi-arch mirroring, CRD validation, namespace operations, and various CLI debugging features. Includes orchestration of dynamic manifest manipulation, cross-OS compatibility checks, and asynchronous state validation.
Test Utilities and Helpers
test/e2e/util.go
Introduces comprehensive test scaffolding including CLI abstraction layer (NewCLI, CLICommand), project setup helpers, platform/architecture detection, node/pod management utilities, deployment and replica set helpers, CRD waiting functions, networking checks, and numerous domain-specific types (ArchitectureType, PlatformType, ControlplaneInfo, etc.) supporting OpenShift/Kubernetes e2e operations.
Test Fixtures Management
test/testdata/fixtures.go
Adds fixture management utility with embedded FS support, providing FixturePath for resolving test data, GetFixtureDir for temp directory access, CleanupFixtures for cleanup, and ListFixtures for enumerating available embedded fixtures.
Custom Resource Definitions
test/testdata/oc_cli/case72217/crd-*.yaml, test/testdata/oc_cli/case72217/cr-*.yaml
Introduces three CRD definitions (Cattoy72217, CronTab72217, Customtask72217) with openAPI schemas and three corresponding custom resource instances for CRD-driven test scenarios.
Pod and Deployment Configurations
test/testdata/oc_cli/debugpod_48681.yaml, test/testdata/oc_cli/deploymentconfig_with_quota.yaml, test/testdata/oc_cli/initContainer*.yaml
Adds template and manifest definitions for debug pods, deployment configurations with resource quotas, and pods with init containers for testing debugging and initialization workflows.
Cluster Resource Configuration
test/testdata/oc_cli/clusterresource_for_user.yaml, test/testdata/oc_cli/idmsFile64921.yaml
Introduces ClusterResourceQuota configuration for user-scoped quotas and ImageDigestMirrorSet manifest for image mirroring test scenarios.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@gangwgr
Copy link
Contributor Author

gangwgr commented Jan 30, 2026

/payload 4.22 nightly blocking

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 30, 2026

@gangwgr: trigger 14 job(s) of type blocking for the nightly release of OCP 4.22

  • periodic-ci-openshift-release-master-ci-4.22-e2e-aws-upgrade-ovn-single-node
  • periodic-ci-openshift-release-master-nightly-4.22-e2e-aws-ovn-upgrade-fips
  • periodic-ci-openshift-release-master-ci-4.22-e2e-azure-ovn-upgrade
  • periodic-ci-openshift-release-master-ci-4.22-upgrade-from-stable-4.21-e2e-gcp-ovn-rt-upgrade
  • periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-aws-ovn-conformance
  • periodic-ci-openshift-release-master-nightly-4.22-e2e-aws-ovn-serial-1of2
  • periodic-ci-openshift-release-master-nightly-4.22-e2e-aws-ovn-serial-2of2
  • periodic-ci-openshift-release-master-ci-4.22-e2e-aws-ovn-techpreview
  • periodic-ci-openshift-release-master-ci-4.22-e2e-aws-ovn-techpreview-serial-1of3
  • periodic-ci-openshift-release-master-ci-4.22-e2e-aws-ovn-techpreview-serial-2of3
  • periodic-ci-openshift-release-master-ci-4.22-e2e-aws-ovn-techpreview-serial-3of3
  • periodic-ci-openshift-release-master-nightly-4.22-e2e-aws-ovn-upgrade-fips-no-nat-instance
  • periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-ovn-ipv4
  • periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-ovn-ipv6

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ac0a49c0-fdc7-11f0-88f6-772b4a411e87-0

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 30, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gangwgr
Once this PR has been reviewed and has the lgtm label, please assign atiratree for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gangwgr gangwgr changed the title Fixing oc client caes for microshift and metal ipv6 profliles Fixing oc client cases for microshift and metal ipv6 profliles Jan 30, 2026
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🤖 Fix all issues with AI agents
In `@test/e2e/cli.go`:
- Around line 546-553: The code incorrectly treats the variable output as a file
path by running exec.Command("bash", "-c", "cat "+output+" | awk '{print $1}'");
instead, treat output (the byte/string from oc get events) as the data to parse:
remove the exec.Command call and compute result by splitting string(output) into
lines, extracting the first whitespace-separated field of each non-empty line
(e.g., using strings.Split and strings.Fields), joining or scanning those first
fields into a single result string, then run regexp.MatchString("unknown",
result) as before; update references to output and result accordingly.

In `@test/e2e/util.go`:
- Around line 847-853: The function getShouldPruneRSFromCreateTime can panic
when keepNum > totalCompletedRsListNum; add a bounds check to compute a safe end
index (e.g., end := totalCompletedRsListNum - keepNum; if end < 0 { end = 0 })
before slicing, then slice using rsList := totalCompletedRsList[0:end], and only
call sort.Strings and log when end > 0; return the resulting rsList (which will
be an empty slice when keepNum >= totalCompletedRsListNum).
- Around line 913-930: getLatestPayload currently calls http.Get without a
timeout and defers res.Body.Close() after ioutil.ReadAll, risking hangs and
leaking the response body on read errors; replace the call with an http.Client
that has a sensible Timeout (e.g., few seconds) and check err before using res,
then immediately defer res.Body.Close() right after verifying res != nil and err
== nil, and keep the existing error checks for ioutil.ReadAll/json.Unmarshal;
update references inside getLatestPayload accordingly.

In `@test/testdata/oc_cli/case72217/cr-custom-72217.yaml`:
- Around line 1-6: The CR includes an undefined field spec.test which will be
pruned because the CRD crd-customtask-72217.yaml only defines spec.cronSpec,
spec.image, and spec.replicas; update the CR (cr-custom-72217.yaml) to only use
the defined fields or modify the CRD (crd-customtask-72217.yaml) to add
spec.test to the structural schema (or enable preserveUnknownFields) so the
field is accepted—also apply the same fix for cr-cat-72217.yaml and
cr-cron-72217.yaml where spec.test is present.

In `@test/testdata/oc_cli/initContainer.yaml`:
- Around line 8-20: Add a securityContext to both the initContainer "wait" and
the main container "hello-pod" to satisfy restricted PodSecurity/SCC rules: set
allowPrivilegeEscalation: false, ensure runAsNonRoot: true (and a non-root
runAsUser if needed), and enable readOnlyRootFilesystem where appropriate;
update the initContainers and containers entries to include these
securityContext fields so the pod is accepted under restricted policies and
matches other fixtures like debugpod_48681.yaml.
🧹 Nitpick comments (7)
test/testdata/fixtures.go (1)

117-128: ListFixtures silently ignores WalkDir errors.

If fs.WalkDir encounters an error, it's returned but never checked or logged after the function completes. This could hide issues during test debugging.

♻️ Suggested fix
 func ListFixtures() []string {
 	var fixtures []string
-	fs.WalkDir(embeddedFixtures, ".", func(path string, d fs.DirEntry, err error) error {
+	err := fs.WalkDir(embeddedFixtures, ".", func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
 			return err
 		}
 		if !d.IsDir() {
 			fixtures = append(fixtures, path)
 		}
 		return nil
 	})
+	if err != nil {
+		panic(fmt.Sprintf("failed to list fixtures: %v", err))
+	}
 	return fixtures
 }
test/e2e/cli.go (1)

484-508: Unconventional assertion pattern may cause confusing test failures.

Using o.Expect(...).To(o.BeTrue()) inside if conditions is unusual. If the expectation fails, Gomega will fail the test immediately regardless of the if logic, making the control flow misleading.

♻️ Suggested refactor
-		if o.Expect(strings.Contains(architectureStr, "x86_64")).To(o.BeTrue()) {
-			if o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue()) {
+		if strings.Contains(architectureStr, "x86_64") {
+			o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue(), "Expected amd64 arch for x86_64 platform")
 				e2e.Logf("Found the expected Arch amd64")
-			} else {
-				e2e.Failf("Failed to find the expected Arch for mirrored image")
-			}
-		} else if o.Expect(strings.Contains(architectureStr, "aarch64")).To(o.BeTrue()) {
+		} else if strings.Contains(architectureStr, "aarch64") {
+			o.Expect(strings.Contains(imageInfo, "arm64")).To(o.BeTrue(), "Expected arm64 arch for aarch64 platform")
-			if o.Expect(strings.Contains(imageInfo, "arm64")).To(o.BeTrue()) {
 				e2e.Logf("Found the expected Arch aarch64")
-			} else {
-				e2e.Failf("Failed to find the expected Arch for mirrored image")
-			}
 		}
 		// ... similar pattern for other architectures
test/e2e/util.go (5)

11-11: Replace deprecated io/ioutil package.

io/ioutil has been deprecated since Go 1.16. The functions are now available in io and os packages. This affects multiple usages in this file (lines 918, 1058, 1087, 1151, 1324).

Suggested replacements
-	"io/ioutil"

Then update usages:

  • ioutil.ReadAllio.ReadAll
  • ioutil.ReadFileos.ReadFile
  • ioutil.ReadDiros.ReadDir
  • ioutil.WriteFileos.WriteFile

622-630: Remove duplicate getRandomString function.

This function duplicates GetRandomString defined at line 363. Use the exported version instead to avoid maintaining duplicate code.

Suggested fix
-func getRandomString() string {
-	chars := "abcdefghijklmnopqrstuvwxyz0123456789"
-	seed := rand.New(rand.NewSource(time.Now().UnixNano()))
-	buffer := make([]byte, 8)
-	for index := range buffer {
-		buffer[index] = chars[seed.Intn(len(chars))]
-	}
-	return string(buffer)
-}

Replace any calls to getRandomString() with GetRandomString().


477-488: Consider migrating from deprecated wait.Poll to wait.PollUntilContextTimeout.

wait.Poll is deprecated and doesn't support context cancellation. This pattern appears throughout the file. Line 933 already uses the newer wait.PollUntilContextTimeout correctly - consider applying the same pattern to other poll usages for consistency and proper cancellation support.

Example migration
-	err := wait.Poll(5*time.Second, 5*time.Minute, func() (bool, error) {
+	err := wait.PollUntilContextTimeout(context.Background(), 5*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) {
		output, err := c.AsAdmin().WithoutNamespace().Run("get").Args("pod", podName, "-n", namespace, "-o=jsonpath={.status.conditions[?(@.type=='Ready')].status}").Output()

1047-1049: Simplify file reading - avoid unnecessary shell command.

Using exec.Command("bash", "-c", "cat ...") to read a file is unnecessarily complex when os.ReadFile can be used directly.

Suggested fix
 	oauthFilePath := dirName + "/.dockerconfigjson"
-	secretContent, err := exec.Command("bash", "-c", fmt.Sprintf("cat %v", oauthFilePath)).Output()
+	secretContent, err := os.ReadFile(oauthFilePath)
 	o.Expect(err).NotTo(o.HaveOccurred())

1322-1343: Temp file is never cleaned up.

The temp file created at line 1323 is never deleted, which can cause temp directory to fill up during long test runs. Consider returning a cleanup function or using os.CreateTemp with proper cleanup.

Suggested approach

Either document that callers must clean up the returned file path, or use a pattern like:

// At the call site, ensure cleanup:
tmpFile, err := applyResourceFromTemplate48681(oc, args...)
if err == nil {
    defer os.Remove(tmpFile)
}

Or modify the function to accept a cleanup callback.

Comment on lines +546 to +553
if match, _ := regexp.MatchString("No resources found", string(output)); match {
e2e.Logf("No events in project: %v", projectN)
} else {
result, _ := exec.Command("bash", "-c", "cat "+output+" | awk '{print $1}'").Output()
if match, _ := regexp.MatchString("unknown", string(result)); match {
e2e.Failf("Does not show timestamp as expected: %v", result)
}
}
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: cat command used on event output string instead of file path.

The variable output contains the actual events text from oc get events, not a file path. The cat command will fail or produce incorrect results.

🐛 Suggested fix
 			if match, _ := regexp.MatchString("No resources found", string(output)); match {
 				e2e.Logf("No events in project: %v", projectN)
 			} else {
-				result, _ := exec.Command("bash", "-c", "cat "+output+" | awk '{print $1}'").Output()
+				// Parse the output directly instead of using cat
+				lines := strings.Split(output, "\n")
+				for _, line := range lines {
+					fields := strings.Fields(line)
+					if len(fields) > 0 && fields[0] == "unknown" {
+						e2e.Failf("Does not show timestamp as expected: %v", line)
+					}
+				}
-				if match, _ := regexp.MatchString("unknown", string(result)); match {
-					e2e.Failf("Does not show timestamp as expected: %v", result)
-				}
 			}
📝 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
if match, _ := regexp.MatchString("No resources found", string(output)); match {
e2e.Logf("No events in project: %v", projectN)
} else {
result, _ := exec.Command("bash", "-c", "cat "+output+" | awk '{print $1}'").Output()
if match, _ := regexp.MatchString("unknown", string(result)); match {
e2e.Failf("Does not show timestamp as expected: %v", result)
}
}
if match, _ := regexp.MatchString("No resources found", string(output)); match {
e2e.Logf("No events in project: %v", projectN)
} else {
// Parse the output directly instead of using cat
lines := strings.Split(output, "\n")
for _, line := range lines {
fields := strings.Fields(line)
if len(fields) > 0 && fields[0] == "unknown" {
e2e.Failf("Does not show timestamp as expected: %v", line)
}
}
}
🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 546 - 553, The code incorrectly treats the
variable output as a file path by running exec.Command("bash", "-c", "cat
"+output+" | awk '{print $1}'"); instead, treat output (the byte/string from oc
get events) as the data to parse: remove the exec.Command call and compute
result by splitting string(output) into lines, extracting the first
whitespace-separated field of each non-empty line (e.g., using strings.Split and
strings.Fields), joining or scanning those first fields into a single result
string, then run regexp.MatchString("unknown", result) as before; update
references to output and result accordingly.

Comment on lines +847 to +853
func getShouldPruneRSFromCreateTime(totalCompletedRsList []string, totalCompletedRsListNum int, keepNum int) []string {
rsList := totalCompletedRsList[0:(totalCompletedRsListNum - keepNum)]
sort.Strings(rsList)
e2e.Logf("rsList %v:", rsList)
return rsList

}
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

Missing bounds check can cause panic.

If keepNum exceeds totalCompletedRsListNum, the slice operation [0:(totalCompletedRsListNum - keepNum)] will panic with a negative index.

Suggested fix
 func getShouldPruneRSFromCreateTime(totalCompletedRsList []string, totalCompletedRsListNum int, keepNum int) []string {
+	if keepNum >= totalCompletedRsListNum {
+		return []string{}
+	}
 	rsList := totalCompletedRsList[0:(totalCompletedRsListNum - keepNum)]
📝 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
func getShouldPruneRSFromCreateTime(totalCompletedRsList []string, totalCompletedRsListNum int, keepNum int) []string {
rsList := totalCompletedRsList[0:(totalCompletedRsListNum - keepNum)]
sort.Strings(rsList)
e2e.Logf("rsList %v:", rsList)
return rsList
}
func getShouldPruneRSFromCreateTime(totalCompletedRsList []string, totalCompletedRsListNum int, keepNum int) []string {
if keepNum >= totalCompletedRsListNum {
return []string{}
}
rsList := totalCompletedRsList[0:(totalCompletedRsListNum - keepNum)]
sort.Strings(rsList)
e2e.Logf("rsList %v:", rsList)
return rsList
}
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 847 - 853, The function
getShouldPruneRSFromCreateTime can panic when keepNum > totalCompletedRsListNum;
add a bounds check to compute a safe end index (e.g., end :=
totalCompletedRsListNum - keepNum; if end < 0 { end = 0 }) before slicing, then
slice using rsList := totalCompletedRsList[0:end], and only call sort.Strings
and log when end > 0; return the resulting rsList (which will be an empty slice
when keepNum >= totalCompletedRsListNum).

Comment on lines +913 to +930
func getLatestPayload(url string) string {
res, err := http.Get(url)
if err != nil {
e2e.Failf("unable to get http with error: %v", err)
}
body, err := ioutil.ReadAll(res.Body)
defer res.Body.Close()
if err != nil {
e2e.Failf("unable to parse the http result with error: %v", err)
}

var data map[string]interface{}
if err := json.Unmarshal(body, &data); err != nil {
e2e.Failf("unable to parse JSON with error: %v", err)
}
pullSpec, _ := data["pullSpec"].(string)
return pullSpec
}
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

HTTP request lacks timeout and has potential resource leak.

  1. http.Get has no timeout and can hang tests indefinitely if the server doesn't respond.
  2. defer res.Body.Close() is placed after ioutil.ReadAll, so if ReadAll fails, the body won't be closed.
Suggested fix
 func getLatestPayload(url string) string {
-	res, err := http.Get(url)
+	client := &http.Client{Timeout: 30 * time.Second}
+	res, err := client.Get(url)
 	if err != nil {
 		e2e.Failf("unable to get http with error: %v", err)
 	}
-	body, err := ioutil.ReadAll(res.Body)
 	defer res.Body.Close()
+	body, err := io.ReadAll(res.Body)
 	if err != nil {
 		e2e.Failf("unable to parse the http result with error: %v", 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
func getLatestPayload(url string) string {
res, err := http.Get(url)
if err != nil {
e2e.Failf("unable to get http with error: %v", err)
}
body, err := ioutil.ReadAll(res.Body)
defer res.Body.Close()
if err != nil {
e2e.Failf("unable to parse the http result with error: %v", err)
}
var data map[string]interface{}
if err := json.Unmarshal(body, &data); err != nil {
e2e.Failf("unable to parse JSON with error: %v", err)
}
pullSpec, _ := data["pullSpec"].(string)
return pullSpec
}
func getLatestPayload(url string) string {
client := &http.Client{Timeout: 30 * time.Second}
res, err := client.Get(url)
if err != nil {
e2e.Failf("unable to get http with error: %v", err)
}
defer res.Body.Close()
body, err := io.ReadAll(res.Body)
if err != nil {
e2e.Failf("unable to parse the http result with error: %v", err)
}
var data map[string]interface{}
if err := json.Unmarshal(body, &data); err != nil {
e2e.Failf("unable to parse JSON with error: %v", err)
}
pullSpec, _ := data["pullSpec"].(string)
return pullSpec
}
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 913 - 930, getLatestPayload currently calls
http.Get without a timeout and defers res.Body.Close() after ioutil.ReadAll,
risking hangs and leaking the response body on read errors; replace the call
with an http.Client that has a sensible Timeout (e.g., few seconds) and check
err before using res, then immediately defer res.Body.Close() right after
verifying res != nil and err == nil, and keep the existing error checks for
ioutil.ReadAll/json.Unmarshal; update references inside getLatestPayload
accordingly.

Comment on lines +1 to +6
apiVersion: "example.com/v1"
kind: Customtask72217
metadata:
name: my-new-customtask-object
spec:
test: test
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

🧩 Analysis chain

🏁 Script executed:

find test/testdata/oc_cli/case72217 -type f -name "*.yaml" -o -name "*.yml" | head -20

Repository: openshift/oc

Length of output: 370


🏁 Script executed:

cat -n test/testdata/oc_cli/case72217/cr-custom-72217.yaml

Repository: openshift/oc

Length of output: 210


🏁 Script executed:

cat -n test/testdata/oc_cli/case72217/crd-customtask-72217.yaml

Repository: openshift/oc

Length of output: 940


🏁 Script executed:

grep -r "case72217\|cr-custom-72217\|crd-customtask-72217" test --include="*.go" --include="*.sh" | head -20

Repository: openshift/oc

Length of output: 336


🏁 Script executed:

grep -B 10 -A 20 "case72217" test/e2e/cli.go

Repository: openshift/oc

Length of output: 1841


🏁 Script executed:

grep -B 5 -A 40 "customTaskCRDF\|customCRF" test/e2e/cli.go | grep -A 40 "Create the customtask"

Repository: openshift/oc

Length of output: 38


🏁 Script executed:

sed -n '1,5000p' test/e2e/cli.go | grep -B 3 -A 25 "customTaskCRDF :="

Repository: openshift/oc

Length of output: 1830


🏁 Script executed:

sed -n '1,5000p' test/e2e/cli.go | grep -B 2 -A 10 "customCRF\|cr of.*custom"

Repository: openshift/oc

Length of output: 1743


🏁 Script executed:

cat -n test/testdata/oc_cli/case72217/crd-customtask-72217.yaml | grep -i "preserve\|test"

Repository: openshift/oc

Length of output: 38


🏁 Script executed:

cat test/testdata/oc_cli/case72217/crd-customtask-72217.yaml

Repository: openshift/oc

Length of output: 730


🏁 Script executed:

git log --oneline --all -S "case72217" -- test/testdata/oc_cli/case72217 | head -5

Repository: openshift/oc

Length of output: 38


🏁 Script executed:

git log --oneline --all -S "case72217" | head -10

Repository: openshift/oc

Length of output: 142


🏁 Script executed:

grep -r "spec.test\|\.test\|test:" test/testdata/oc_cli/case72217/ --include="*.yaml" --include="*.yml"

Repository: openshift/oc

Length of output: 245


🏁 Script executed:

grep -A 20 "waitCreateCr(oc, customCRF" test/e2e/cli.go | head -30

Repository: openshift/oc

Length of output: 1502


CR uses a field not defined in its CRD schema.

Line 6 sets spec.test, but the CRD (crd-customtask-72217.yaml) only defines spec.cronSpec, spec.image, and spec.replicas. With v1 structural schemas and no preserveUnknownFields setting, unknown fields are pruned, so spec.test will be silently discarded. Align the CR with the schema or extend the CRD.

Note: The same issue affects cr-cat-72217.yaml and cr-cron-72217.yaml—all three CRs in this test case include spec.test which is undefined in their respective CRD schemas.

🔧 Example fix (extend CRD schema)
# test/testdata/oc_cli/case72217/crd-customtask-72217.yaml
           properties:
             spec:
               type: object
               properties:
                 cronSpec:
                   type: string
                 image:
                   type: string
                 replicas:
                   type: integer
+                test:
+                  type: string
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/case72217/cr-custom-72217.yaml` around lines 1 - 6, The
CR includes an undefined field spec.test which will be pruned because the CRD
crd-customtask-72217.yaml only defines spec.cronSpec, spec.image, and
spec.replicas; update the CR (cr-custom-72217.yaml) to only use the defined
fields or modify the CRD (crd-customtask-72217.yaml) to add spec.test to the
structural schema (or enable preserveUnknownFields) so the field is
accepted—also apply the same fix for cr-cat-72217.yaml and cr-cron-72217.yaml
where spec.test is present.

Comment on lines +8 to +20
initContainers:
- name: wait
image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c
command: ["/bin/sh", "-c", "sleep 10"]
containers:
- name: hello-pod
image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83
ports:
- containerPort: 8080
volumeMounts:
- mountPath: /tmp
name: tmp
volumes:
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

🧩 Analysis chain

🏁 Script executed:

# First, check if the file exists and examine its full content
find . -name "initContainer.yaml" -type f

Repository: openshift/oc

Length of output: 97


🏁 Script executed:

# Also search for the file path mentioned in the review
if [ -f "test/testdata/oc_cli/initContainer.yaml" ]; then
  cat -n test/testdata/oc_cli/initContainer.yaml
fi

Repository: openshift/oc

Length of output: 811


🏁 Script executed:

# Check context: is there a similar test setup in other testdata files?
find test/testdata -name "*.yaml" -type f | head -10

Repository: openshift/oc

Length of output: 556


🏁 Script executed:

# Check if there are other pod fixtures in testdata for comparison
grep -l "kind: Pod" test/testdata/oc_cli/*.yaml 2>/dev/null | head -5

Repository: openshift/oc

Length of output: 181


🏁 Script executed:

# Examine one or two similar pod fixtures to see if they have securityContext
for f in $(grep -l "kind: Pod" test/testdata/oc_cli/*.yaml 2>/dev/null | head -2); do
  echo "=== $f ==="
  head -30 "$f"
done

Repository: openshift/oc

Length of output: 1430


🌐 Web query:

Kubernetes Pod securityContext allowPrivilegeEscalation default value

💡 Result:

allowPrivilegeEscalation is a container-level field (spec.containers[*].securityContext.allowPrivilegeEscalation, not spec.securityContext).

  • Default (when unset): effectively true (privilege escalation is allowed unless you explicitly set it to false). (dynatrace.com)
  • Forced behavior: it is always true if the container is privileged or has CAP_SYS_ADMIN, regardless of what you set. (v1-33.docs.kubernetes.io)
  • Note: Kubernetes Pod Security Standards (restricted) require this field to be set to false. (kubernetes.io)

Citations:


Add securityContext to keep this pod valid under restricted policies.
Lines 8-20 omit securityContext for both containers. In clusters enforcing Pod Security "restricted" (or strict SCCs), this causes admission failures because allowPrivilegeEscalation defaults to true. Other test fixtures in this repo (e.g., debugpod_48681.yaml) follow this hardening pattern; add it here for consistency and test portability.

Suggested hardening
 spec:
+  securityContext:
+    runAsNonRoot: true
   initContainers:
     - name: wait
       image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c
       command: ["/bin/sh", "-c", "sleep 10"]
+      securityContext:
+        allowPrivilegeEscalation: false
   containers:
     - name: hello-pod
       image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83
       ports:
         - containerPort: 8080
       volumeMounts:
         - mountPath: /tmp
           name: tmp
+      securityContext:
+        allowPrivilegeEscalation: false
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/initContainer.yaml` around lines 8 - 20, Add a
securityContext to both the initContainer "wait" and the main container
"hello-pod" to satisfy restricted PodSecurity/SCC rules: set
allowPrivilegeEscalation: false, ensure runAsNonRoot: true (and a non-root
runAsUser if needed), and enable readOnlyRootFilesystem where appropriate;
update the initContainers and containers entries to include these
securityContext fields so the pod is accepted under restricted policies and
matches other fixtures like debugpod_48681.yaml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant