Migrating oc cli tests to ote #2181
Conversation
WalkthroughDeletes a placeholder Ginkgo e2e test and adds a new end-to-end test framework: a large CLI utility implementation, embedded fixture management, and many test data fixtures (YAML/JSON/text) for OpenShift/Kubernetes scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
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: 7
🤖 Fix all issues with AI agents
In `@test/e2e/util.go`:
- Around line 1288-1305: applyResourceFromTemplate builds the wrong command by
calling oc.Run("oc") which yields a double "oc" (e.g., "oc oc process ...");
change the invocation to use the verb only (e.g., oc.Run("process", ...)) or
otherwise call oc.execPath directly when building the process command so the
constructed command is "oc process ..."; update the call sites inside
applyResourceFromTemplate (references: function applyResourceFromTemplate,
method oc.Run, field oc.execPath, use of exec.Command) to ensure the processed
template step runs "oc process ..." and not "oc oc process ...", preserving the
rest of the logic (namespace and KUBECONFIG env handling) and returning
cmd.Run() as before.
- Around line 1086-1094: In sha256File, don't defer file.Close() before
verifying os.Open succeeded; check err returned by os.Open(fileName) first and
handle/return the error (or call o.Expect after confirming file is non-nil),
then defer file.Close(); ensure subsequent io.Copy and hex.EncodeToString still
run when file is valid and propagate any errors from io.Copy instead of causing
a nil-pointer panic.
- Around line 1307-1332: The function applyResourceFromTemplate48681 incorrectly
invokes oc.Run("oc") which constructs an invalid command; replace that call with
exec.Command using oc.execPath and the constructed fullArgs (i.e., build cmd :=
exec.Command(oc.execPath, fullArgs...) and call cmd.Output()), and make sure to
set cmd.Env to include "KUBECONFIG="+oc.kubeconfig when oc.kubeconfig is
non-empty (and include any necessary namespace flags in fullArgs if required) so
the template processing output is captured correctly and returned or error
propagated.
- Around line 886-899: In getLatestPayload, the defer res.Body.Close() is
currently placed after the http.Get error check but before ioutil.ReadAll, which
can still leak the body if ReadAll errors; move the defer immediately after
confirming http.Get returned no error and res != nil (i.e., right after the err
check) so the response body is guaranteed to be closed on every return, and
ensure any early e2e.Failf paths occur after that placement; keep existing error
handling for ioutil.ReadAll and gjson parsing.
In `@testdata/oc_cli/deploymentconfig_with_quota.yaml`:
- Around line 40-42: The memory request for the first container currently uses
the SI unit "40M" which is inconsistent with the second container's "40Mi" and
Kubernetes conventions; update the memory request value under the "requests:"
block (the entry currently "memory: 40M") to use the mebibyte unit "40Mi" so
both containers use the same unit and avoid quota calculation discrepancies.
In `@testdata/oc_cli/pod_mirror.yaml`:
- Around line 16-19: The shell loop condition in the args block currently uses
"[ ls /etc/foo/.dockerconfigjson ]" which always evaluates true; replace that
test with a proper file-existence check using the -f unary operator and negation
so the loop waits until /etc/foo/.dockerconfigjson exists (i.e., change the
while condition to test for "not -f /etc/foo/.dockerconfigjson" and keep the
surrounding "do sleep 1; done" and the subsequent exec oc adm release mirror
command intact).
In `@testdata/oc_cli/rc-match-service.yaml`:
- Around line 30-31: The volume entry named "config" under the "volumes" list is
missing a volume source and will fail validation; update the "config" volume
definition (the volumes -> name: config entry) to include an appropriate source
such as secret (e.g., secret: {secretName: ...}), configMap (e.g., configMap:
{name: ...}), or emptyDir, matching how the PodSpec mounts it (and ensure any
referenced secret/configMap exists and the mount path in the container matches).
🧹 Nitpick comments (3)
testdata/oc_cli/config_74099.yaml (1)
35-151: Consider security hardening for DeploymentConfig test fixture.The static analysis flags missing
allowPrivilegeEscalation: false(CKV_K8S_20). While this is acceptable for testdata captured from existing clusters, consider adding explicit security constraints if this fixture is meant to represent best practices:securityContext: allowPrivilegeEscalation: false capabilities: drop: - ALLGiven this appears to be a captured cluster configuration for test purposes, this is a minor concern.
testdata/oc_cli/stable-storage.yaml (1)
33-46: Consider explicit security settings for test consistency.The static analysis flags missing
allowPrivilegeEscalation: falseandrunAsNonRoot: true. While this is test data, adding explicit security settings ensures the test fixtures align with security best practices and can validate that workloads with proper security contexts function correctly.🔒 Optional: Add explicit security context settings
securityContext: capabilities: {} privileged: false + allowPrivilegeEscalation: false + runAsNonRoot: truetest/e2e/util.go (1)
11-11: Deprecatedio/ioutilpackage usage.The
io/ioutilpackage is deprecated since Go 1.16. Replace withosandiopackage equivalents:
ioutil.ReadAll→io.ReadAllioutil.ReadFile→os.ReadFileioutil.WriteFile→os.WriteFileioutil.ReadDir→os.ReadDir
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (176)
go.sumis excluded by!**/*.sumvendor/github.com/containerd/typeurl/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types_gogo.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/funcr/funcr.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/CONTRIBUTINGis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/expect.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/color.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/ssh.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/termios.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/format/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/duration_bundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/gomega.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/polling_signal_error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/vetoptdesc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/and.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/assignable_to_type_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_directory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_regular_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_an_existing_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_closed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_element_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_empty_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_equivalent_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_false_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_identical_to.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_key_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_nil_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_numerically_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_sent_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_temporally_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_true_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_zero_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/consist_of.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_element_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_elements_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_substring_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/equal_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_cap_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_each_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_exact_elements.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_existing_field_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_field.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_body_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_header_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_status_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_len_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_occurred_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_prefix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_suffix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_value.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_iter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_noiter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_error_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_json_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_regexp_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_xml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/not.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/or.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/panic_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/receive_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/satisfy_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/semi_structured_data_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/succeed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/bipartitegraph/bipartitegraph.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/node/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/type_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/with_transform.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/opencontainers/image-spec/specs-go/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/SYNTAX.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/gjson.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/match.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/pretty.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/logo.pngis excluded by!**/*.png,!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/sjson.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/filter.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/internal/attribute.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/gen.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/auto.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/span.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/status.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/traces.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/noop.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/block.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/cipher.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/const.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_arm64.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_generic.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_noasm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_ppc64x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_ppc64x.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_s390x.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/xor.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/curve25519/curve25519.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/buffer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/certs.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/channel.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/cipher.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/client.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/client_auth.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/common.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/connection.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/handshake.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/internal/bcrypt_pbkdf/bcrypt_pbkdf.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/kex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/keys.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mac.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/messages.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mlkem.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/session.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/ssh_gss.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/streamlocal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/tcpip.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/transport.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/rpc/status/status.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/NOTICE.txtis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/codes/code_string.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/codes/codes.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/connectivity/connectivity.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/component.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/grpclog.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/grpclog.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/logger.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/loggerv2.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/logger.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/loggerv2.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/experimental.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/status/status.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_others.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_unix.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_windows.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/serviceconfig/serviceconfig.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/status/status.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/protoadapt/convert.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/README.mdis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/patch.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/decode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/encode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/stream.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (36)
go.modtest/e2e/cli.gotest/e2e/e2e.gotest/e2e/util.gotestdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yamltestdata/oc_cli/OWNERStestdata/oc_cli/application-template-stibuild-with-mount-secret.jsontestdata/oc_cli/auth.jsontestdata/oc_cli/build_66672.yamltestdata/oc_cli/case72217/cr-cat-72217.yamltestdata/oc_cli/case72217/cr-cron-72217.yamltestdata/oc_cli/case72217/cr-custom-72217.yamltestdata/oc_cli/case72217/crd-cattoy-72217.yamltestdata/oc_cli/case72217/crd-crontab-72217.yamltestdata/oc_cli/case72217/crd-customtask-72217.yamltestdata/oc_cli/clusterresource_for_user.yamltestdata/oc_cli/config-images-75997.txttestdata/oc_cli/config_74099.yamltestdata/oc_cli/dc_66672.yamltestdata/oc_cli/debugpod_48681.yamltestdata/oc_cli/deploymentconfig_with_quota.yamltestdata/oc_cli/egressnetworkpolicy.yamltestdata/oc_cli/icsp60499.yamltestdata/oc_cli/icspFile63855.yamltestdata/oc_cli/icspFile64920.yamltestdata/oc_cli/idmsFile63855.yamltestdata/oc_cli/idmsFile64921.yamltestdata/oc_cli/initContainer.yamltestdata/oc_cli/initContainer66989.yamltestdata/oc_cli/mirror-from-filesystem.txttestdata/oc_cli/only-rc.yamltestdata/oc_cli/pod_mirror.yamltestdata/oc_cli/rc-match-service.yamltestdata/oc_cli/stable-storage.yamltestdata/oc_cli/testmapping.txttestdata/oc_cli/update_egressnetworkpolicy.yaml
💤 Files with no reviewable changes (1)
- test/e2e/e2e.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
testdata/oc_cli/initContainer66989.yamlgo.modtestdata/oc_cli/case72217/cr-cron-72217.yamltestdata/oc_cli/mirror-from-filesystem.txttestdata/oc_cli/only-rc.yamltestdata/oc_cli/case72217/crd-cattoy-72217.yamltestdata/oc_cli/config_74099.yamltestdata/oc_cli/debugpod_48681.yamltestdata/oc_cli/deploymentconfig_with_quota.yamltestdata/oc_cli/case72217/crd-crontab-72217.yamltestdata/oc_cli/idmsFile63855.yamltestdata/oc_cli/pod_mirror.yamltestdata/oc_cli/config-images-75997.txttestdata/oc_cli/initContainer.yamltestdata/oc_cli/case72217/crd-customtask-72217.yamltestdata/oc_cli/OWNERStestdata/oc_cli/case72217/cr-custom-72217.yamltestdata/oc_cli/idmsFile64921.yamltestdata/oc_cli/icspFile63855.yamltestdata/oc_cli/testmapping.txttestdata/oc_cli/auth.jsontestdata/oc_cli/icsp60499.yamltestdata/oc_cli/dc_66672.yamltestdata/oc_cli/stable-storage.yamltestdata/oc_cli/icspFile64920.yamltestdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yamltestdata/oc_cli/case72217/cr-cat-72217.yamltestdata/oc_cli/update_egressnetworkpolicy.yamltestdata/oc_cli/clusterresource_for_user.yamltestdata/oc_cli/rc-match-service.yamltestdata/oc_cli/application-template-stibuild-with-mount-secret.jsontestdata/oc_cli/egressnetworkpolicy.yamltest/e2e/util.gotestdata/oc_cli/build_66672.yaml
🪛 Checkov (3.2.334)
testdata/oc_cli/initContainer66989.yaml
[medium] 1-33: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-33: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/only-rc.yaml
[medium] 1-31: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-31: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/config_74099.yaml
[medium] 35-151: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/deploymentconfig_with_quota.yaml
[medium] 1-62: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/initContainer.yaml
[medium] 1-32: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-32: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/dc_66672.yaml
[medium] 1-19: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/stable-storage.yaml
[medium] 17-60: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 17-60: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/rc-match-service.yaml
[medium] 1-31: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-31: Minimize the admission of root containers
(CKV_K8S_23)
🪛 Gitleaks (8.30.0)
testdata/oc_cli/auth.json
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (30)
testdata/oc_cli/dc_66672.yaml (1)
1-19: Test fixture looks good for CLI e2e testing.The DeploymentConfig manifest is well-formed with properly matching selector and template labels, and uses a pinned image digest for reproducibility.
Regarding the static analysis hint (CKV_K8S_20): the missing
allowPrivilegeEscalation: falseis a valid security best practice for production workloads, but is acceptable to omit in test fixtures unless the test specifically validates security context behavior.testdata/oc_cli/update_egressnetworkpolicy.yaml (1)
1-12: LGTM! Valid test manifest for EgressNetworkPolicy update scenario.The manifest structure is correct for testing OpenShift egress policy updates. The use of the same
metadata.nameas the base egressnetworkpolicy.yaml file is appropriate for testing resource update operations.testdata/oc_cli/egressnetworkpolicy.yaml (1)
1-12: LGTM! Valid test manifest for baseline EgressNetworkPolicy.The manifest structure is correct for OpenShift egress policy testing. The Allow rule for a specific /32 CIDR followed by a deny-all rule provides appropriate test isolation.
testdata/oc_cli/OWNERS (1)
1-3: LGTM!Standard OWNERS file format with designated approvers for the testdata directory.
testdata/oc_cli/auth.json (1)
1-7: LGTM - Dummy test credentials are appropriate.The base64 value decodes to
admin:123456, which is clearly placeholder test data. The static analysis false positive can be safely ignored since this is an intentional fixture for testing authentication flows, not real credentials.testdata/oc_cli/clusterresource_for_user.yaml (1)
1-15: LGTM!Valid ClusterResourceQuota manifest with reasonable test values. The annotation-based selector pattern is correct for user-scoped quotas.
testdata/oc_cli/mirror-from-filesystem.txt (1)
1-1: LGTM!Valid image mirror mapping format for filesystem-to-registry test scenarios.
testdata/oc_cli/config-images-75997.txt (1)
1-2: LGTM!Valid image mapping entries for multi-architecture test scenarios using a local registry destination.
testdata/oc_cli/testmapping.txt (1)
1-1: LGTM!Valid image mapping format for test fixtures.
testdata/oc_cli/initContainer66989.yaml (1)
1-33: LGTM!Well-structured Pod manifest for initContainer testing. The pinned image digests ensure test reproducibility. The static analysis security hints (CKV_K8S_20, CKV_K8S_23) are appropriately ignored for test fixtures where minimal security configuration is intentional.
testdata/oc_cli/initContainer.yaml (1)
1-32: LGTM!Valid Pod manifest for initContainer testing with 10-second sleep. Static analysis security hints are appropriately ignored for test fixtures.
testdata/oc_cli/only-rc.yaml (1)
1-31: LGTM!Well-structured ReplicationController manifest with properly configured secret volume mount. Static analysis security hints are appropriately ignored for test fixtures.
testdata/oc_cli/debugpod_48681.yaml (1)
1-39: Well-configured security context for test pod.The template properly enforces security constraints:
runAsNonRoot,seccompProfile: RuntimeDefault,allowPrivilegeEscalation: false, and droppedALLcapabilities. This is appropriate for testing secure pod configurations.testdata/oc_cli/icspFile63855.yaml (1)
1-12: LGTM - valid ICSP test fixture.The ImageContentSourcePolicy structure is correct for testing mirror configuration scenarios. The hardcoded EC2 hostname is acceptable for test fixtures.
testdata/oc_cli/build_66672.yaml (1)
1-14: LGTM - valid Build test fixture.The Build configuration is well-structured with a pinned image digest for reproducibility. Appropriate for testing source strategy builds.
testdata/oc_cli/case72217/crd-crontab-72217.yaml (1)
1-30: LGTM!The CRD manifest is well-structured and follows Kubernetes conventions. The schema, naming, and scope are appropriately defined for test purposes.
testdata/oc_cli/case72217/crd-customtask-72217.yaml (1)
1-30: LGTM!The CRD manifest follows Kubernetes conventions and is consistent with the other test CRDs in this PR. The schema structure is appropriately defined for test purposes.
go.mod (1)
34-34:goexpectversion is appropriate—no updates or security issues.The
goexpectdependency is pinned tov0.0.0-20210430020637, which is the final commit from the google/goexpect repository (April 30, 2021). The repository became archived in February 2023 and has no formal releases or newer versions available. No CVE entries or security vulnerabilities exist for this version. No action required.testdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yaml (1)
1-913: LGTM - Test fixture data for ImageStream scenarios.This is a comprehensive ImageStream test fixture capturing a multi-arch nightly release with documented RPM inconsistencies. The structure is valid and serves its purpose for CLI testing.
test/e2e/util.go (2)
49-66: LGTM - CLI wrapper structure.The CLI struct and constructor provide a clean fluent API for test command execution.
229-239: LGTM - Project setup with cleanup.Good use of
g.DeferCleanupto ensure test project cleanup.testdata/oc_cli/icsp60499.yaml (1)
1-12: LGTM - ICSP test fixture.Valid ImageContentSourcePolicy test fixture. The duplicate mirror entries may be intentional for testing deduplication or redundant configuration handling.
testdata/oc_cli/case72217/cr-cat-72217.yaml (1)
1-6: LGTM - Custom Resource test fixture.Simple and valid CR instance for testing CRD scenarios with the corresponding Cattoy72217 CRD.
testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
244-251: Inconsistent indentation in volume definition.The volume definition uses mixed tabs and irregular spacing, which differs from the rest of the file's formatting. While JSON parsers will handle this, it reduces readability and maintainability.
🔧 Suggested fix for consistent formatting
- "volumes": [ - { - "name": "mytest-volume", - "secret": { - "secretName": "my-secret" - } - } - ] + "volumes": [ + { + "name": "mytest-volume", + "secret": { + "secretName": "my-secret" + } + } + ]Likely an incorrect or invalid review comment.
testdata/oc_cli/case72217/cr-custom-72217.yaml (1)
1-6: LGTM!Valid custom resource manifest that correctly references the
example.com/v1API group andCustomtask72217kind defined in the corresponding CRD.testdata/oc_cli/case72217/cr-cron-72217.yaml (1)
1-6: LGTM!Valid custom resource manifest for
CronTab72217with correct API group reference.testdata/oc_cli/idmsFile64921.yaml (1)
1-18: LGTM!Valid
ImageDigestMirrorSetmanifest with appropriate localhost mirrors for testing. The explicitmirrorSourcePolicysettings are good practice.testdata/oc_cli/case72217/crd-cattoy-72217.yaml (1)
1-30: LGTM!Well-structured CRD manifest with proper OpenAPI v3 schema, correct API version, and all required fields.
testdata/oc_cli/idmsFile63855.yaml (1)
1-12: LGTM!Valid
ImageDigestMirrorSetmanifest. The hardcoded EC2 hostname is acceptable for test fixtures where actual connectivity isn't required during test execution.testdata/oc_cli/icspFile64920.yaml (1)
1-2: Consider updating to the current ImageDigestMirrorSet API if this is not specifically testing backward compatibility.The
operator.openshift.io/v1alpha1API version for ImageContentSourcePolicy is deprecated as of OpenShift 4.13 and replaced by ImageDigestMirrorSet (usingconfig.openshift.io/v1). While still supported, new test data should use the current stable API unless this file is intended to specifically test ICSP compatibility or migration paths.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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) | ||
| } | ||
|
|
||
| pullSpec := gjson.Get(string(body), `pullSpec`).String() | ||
| return pullSpec | ||
| } |
There was a problem hiding this comment.
Defer placement after error check can cause resource leak.
The defer res.Body.Close() is placed after error handling for http.Get but the body could still leak if ioutil.ReadAll fails since the defer comes after the first error check but before the read operation succeeds.
🐛 Recommended fix
func getLatestPayload(url string) string {
res, err := http.Get(url)
if err != nil {
e2e.Failf("unable to get http with error: %v", err)
}
+ defer res.Body.Close()
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)
}📝 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.
| 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) | |
| } | |
| pullSpec := gjson.Get(string(body), `pullSpec`).String() | |
| return pullSpec | |
| } | |
| func getLatestPayload(url string) string { | |
| res, err := http.Get(url) | |
| if err != nil { | |
| e2e.Failf("unable to get http with error: %v", err) | |
| } | |
| defer res.Body.Close() | |
| body, err := ioutil.ReadAll(res.Body) | |
| if err != nil { | |
| e2e.Failf("unable to parse the http result with error: %v", err) | |
| } | |
| pullSpec := gjson.Get(string(body), `pullSpec`).String() | |
| return pullSpec | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 886 - 899, In getLatestPayload, the defer
res.Body.Close() is currently placed after the http.Get error check but before
ioutil.ReadAll, which can still leak the body if ReadAll errors; move the defer
immediately after confirming http.Get returned no error and res != nil (i.e.,
right after the err check) so the response body is guaranteed to be closed on
every return, and ensure any early e2e.Failf paths occur after that placement;
keep existing error handling for ioutil.ReadAll and gjson parsing.
| requests: | ||
| cpu: 50m | ||
| memory: 40M |
There was a problem hiding this comment.
Inconsistent memory unit notation.
Line 42 uses 40M (SI megabytes) while line 54 uses 40Mi (mebibytes). This inconsistency could cause confusion in quota calculations since 40M ≈ 38.15Mi.
For consistency with Kubernetes conventions and the second container's specification:
requests:
cpu: 50m
- memory: 40M
+ memory: 40Mi📝 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.
| requests: | |
| cpu: 50m | |
| memory: 40M | |
| requests: | |
| cpu: 50m | |
| memory: 40Mi |
🤖 Prompt for AI Agents
In `@testdata/oc_cli/deploymentconfig_with_quota.yaml` around lines 40 - 42, The
memory request for the first container currently uses the SI unit "40M" which is
inconsistent with the second container's "40Mi" and Kubernetes conventions;
update the memory request value under the "requests:" block (the entry currently
"memory: 40M") to use the mebibyte unit "40Mi" so both containers use the same
unit and avoid quota calculation discrepancies.
| volumes: | ||
| - name: config |
There was a problem hiding this comment.
Incomplete volume definition will fail validation.
The config volume only has a name but no volume source. Kubernetes requires a source such as secret, configMap, or emptyDir. This manifest will fail when applied.
🐛 Proposed fix - add volume source
If this should be a secret (like only-rc.yaml):
volumes:
- name: config
+ secret:
+ secretName: mysecretOr if it should be a configMap:
volumes:
- name: config
+ configMap:
+ name: master-config🧰 Tools
🪛 Checkov (3.2.334)
[medium] 1-31: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-31: Minimize the admission of root containers
(CKV_K8S_23)
🤖 Prompt for AI Agents
In `@testdata/oc_cli/rc-match-service.yaml` around lines 30 - 31, The volume entry
named "config" under the "volumes" list is missing a volume source and will fail
validation; update the "config" volume definition (the volumes -> name: config
entry) to include an appropriate source such as secret (e.g., secret:
{secretName: ...}), configMap (e.g., configMap: {name: ...}), or emptyDir,
matching how the PodSpec mounts it (and ensure any referenced secret/configMap
exists and the mount path in the container matches).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (176)
go.sumis excluded by!**/*.sumvendor/github.com/containerd/typeurl/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types_gogo.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/funcr/funcr.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/CONTRIBUTINGis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goexpect/expect.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/color.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/ssh.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/google/goterm/term/termios.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/format/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/duration_bundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/gomega.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/polling_signal_error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/vetoptdesc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/and.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/assignable_to_type_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_directory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_regular_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_an_existing_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_closed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_element_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_empty_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_equivalent_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_false_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_identical_to.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_key_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_nil_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_numerically_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_sent_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_temporally_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_true_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_zero_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/consist_of.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_element_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_elements_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_substring_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/equal_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_cap_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_each_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_exact_elements.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_existing_field_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_field.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_body_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_header_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_status_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_len_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_occurred_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_prefix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_suffix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_value.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_iter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_noiter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_error_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_json_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_regexp_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_xml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/not.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/or.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/panic_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/receive_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/satisfy_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/semi_structured_data_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/succeed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/bipartitegraph/bipartitegraph.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/node/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/type_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/with_transform.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/opencontainers/image-spec/specs-go/version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/SYNTAX.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/gjson/gjson.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/match/match.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/pretty/pretty.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/logo.pngis excluded by!**/*.png,!vendor/**,!**/vendor/**vendor/github.com/tidwall/sjson/sjson.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/filter.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/internal/attribute.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/gen.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/auto.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/span.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/status.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/traces.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/noop.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/block.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/cipher.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/blowfish/const.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_arm64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_arm64.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_generic.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_noasm.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_ppc64x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_ppc64x.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_s390x.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/chacha_s390x.sis excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/chacha20/xor.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/curve25519/curve25519.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/buffer.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/certs.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/channel.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/cipher.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/client.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/client_auth.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/common.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/connection.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/doc.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/handshake.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/internal/bcrypt_pbkdf/bcrypt_pbkdf.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/kex.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/keys.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mac.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/messages.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mlkem.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/mux.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/server.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/session.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/ssh_gss.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/streamlocal.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/tcpip.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/ssh/transport.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/rpc/status/status.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/NOTICE.txtis excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/codes/code_string.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/codes/codes.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/connectivity/connectivity.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/component.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/grpclog.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/grpclog.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/logger.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/internal/loggerv2.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/logger.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/grpclog/loggerv2.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/experimental.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/status/status.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_others.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_unix.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/internal/tcp_keepalive_windows.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/serviceconfig/serviceconfig.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/grpc/status/status.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/protobuf/protoadapt/convert.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/README.mdis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/patch.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/decode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/encode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/stream.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (1)
go.mod
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
go.mod
🔇 Additional comments (3)
go.mod (3)
244-244: Investigate the necessity and compatibility of the ginkgo fork.The
replacedirective overridesgithub.com/onsi/ginkgo/v2with an OpenShift fork based on v2.6.1, while line 40 requires v2.27.2. This represents a significant version gap (21 minor versions behind upstream).Verify:
- Whether the OpenShift fork includes critical patches or OpenShift-specific features that justify the version gap
- If the fork is actively maintained and receives security updates
- Whether removing the replace directive and using upstream v2.27.2 directly is compatible with the codebase
- If this fork replacement is still necessary or can be removed
104-106: Verify compatibility and security for indirect dependency updates against release notes.Multiple indirect dependencies have been updated including gRPC, Protobuf, OpenTelemetry, and Kubernetes ecosystem packages. While web searches reveal no critical unpatched vulnerabilities in the versions mentioned (Protobuf v1.36.8, OpenTelemetry v1.36.0, containerd/errdefs v1.0.0), check the release notes and changelogs for:
- gRPC v1.72.2: Verify whether this version addresses known gRPC CVEs (CVE-2024-7246, CVE-2024-11407, and others)
- All listed updates: Confirm no breaking changes or compatibility issues with your target Kubernetes/gRPC versions
The shell script approach in the original comment has limitations (requires
ghCLI with proper authentication); a direct review of project release notes is more reliable.
40-41: No breaking changes or critical issues found in the test framework updates.Release notes for ginkgo/v2 (v2.21.0→v2.27.2) and gomega (v1.35.1→v1.38.2) show only maintenance fixes and backwards-compatible features. Notable additions include new environment variables (GINKGO_TIME_FORMAT, GINKGO_NO_COLOR) and optional reporting formats (JSON output options, SemVer filtering), but these do not affect existing tests. The updates are safe to apply.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
go.mod
Outdated
| github.com/go-ldap/ldap/v3 v3.4.11 | ||
| github.com/gonum/graph v0.0.0-20170401004347-50b27dea7ebb | ||
| github.com/google/go-cmp v0.7.0 | ||
| github.com/google/goexpect v0.0.0-20210430020637-ab937bf7fd6f |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
google/goexpect github releases tagged versions
💡 Result:
google/goexpect has no GitHub releases — the repository is archived (read-only) and the Releases page shows "There aren’t any releases here." [1][2]
Sources:
- google/goexpect repository (archived). [1]
- google/goexpect Releases page (no releases). [2]
🏁 Script executed:
#!/bin/bash
# Check goexpect repository metadata and release information
# Using curl to query GitHub API without gh CLI
echo "=== Checking for goexpect releases ==="
curl -s https://api.github.com/repos/google/goexpect/releases | jq -r '.[].tag_name' | head -20
echo -e "\n=== Checking latest commits ==="
curl -s https://api.github.com/repos/google/goexpect/commits | jq -r '.[] | "\(.commit.committer.date) - \(.commit.message | split("\n")[0])"' | head -5
echo -e "\n=== Checking repository metadata ==="
curl -s https://api.github.com/repos/google/goexpect | jq '{archived: .archived, updated_at: .updated_at, forks: .forks_count, watchers: .watchers_count, open_issues: .open_issues_count}'Repository: openshift/oc
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check if there are any Go security advisories for goexpect
# Search for known vulnerability databases
echo "=== Searching for security information about goexpect ==="
# Check Go vulnerability database
curl -s https://pkg.go.dev/github.com/google/goexpect | grep -i "vulnerability\|security\|deprecated" || echo "No direct security info found"
# Check for any Go mod issues documentation
echo -e "\n=== Checking go.dev package page metadata ==="
curl -s https://pkg.go.dev/api/gomod/github.com/google/goexpect/v0.0.0-20210430020637-ab937bf7fd6f 2>/dev/null | jq '.' || echo "No metadata available for this specific pseudo-version"Repository: openshift/oc
Length of output: 168
Replace github.com/google/goexpect with an actively maintained alternative.
The google/goexpect repository is archived and has no tagged releases. The pseudo-version points to a commit from April 2021, and the library is no longer maintained. Using archived, unmaintained dependencies introduces maintainability and security risks.
Consider either switching to an actively maintained expect-style testing library or evaluating whether this dependency is still necessary.
There was a problem hiding this comment.
We have several dependencies and for the sustainability we can't add new ones. So it is better to add the tests that are using our current libraries without any new dependency.
There was a problem hiding this comment.
@ardaguclu Understood — I appreciate you clarifying the project's dependency policy. Keeping the dependency footprint minimal is a sensible approach for sustainability and maintainability.
If the test infrastructure can be refactored to use existing dependencies or built-in Go functionality instead of adding google/goexpect, tidwall/gjson, and tidwall/sjson, that would align with your guidelines. Let me know if you'd like any suggestions on alternative approaches using the current dependency set.
|
/test unit |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@testdata/oc_cli/application-template-stibuild-with-mount-secret.json`:
- Around line 200-251: The container "ruby-helloworld" is missing a volumeMount
for the defined secret volume "mytest-volume", so add a "volumeMounts" entry
inside the container spec for the "ruby-helloworld" container that mounts
"mytest-volume" at an appropriate path (e.g., /etc/secret or /etc/my-secret)
with "readOnly": true; ensure the "volumeMounts" array is at the same level as
"env", "resources", and "imagePullPolicy" in the "containers" element so the
secret becomes accessible inside the container.
♻️ Duplicate comments (2)
testdata/oc_cli/rc-match-service.yaml (1)
30-31: Incomplete volume definition will fail validation.The
configvolume is missing a volume source (e.g.,secret,configMap,emptyDir). Kubernetes will reject this manifest.testdata/oc_cli/deploymentconfig_with_quota.yaml (1)
40-42: Inconsistent memory unit notation.Line 42 uses
40Mwhile line 54 uses40Mi. This mismatch could cause subtle quota calculation differences (40M ≈ 38.15Mi). Align both containers to use40Mifor consistency.requests: cpu: 50m - memory: 40M + memory: 40Mi
🧹 Nitpick comments (4)
testdata/oc_cli/initContainer.yaml (1)
8-26: Consider adding security hardening to match other test fixtures.The static analysis correctly flags missing security constraints. Unlike
debugpod_48681.yamlin this PR which properly configures security, this manifest lacksallowPrivilegeEscalation: falseandrunAsNonRoot: true. The initContainer also has no securityContext.If this is intentional for testing permissive scenarios, consider adding a comment. Otherwise, aligning with the hardened pattern improves consistency.
🔒 Suggested security hardening
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault initContainers: - name: wait image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c command: ["/bin/sh", "-c", "sleep 10"] + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL containers: - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" ... securityContext: - capabilities: {} - privileged: false + allowPrivilegeEscalation: false + capabilities: + drop: + - ALLtestdata/oc_cli/initContainer66989.yaml (1)
1-33: Same security hardening recommendation applies.This manifest is nearly identical to
initContainer.yamlwith a longer sleep duration. The same security context improvements suggested for that file apply here.The trailing
status: {}on line 33 is typically server-populated and unnecessary in a source manifest, though it's harmless.testdata/oc_cli/stable-storage.yaml (1)
31-32: Deprecated annotation present.The annotation
pod.alpha.kubernetes.io/initializedis from very old Kubernetes versions (pre-1.5) and is no longer functional. If this testdata is meant to test handling of legacy manifests, this is intentional. Otherwise, it can be removed.testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
244-251: Inconsistent indentation involumessection.This block uses tabs while the rest of the file uses spaces. Consider normalizing to spaces for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (94)
go.sumis excluded by!**/*.sumvendor/github.com/containerd/typeurl/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/containerd/typeurl/v2/types_gogo.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-logr/logr/funcr/funcr.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/format/format.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/gomega_dsl.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/async_assertion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/duration_bundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/gomega.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/polling_signal_error.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/internal/vetoptdesc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/and.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/assignable_to_type_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_directory.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_a_regular_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_an_existing_file.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_closed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_comparable_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_element_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_empty_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_equivalent_to_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_false_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_identical_to.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_key_of_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_nil_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_numerically_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_sent_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_temporally_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_true_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/be_zero_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/consist_of.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_element_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_elements_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/contain_substring_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/equal_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_cap_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_each_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_exact_elements.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_existing_field_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_field.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_body_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_header_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_http_status_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_key_with_value_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_len_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_occurred_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_prefix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_suffix_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/have_value.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_iter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/internal/miter/type_support_noiter.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_error_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_json_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_regexp_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_xml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/match_yaml_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/not.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/or.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/panic_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/receive_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/satisfy_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/semi_structured_data_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/succeed_matcher.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/bipartitegraph/bipartitegraph.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/support/goraph/node/node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/type_support.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/matchers/with_transform.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/onsi/gomega/types/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/opencontainers/image-spec/specs-go/version.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/filter.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/internal/attribute.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/attribute/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/gen.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/internal/rawhelpers.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/auto.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/span.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/status.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/traces.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/internal/telemetry/value.gois excluded by!vendor/**,!**/vendor/**vendor/go.opentelemetry.io/otel/trace/noop.gois excluded by!vendor/**,!**/vendor/**vendor/google.golang.org/genproto/googleapis/rpc/status/status.pb.gois excluded by!**/*.pb.go,!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/README.mdis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/evanphx/json-patch.v4/patch.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/decode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/encode.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/json/internal/golang/encoding/json/stream.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (36)
go.modtest/e2e/cli.gotest/e2e/e2e.gotest/e2e/util.gotestdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yamltestdata/oc_cli/OWNERStestdata/oc_cli/application-template-stibuild-with-mount-secret.jsontestdata/oc_cli/auth.jsontestdata/oc_cli/build_66672.yamltestdata/oc_cli/case72217/cr-cat-72217.yamltestdata/oc_cli/case72217/cr-cron-72217.yamltestdata/oc_cli/case72217/cr-custom-72217.yamltestdata/oc_cli/case72217/crd-cattoy-72217.yamltestdata/oc_cli/case72217/crd-crontab-72217.yamltestdata/oc_cli/case72217/crd-customtask-72217.yamltestdata/oc_cli/clusterresource_for_user.yamltestdata/oc_cli/config-images-75997.txttestdata/oc_cli/config_74099.yamltestdata/oc_cli/dc_66672.yamltestdata/oc_cli/debugpod_48681.yamltestdata/oc_cli/deploymentconfig_with_quota.yamltestdata/oc_cli/egressnetworkpolicy.yamltestdata/oc_cli/icsp60499.yamltestdata/oc_cli/icspFile63855.yamltestdata/oc_cli/icspFile64920.yamltestdata/oc_cli/idmsFile63855.yamltestdata/oc_cli/idmsFile64921.yamltestdata/oc_cli/initContainer.yamltestdata/oc_cli/initContainer66989.yamltestdata/oc_cli/mirror-from-filesystem.txttestdata/oc_cli/only-rc.yamltestdata/oc_cli/pod_mirror.yamltestdata/oc_cli/rc-match-service.yamltestdata/oc_cli/stable-storage.yamltestdata/oc_cli/testmapping.txttestdata/oc_cli/update_egressnetworkpolicy.yaml
💤 Files with no reviewable changes (1)
- test/e2e/e2e.go
✅ Files skipped from review due to trivial changes (4)
- testdata/oc_cli/case72217/cr-custom-72217.yaml
- testdata/oc_cli/case72217/crd-cattoy-72217.yaml
- testdata/oc_cli/mirror-from-filesystem.txt
- testdata/oc_cli/idmsFile64921.yaml
🚧 Files skipped from review as they are similar to previous changes (15)
- testdata/oc_cli/OWNERS
- testdata/oc_cli/update_egressnetworkpolicy.yaml
- testdata/oc_cli/build_66672.yaml
- go.mod
- testdata/oc_cli/idmsFile63855.yaml
- testdata/oc_cli/pod_mirror.yaml
- testdata/oc_cli/testmapping.txt
- testdata/oc_cli/icspFile64920.yaml
- testdata/oc_cli/case72217/cr-cat-72217.yaml
- testdata/oc_cli/case72217/cr-cron-72217.yaml
- testdata/oc_cli/case72217/crd-crontab-72217.yaml
- testdata/oc_cli/config-images-75997.txt
- testdata/oc_cli/egressnetworkpolicy.yaml
- test/e2e/util.go
- testdata/oc_cli/icsp60499.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
testdata/oc_cli/debugpod_48681.yamltestdata/oc_cli/initContainer.yamltestdata/oc_cli/clusterresource_for_user.yamltestdata/oc_cli/stable-storage.yamltestdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yamltestdata/oc_cli/application-template-stibuild-with-mount-secret.jsontestdata/oc_cli/case72217/crd-customtask-72217.yamltestdata/oc_cli/only-rc.yamltestdata/oc_cli/icspFile63855.yamltestdata/oc_cli/auth.jsontestdata/oc_cli/config_74099.yamltestdata/oc_cli/dc_66672.yamltestdata/oc_cli/deploymentconfig_with_quota.yamltestdata/oc_cli/initContainer66989.yamltestdata/oc_cli/rc-match-service.yaml
🪛 Checkov (3.2.334)
testdata/oc_cli/initContainer.yaml
[medium] 1-32: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-32: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/stable-storage.yaml
[medium] 17-60: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 17-60: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/only-rc.yaml
[medium] 1-31: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-31: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/config_74099.yaml
[medium] 35-151: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/dc_66672.yaml
[medium] 1-19: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/deploymentconfig_with_quota.yaml
[medium] 1-62: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
testdata/oc_cli/initContainer66989.yaml
[medium] 1-33: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-33: Minimize the admission of root containers
(CKV_K8S_23)
testdata/oc_cli/rc-match-service.yaml
[medium] 1-31: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-31: Minimize the admission of root containers
(CKV_K8S_23)
🪛 Gitleaks (8.30.0)
testdata/oc_cli/auth.json
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (11)
testdata/oc_cli/clusterresource_for_user.yaml (1)
1-15: LGTM!This is a well-formed ClusterResourceQuota test fixture with valid structure, appropriate quota limits for testing, and correct annotation-based selector configuration.
testdata/oc_cli/auth.json (1)
1-7: LGTM — Test fixture with placeholder credentials.The static analysis warning is a false positive. The base64 value decodes to
admin:123456, which is clearly a dummy credential for test purposes, appropriately located in thetestdata/directory.testdata/oc_cli/case72217/crd-customtask-72217.yaml (1)
1-30: LGTM!This is a well-structured CRD test fixture. The manifest follows the Kubernetes apiextensions.k8s.io/v1 API specification correctly with proper versioning, schema definition, and naming configuration.
testdata/oc_cli/icspFile63855.yaml (1)
1-12: LGTM!The ImageContentSourcePolicy manifest is well-structured with valid apiVersion, kind, metadata, and spec fields. The two
repositoryDigestMirrorsentries correctly map upstream sources to the mirror registry.testdata/oc_cli/only-rc.yaml (1)
1-31: LGTM!The ReplicationController manifest is well-formed with a properly configured secret volume source. Suitable for test data purposes.
testdata/oc_cli/deploymentconfig_with_quota.yaml (1)
45-56: LGTM!Second container configuration is well-defined with consistent resource units.
testdata/oc_cli/debugpod_48681.yaml (1)
1-39: Well-configured security context for test pod.The template properly enforces restricted Pod Security Standards with
runAsNonRoot,seccompProfile: RuntimeDefault,allowPrivilegeEscalation: false, and drops all capabilities. This is a good practice even for test fixtures.testdata/oc_cli/config_74099.yaml (1)
1-151: Testdata fixture looks appropriate for CLI testing.This manifest provides good coverage for testing DeploymentConfig with secrets, probes, and deployment strategy. The hardcoded values (clusterIP, route host, namespace) are acceptable for test fixtures.
Regarding the static analysis hint (CKV_K8S_20): the
allowPrivilegeEscalationwarning is not actionable here since this is testdata for CLI validation, not a production deployment manifest.testdata/oc_cli/dc_66672.yaml (1)
1-19: Minimal DeploymentConfig fixture is valid.This provides a clean minimal manifest for testing basic DeploymentConfig operations. The static analysis warning (CKV_K8S_20) is not applicable for test fixtures.
testdata/oc_cli/stable-storage.yaml (1)
1-60: StatefulSet fixture is suitable for testing.The manifest correctly defines a headless Service for StatefulSet DNS and includes PVC templates for storage testing. The static analysis warnings (CKV_K8S_20, CKV_K8S_23) are not actionable for test fixtures.
testdata/oc_cli/12708358_4.11.0-0.nightly-multi-2022-04-18-120932-release-imagestream.yaml (1)
1-913: ImageStream fixture is appropriate for release image testing.This comprehensive release ImageStream snapshot provides excellent coverage for testing
occommands related to image operations (mirror, extract, info). The embedded inconsistency annotations accurately reflect real-world release scenarios and will help validate CLI handling of such metadata.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "spec": { | ||
| "containers": [ | ||
| { | ||
| "name": "ruby-helloworld", | ||
| "image": "origin-ruby-sample", | ||
| "ports": [ | ||
| { | ||
| "containerPort": 8080, | ||
| "protocol": "TCP" | ||
| } | ||
| ], | ||
| "env": [ | ||
| { | ||
| "name": "ADMIN_USERNAME", | ||
| "value": "${ADMIN_USERNAME}" | ||
| }, | ||
| { | ||
| "name": "ADMIN_PASSWORD", | ||
| "value": "${ADMIN_PASSWORD}" | ||
| }, | ||
| { | ||
| "name": "MYSQL_USER", | ||
| "value": "${MYSQL_USER}" | ||
| }, | ||
| { | ||
| "name": "MYSQL_PASSWORD", | ||
| "value": "${MYSQL_PASSWORD}" | ||
| }, | ||
| { | ||
| "name": "MYSQL_DATABASE", | ||
| "value": "${MYSQL_DATABASE}" | ||
| } | ||
| ], | ||
| "resources": {}, | ||
| "terminationMessagePath": "/dev/termination-log", | ||
| "imagePullPolicy": "IfNotPresent", | ||
| "securityContext": { | ||
| "capabilities": {}, | ||
| "privileged": false | ||
| } | ||
| } | ||
| ], | ||
| "restartPolicy": "Always", | ||
| "dnsPolicy": "ClusterFirst", | ||
| "volumes": [ | ||
| { | ||
| "name": "mytest-volume", | ||
| "secret": { | ||
| "secretName": "my-secret" | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
Missing volumeMounts for the secret volume.
The volumes section defines mytest-volume backed by secret my-secret, but the container ruby-helloworld lacks a corresponding volumeMounts entry. The secret won't be accessible inside the container.
Given the template name emphasizes "with-mount-secret", this appears unintentional.
Proposed fix
"resources": {},
"terminationMessagePath": "/dev/termination-log",
"imagePullPolicy": "IfNotPresent",
+ "volumeMounts": [
+ {
+ "name": "mytest-volume",
+ "mountPath": "/etc/secret-volume",
+ "readOnly": true
+ }
+ ],
"securityContext": {
"capabilities": {},
"privileged": false
}📝 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.
| "spec": { | |
| "containers": [ | |
| { | |
| "name": "ruby-helloworld", | |
| "image": "origin-ruby-sample", | |
| "ports": [ | |
| { | |
| "containerPort": 8080, | |
| "protocol": "TCP" | |
| } | |
| ], | |
| "env": [ | |
| { | |
| "name": "ADMIN_USERNAME", | |
| "value": "${ADMIN_USERNAME}" | |
| }, | |
| { | |
| "name": "ADMIN_PASSWORD", | |
| "value": "${ADMIN_PASSWORD}" | |
| }, | |
| { | |
| "name": "MYSQL_USER", | |
| "value": "${MYSQL_USER}" | |
| }, | |
| { | |
| "name": "MYSQL_PASSWORD", | |
| "value": "${MYSQL_PASSWORD}" | |
| }, | |
| { | |
| "name": "MYSQL_DATABASE", | |
| "value": "${MYSQL_DATABASE}" | |
| } | |
| ], | |
| "resources": {}, | |
| "terminationMessagePath": "/dev/termination-log", | |
| "imagePullPolicy": "IfNotPresent", | |
| "securityContext": { | |
| "capabilities": {}, | |
| "privileged": false | |
| } | |
| } | |
| ], | |
| "restartPolicy": "Always", | |
| "dnsPolicy": "ClusterFirst", | |
| "volumes": [ | |
| { | |
| "name": "mytest-volume", | |
| "secret": { | |
| "secretName": "my-secret" | |
| } | |
| } | |
| ] | |
| "spec": { | |
| "containers": [ | |
| { | |
| "name": "ruby-helloworld", | |
| "image": "origin-ruby-sample", | |
| "ports": [ | |
| { | |
| "containerPort": 8080, | |
| "protocol": "TCP" | |
| } | |
| ], | |
| "env": [ | |
| { | |
| "name": "ADMIN_USERNAME", | |
| "value": "${ADMIN_USERNAME}" | |
| }, | |
| { | |
| "name": "ADMIN_PASSWORD", | |
| "value": "${ADMIN_PASSWORD}" | |
| }, | |
| { | |
| "name": "MYSQL_USER", | |
| "value": "${MYSQL_USER}" | |
| }, | |
| { | |
| "name": "MYSQL_PASSWORD", | |
| "value": "${MYSQL_PASSWORD}" | |
| }, | |
| { | |
| "name": "MYSQL_DATABASE", | |
| "value": "${MYSQL_DATABASE}" | |
| } | |
| ], | |
| "resources": {}, | |
| "terminationMessagePath": "/dev/termination-log", | |
| "imagePullPolicy": "IfNotPresent", | |
| "volumeMounts": [ | |
| { | |
| "name": "mytest-volume", | |
| "mountPath": "/etc/secret-volume", | |
| "readOnly": true | |
| } | |
| ], | |
| "securityContext": { | |
| "capabilities": {}, | |
| "privileged": false | |
| } | |
| } | |
| ], | |
| "restartPolicy": "Always", | |
| "dnsPolicy": "ClusterFirst", | |
| "volumes": [ | |
| { | |
| "name": "mytest-volume", | |
| "secret": { | |
| "secretName": "my-secret" | |
| } | |
| } | |
| ] |
🤖 Prompt for AI Agents
In `@testdata/oc_cli/application-template-stibuild-with-mount-secret.json` around
lines 200 - 251, The container "ruby-helloworld" is missing a volumeMount for
the defined secret volume "mytest-volume", so add a "volumeMounts" entry inside
the container spec for the "ruby-helloworld" container that mounts
"mytest-volume" at an appropriate path (e.g., /etc/secret or /etc/my-secret)
with "readOnly": true; ensure the "volumeMounts" array is at the same level as
"env", "resources", and "imagePullPolicy" in the "containers" element so the
secret becomes accessible inside the container.
gangwgr
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
gangwgr
force-pushed
the
cli-tests
branch
13 times, most recently
from
9135593 to
fc528a8
Compare
|
/test e2e-aws-ovn-serial-2of2 |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
gangwgr
force-pushed
the
cli-tests
branch
3 times, most recently
from
90d131e to
913e741
Compare
gangwgr
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Just quickly skimmed through and dropped a few comments
test/bindata.mk
Outdated
| @@ -0,0 +1,27 @@ | |||
| # Bindata generation for test fixtures | |||
test/e2e/cli.go
Outdated
| e2e.Logf("sos not found, attempting to install...") | ||
|
|
||
| // Try to install sos using dnf (RHEL/Fedora/CentOS) | ||
| installCmd := exec.Command("sudo", "dnf", "install", "-y", "sos") |
There was a problem hiding this comment.
What about dropping sos related tests?. I think, we should not call dnf, yum, etc. in here.
testdata/oc_cli/OWNERS
Outdated
| @@ -0,0 +1,3 @@ | |||
| approvers: | |||
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/testdata/fixtures.go`:
- Around line 96-99: The os.Open call used to verify readability opens a file
handle that is never closed; update the check in test/testdata/fixtures.go so
that after calling os.Open(targetPath) you immediately capture the returned
*os.File (e.g., f) and ensure f.Close() is called (preferably via defer
f.Close() right after the successful open) or replace the open with
os.Stat(targetPath) if you only need existence/readability info; reference the
os.Open(targetPath) invocation and targetPath variable when making the change.
In `@testdata/oc_cli/dc_66672.yaml`:
- Around line 16-19: The container spec for the entry named "default-container"
under the top-level containers list is missing a securityContext to disable
privilege escalation; update that container's spec (the block containing name:
default-container and resources: {}) to include a securityContext with
allowPrivilegeEscalation: false so the manifest satisfies PSA "restricted"/SCC
checks and prevents privilege escalation.
In `@testdata/oc_cli/stable-storage.yaml`:
- Around line 35-45: The container securityContext for the container named
"hello-pod" only sets capabilities and privileged; add the missing controls by
updating the securityContext for hello-pod to include allowPrivilegeEscalation:
false and runAsNonRoot: true so privilege escalation is disabled and the
container runs as non-root, matching the pattern used in pod_mirror.yaml and
debugpod_48681.yaml.
♻️ Duplicate comments (3)
testdata/oc_cli/rc-match-service.yaml (1)
30-31: Add a volume source forconfigto avoid an invalid RC manifest.The
configvolume has only a name and no source, so the manifest will be rejected. Please define an appropriate source (e.g., secret/configMap/emptyDir) consistent with the test’s intent.🐛 Proposed fix (example using a secret)
volumes: - name: config + secret: + secretName: mysecrettestdata/oc_cli/deploymentconfig_with_quota.yaml (1)
40-42: Inconsistent memory unit notation already flagged.The memory unit inconsistency between
40M(line 42) and40Mi(line 54) was identified in a previous review. For consistency with Kubernetes conventions, both containers should use mebibytes (Mi).testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
200-251: MissingvolumeMountsfor the secret volume already flagged.The previous review correctly identified that while
mytest-volumeis defined in thevolumessection (lines 244-251), theruby-helloworldcontainer lacks a correspondingvolumeMountsentry to make the secret accessible. Given the filename indicates "with-mount-secret", this appears to be an oversight that should be addressed.
🧹 Nitpick comments (2)
test/testdata/fixtures.go (2)
8-8: Consider replacing deprecatedioutilpackage.
io/ioutilis deprecated since Go 1.16. Replace withospackage equivalents:
ioutil.TempDir→os.MkdirTemp♻️ Suggested fix
import ( "fmt" - "io/ioutil" "os" "path/filepath" "strings" )func init() { var err error - fixtureDir, err = ioutil.TempDir("", "oc-testdata-fixtures-") + fixtureDir, err = os.MkdirTemp("", "oc-testdata-fixtures-") if err != nil { panic(fmt.Sprintf("failed to create fixture directory: %v", err)) } }Also applies to: 18-18
82-90: Walk errors are silently ignored.The
filepath.Walkreturn value is discarded. If anyChmodfails, the error is lost. However, the readability check at line 97 provides a safety net.Consider logging or returning the error for debugging purposes, or explicitly ignore it with
_ =to indicate intent.
| containers: | ||
| - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 | ||
| name: default-container | ||
| resources: {} |
There was a problem hiding this comment.
Add securityContext.allowPrivilegeEscalation: false to meet restricted policies
This manifest can be rejected under PSA “restricted” (and similar SCC policies) without explicitly disabling privilege escalation, which can break CLI tests in locked-down clusters. Consider setting it on the container.
✅ Suggested fix
containers:
- image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3
name: default-container
+ securityContext:
+ allowPrivilegeEscalation: false
resources: {}📝 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.
| containers: | |
| - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 | |
| name: default-container | |
| resources: {} | |
| containers: | |
| - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 | |
| name: default-container | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| resources: {} |
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 1-19: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
🤖 Prompt for AI Agents
In `@testdata/oc_cli/dc_66672.yaml` around lines 16 - 19, The container spec for
the entry named "default-container" under the top-level containers list is
missing a securityContext to disable privilege escalation; update that
container's spec (the block containing name: default-container and resources:
{}) to include a securityContext with allowPrivilegeEscalation: false so the
manifest satisfies PSA "restricted"/SCC checks and prevents privilege
escalation.
| containers: | ||
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | ||
| imagePullPolicy: IfNotPresent | ||
| name: hello-pod | ||
| ports: | ||
| - containerPort: 8080 | ||
| protocol: TCP | ||
| resources: {} | ||
| securityContext: | ||
| capabilities: {} | ||
| privileged: false |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate and examine the file
git ls-files | grep -E "stable-storage\.yaml"Repository: openshift/oc
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Read the file and check its content around lines 35-45
if [ -f "testdata/oc_cli/stable-storage.yaml" ]; then
echo "=== File exists ==="
wc -l testdata/oc_cli/stable-storage.yaml
echo "=== Lines 30-50 ==="
sed -n '30,50p' testdata/oc_cli/stable-storage.yaml
echo "=== Full file (first 100 lines) ==="
head -100 testdata/oc_cli/stable-storage.yaml
fiRepository: openshift/oc
Length of output: 2289
🏁 Script executed:
#!/bin/bash
# Search for similar security context patterns in testdata to understand conventions
rg "securityContext:" testdata/ -A 5 -B 2Repository: openshift/oc
Length of output: 4367
Add missing security controls to container securityContext.
The container's securityContext only sets privileged: false and capabilities: {}, but omits allowPrivilegeEscalation: false and runAsNonRoot: true. This leaves privilege escalation enabled by default. Other test manifests in the same directory (pod_mirror.yaml, debugpod_48681.yaml) include these controls—align this manifest with that pattern for consistency.
Suggested fix
securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
capabilities: {}
privileged: false📝 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.
| containers: | |
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | |
| imagePullPolicy: IfNotPresent | |
| name: hello-pod | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| resources: {} | |
| securityContext: | |
| capabilities: {} | |
| privileged: false | |
| containers: | |
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | |
| imagePullPolicy: IfNotPresent | |
| name: hello-pod | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| resources: {} | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| capabilities: {} | |
| privileged: false |
🤖 Prompt for AI Agents
In `@testdata/oc_cli/stable-storage.yaml` around lines 35 - 45, The container
securityContext for the container named "hello-pod" only sets capabilities and
privileged; add the missing controls by updating the securityContext for
hello-pod to include allowPrivilegeEscalation: false and runAsNonRoot: true so
privilege escalation is disabled and the container runs as non-root, matching
the pattern used in pod_mirror.yaml and debugpod_48681.yaml.
test/e2e/cli.go
Outdated
| ) | ||
|
|
||
| // checkSSHAccess checks if SSH access to nodes works (required for sos collect) | ||
| func checkSSHAccess(oc *CLI) bool { |
There was a problem hiding this comment.
We can remove all the tests that use this function. They test sos not any command in oc.
test/e2e/cli.go
Outdated
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-Author:yinzhou-High-44797-Could define a Command for DC", func() { | ||
| if isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.11") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") && !isEnabledCapability(oc, "DeploymentConfig") { |
There was a problem hiding this comment.
If this test only works on older OCPs, we don't need this.
There was a problem hiding this comment.
it works in latest release also
There was a problem hiding this comment.
We don't need this check anymore?, we are in 4.22?
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: knarra@redhat.com | ||
| g.It("Longduration-ConnectedOnly-Author:knarra-VMonly-Critical-54787-Run sos report against ocp cluster and verify that it works fine[Serial][Timeout:30m]", func() { |
There was a problem hiding this comment.
Should be removed, not related to oc.
|
|
||
| var fixtureDir string | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
fixtures.go is needed. It provides the FixturePath() function that:
- Extracts embedded test fixtures from bindata.go to a temp directory
- Fixes file permissions (bindata creates files with 0000 permissions)
- Is used 20+ times throughout the test suite
Without it, tests can't access their YAML/JSON fixture files.
I tried without it in ci it picking wrong path
|
In my opinion, instead of adding every test, it would be very beneficial to only add the useful ones for the long-term maintainability and support. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@test/e2e/util.go`:
- Around line 1141-1167: The readiness check in waitForDeploymentPodsToBeReady
is using oc.AdminKubeClient(), which in tests returns dummyKubeClientComplete
(always zeros) and causes the function to succeed immediately; switch the call
to the real kube client (use oc.KubeClient() or the method that returns the real
Kubernetes client instead of oc.AdminKubeClient()) so it queries the actual
deployment, and add a nil-check for deployment.Spec.Replicas before
dereferencing to avoid panics; keep the existing failure logging
(oc.AsAdmin().WithoutNamespace().Run("logs") / "describe") unchanged.
In `@testdata/oc_cli/auth.json`:
- Around line 2-4: The test fixture currently embeds a base64 credential under
the JSON keys "auths" -> "testroute" -> "auth" (value decodes to admin:123456);
remove the hardcoded credential and replace it with a redacted placeholder
(e.g., "REDACTED_AUTH") or an env/test-time injected value, and update the tests
to generate or inject the actual base64 credential at runtime (use the
"auths"/"testroute"/"auth" key to locate and update the fixture and tests that
load it).
In `@testdata/oc_cli/deploymentconfig_with_quota.yaml`:
- Around line 32-55: The containers "hello-openshift" and "hello-openshift2" are
missing an explicit securityContext to prevent privilege escalation; add a
securityContext block under each container spec that sets
allowPrivilegeEscalation: false (i.e., add securityContext: {
allowPrivilegeEscalation: false } for both container entries) so PSA/SCC will
not reject the pods.
In `@testdata/oc_cli/icspFile63855.yaml`:
- Around line 7-12: Replace the ephemeral EC2 hostname used in the mirror
entries with a stable placeholder (e.g., localhost:5000 or
registry.example.com:5000): update both mirror URL occurrences under the mirrors
array (the lines containing
ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release) to use the
chosen placeholder so the test fixture in icspFile63855.yaml no longer depends
on a transient AWS hostname.
♻️ Duplicate comments (6)
testdata/oc_cli/rc-match-service.yaml (1)
30-31: Incomplete volume definition will fail Kubernetes validation.The
configvolume is missing a volume source (e.g.,secret,configMap,emptyDir). Compare withonly-rc.yamlwhich correctly specifies:volumes: - name: config secret: secretName: mysecretThis manifest will be rejected by Kubernetes when applied during tests.
testdata/oc_cli/dc_66672.yaml (1)
16-19: AddsecurityContext.allowPrivilegeEscalation: falsefor restricted/SCC compatibility.testdata/oc_cli/stable-storage.yaml (1)
43-45: Security context hardening was previously flagged.The missing
allowPrivilegeEscalation: falseandrunAsNonRoot: truein the container's securityContext was already identified in a prior review. Please refer to that comment for the suggested fix.testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
200-251: Missing volumeMounts for secret was previously flagged.The
mytest-volumesecret volume is defined but not mounted in theruby-helloworldcontainer. This issue was already identified in a prior review—please refer to that comment for the proposed fix addingvolumeMountsto make the secret accessible inside the container.test/e2e/util.go (1)
1044-1061: Defer placement should be immediately after successful HTTP response check.The
defer res.Body.Close()is placed afterioutil.ReadAll, which is non-idiomatic. Move it immediately after confirminghttp.Getsucceeded.🐛 Recommended fix
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() + body, err := io.ReadAll(res.Body) if err != nil { e2e.Failf("unable to parse the http result with error: %v", err) }test/testdata/fixtures.go (1)
96-99: File handle leak in readability check.The file opened at line 97 is never closed, which can cause resource exhaustion or file locking issues on Windows.
🐛 Proposed fix
// Double-check the file is now readable - if _, err := os.Open(targetPath); err != nil { + f, err := os.Open(targetPath) + if err != nil { panic(fmt.Sprintf("file %s exists but cannot be opened: %v", targetPath, err)) } + f.Close()
🧹 Nitpick comments (5)
testdata/oc_cli/initContainer.yaml (1)
8-23: Verify Pod security compatibility for restricted clusters.If these fixtures are applied in clusters enforcing PSA “restricted”, missing
allowPrivilegeEscalation: false(and potentially other constraints) can cause admission failures. Consider adding explicit securityContext settings or confirm tests run in permissive SCCs only.💡 Minimal hardening (if PSA compatibility is required)
initContainers: - name: wait image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c command: ["/bin/sh", "-c", "sleep 10"] + securityContext: + allowPrivilegeEscalation: false containers: - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" imagePullPolicy: IfNotPresent name: hello-pod @@ securityContext: capabilities: {} privileged: false + allowPrivilegeEscalation: falsetest/e2e/util.go (3)
1-35: Consider replacing deprecatedio/ioutilwithioandospackages.The
io/ioutilpackage has been deprecated since Go 1.16. Usages throughout this file (ioutil.ReadAll,ioutil.ReadDir,ioutil.ReadFile,ioutil.WriteFile) should be migrated to their equivalents inioandospackages.
623-631: Remove duplicategetRandomStringfunction.This function duplicates the exported
GetRandomString(lines 364-372). All callers should use the exported version instead.♻️ Suggested removal
-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) -}Then update any callers to use
GetRandomString()instead.
1160-1166: Avoid failing the test if diagnostic logging commands fail.If the deployment wait times out, the diagnostic
logsanddescribecommands shouldn't cause additional test failures viao.Expect(err).NotTo(o.HaveOccurred()). This could mask the actual deployment failure reason.♻️ Suggested fix
if err != nil { - err = oc.AsAdmin().WithoutNamespace().Run("logs").Args("--tail", "15", "deployment/"+name, "-n", namespace).Execute() - o.Expect(err).NotTo(o.HaveOccurred()) - err = oc.AsAdmin().WithoutNamespace().Run("describe").Args("deployment/"+name, "-n", namespace).Execute() - o.Expect(err).NotTo(o.HaveOccurred()) + // Log diagnostics but don't fail if these commands error + _ = oc.AsAdmin().WithoutNamespace().Run("logs").Args("--tail", "15", "deployment/"+name, "-n", namespace).Execute() + _ = oc.AsAdmin().WithoutNamespace().Run("describe").Args("deployment/"+name, "-n", namespace).Execute() e2e.Failf("deployment %s is not availabile", name) }test/testdata/fixtures.go (1)
6-22: Replace deprecatedioutilwithospackage.
ioutil.TempDiris deprecated since Go 1.16. Useos.MkdirTempinstead.♻️ Suggested fix
import ( "fmt" - "io/ioutil" "os" "path/filepath" "strings" ) var fixtureDir string func init() { var err error - fixtureDir, err = ioutil.TempDir("", "oc-testdata-fixtures-") + fixtureDir, err = os.MkdirTemp("", "oc-testdata-fixtures-") if err != nil { panic(fmt.Sprintf("failed to create fixture directory: %v", err)) } }
| // WaitForDeploymentPodsToBeReady waits for the specific deployment to be ready | ||
| func waitForDeploymentPodsToBeReady(oc *CLI, namespace string, name string) { | ||
| e2e.Logf("Waiting for deployment %s to be ready in namespace %s", name, namespace) | ||
| err := wait.Poll(20*time.Second, 300*time.Second, func() (done bool, err error) { | ||
| deployment, err := oc.AdminKubeClient().AppsV1().Deployments(namespace).Get(context.Background(), name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| e2e.Logf("Waiting for availability of deployment/%s\n", name) | ||
| return false, nil | ||
| } | ||
| return false, err | ||
| } | ||
| if deployment.Status.AvailableReplicas == *deployment.Spec.Replicas && deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas { | ||
| e2e.Logf("Deployment %s available (%d/%d)\n", name, deployment.Status.AvailableReplicas, *deployment.Spec.Replicas) | ||
| return true, nil | ||
| } | ||
| e2e.Logf("Waiting for full availability of %s deployment (%d/%d)\n", name, deployment.Status.AvailableReplicas, *deployment.Spec.Replicas) | ||
| return false, nil | ||
| }) | ||
| if err != nil { | ||
| err = oc.AsAdmin().WithoutNamespace().Run("logs").Args("--tail", "15", "deployment/"+name, "-n", namespace).Execute() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| err = oc.AsAdmin().WithoutNamespace().Run("describe").Args("deployment/"+name, "-n", namespace).Execute() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| e2e.Failf("deployment %s is not availabile", name) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check usage of waitForDeploymentPodsToBeReady in the codebase
rg -n "waitForDeploymentPodsToBeReady" --type goRepository: openshift/oc
Length of output: 242
🏁 Script executed:
# Also check if there's an exported version (capitalized)
rg -n "WaitForDeploymentPodsToBeReady" --type goRepository: openshift/oc
Length of output: 157
🏁 Script executed:
# Examine the dummy client implementation around lines 1459-1468
sed -n '1450,1475p' test/e2e/util.go | cat -nRepository: openshift/oc
Length of output: 1061
🏁 Script executed:
# Check AdminKubeClient implementation
rg -n "func.*AdminKubeClient" --type go -A 3Repository: openshift/oc
Length of output: 365
🏁 Script executed:
# Look at the test in cli.go around line 1777
sed -n '1765,1790p' test/e2e/cli.go | cat -nRepository: openshift/oc
Length of output: 1481
🏁 Script executed:
# Also check a broader context to understand what test this is
sed -n '1750,1800p' test/e2e/cli.go | cat -nRepository: openshift/oc
Length of output: 2847
🏁 Script executed:
# Let me look at the actual full function definition to confirm line 1167 end
sed -n '1142,1170p' test/e2e/util.go | cat -nRepository: openshift/oc
Length of output: 1737
🏁 Script executed:
# Check if UpdatedReplicas is set in the dummyDeploymentStatus struct definition
rg -n "type dummyDeploymentStatus" -A 5 --type goRepository: openshift/oc
Length of output: 330
waitForDeploymentPodsToBeReady bypasses actual deployment readiness check using a dummy client.
The test at cli.go:1777 creates a real deployment via oc new-app, but the function then calls oc.AdminKubeClient() which returns a dummyKubeClientComplete that always returns hardcoded zero values (Replicas: 0, AvailableReplicas: 0, UpdatedReplicas: 0). The readiness condition AvailableReplicas == *Replicas && UpdatedReplicas == *Replicas evaluates to 0 == 0 && 0 == 0, returning true on the first poll without actually verifying the real deployment is ready. This causes the test to proceed immediately without waiting for the actual deployment, creating risk of test flakiness and race conditions.
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 1141 - 1167, The readiness check in
waitForDeploymentPodsToBeReady is using oc.AdminKubeClient(), which in tests
returns dummyKubeClientComplete (always zeros) and causes the function to
succeed immediately; switch the call to the real kube client (use
oc.KubeClient() or the method that returns the real Kubernetes client instead of
oc.AdminKubeClient()) so it queries the actual deployment, and add a nil-check
for deployment.Spec.Replicas before dereferencing to avoid panics; keep the
existing failure logging (oc.AsAdmin().WithoutNamespace().Run("logs") /
"describe") unchanged.
| "auths": { | ||
| "testroute": { | ||
| "auth": "YWRtaW46MTIzNDU2" |
There was a problem hiding this comment.
Remove embedded credentials from the test fixture.
Line 4 decodes to admin:123456; committing credentials in-repo is a security risk and will keep tripping secret scanners. Prefer generating this at test runtime or storing a redacted placeholder and injecting the value during tests.
🔧 Suggested change (with test-side injection)
- "auth": "YWRtaW46MTIzNDU2"
+ "auth": "REDACTED"📝 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.
| "auths": { | |
| "testroute": { | |
| "auth": "YWRtaW46MTIzNDU2" | |
| "auths": { | |
| "testroute": { | |
| "auth": "REDACTED" |
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In `@testdata/oc_cli/auth.json` around lines 2 - 4, The test fixture currently
embeds a base64 credential under the JSON keys "auths" -> "testroute" -> "auth"
(value decodes to admin:123456); remove the hardcoded credential and replace it
with a redacted placeholder (e.g., "REDACTED_AUTH") or an env/test-time injected
value, and update the tests to generate or inject the actual base64 credential
at runtime (use the "auths"/"testroute"/"auth" key to locate and update the
fixture and tests that load it).
| containers: | ||
| - image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83 | ||
| imagePullPolicy: Always | ||
| name: hello-openshift | ||
| resources: | ||
| limits: | ||
| cpu: 60m | ||
| memory: 60Mi | ||
| requests: | ||
| cpu: 50m | ||
| memory: 40M | ||
| terminationMessagePath: /dev/termination-log | ||
| terminationMessagePolicy: File | ||
| - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 | ||
| imagePullPolicy: Always | ||
| name: hello-openshift2 | ||
| resources: | ||
| limits: | ||
| cpu: 60m | ||
| memory: 60Mi | ||
| requests: | ||
| cpu: 50m | ||
| memory: 40Mi | ||
| terminationMessagePath: /dev/termination-log |
There was a problem hiding this comment.
Set allowPrivilegeEscalation: false on both containers to avoid PSA/SCC rejections.
🔧 Suggested fix
- image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83
imagePullPolicy: Always
name: hello-openshift
+ securityContext:
+ allowPrivilegeEscalation: false
resources:
limits:
cpu: 60m
memory: 60Mi
@@
- image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3
imagePullPolicy: Always
name: hello-openshift2
+ securityContext:
+ allowPrivilegeEscalation: false
resources:
limits:
cpu: 60m
memory: 60Mi📝 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.
| containers: | |
| - image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83 | |
| imagePullPolicy: Always | |
| name: hello-openshift | |
| resources: | |
| limits: | |
| cpu: 60m | |
| memory: 60Mi | |
| requests: | |
| cpu: 50m | |
| memory: 40M | |
| terminationMessagePath: /dev/termination-log | |
| terminationMessagePolicy: File | |
| - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 | |
| imagePullPolicy: Always | |
| name: hello-openshift2 | |
| resources: | |
| limits: | |
| cpu: 60m | |
| memory: 60Mi | |
| requests: | |
| cpu: 50m | |
| memory: 40Mi | |
| terminationMessagePath: /dev/termination-log | |
| containers: | |
| - image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83 | |
| imagePullPolicy: Always | |
| name: hello-openshift | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| resources: | |
| limits: | |
| cpu: 60m | |
| memory: 60Mi | |
| requests: | |
| cpu: 50m | |
| memory: 40M | |
| terminationMessagePath: /dev/termination-log | |
| terminationMessagePolicy: File | |
| - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 | |
| imagePullPolicy: Always | |
| name: hello-openshift2 | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| resources: | |
| limits: | |
| cpu: 60m | |
| memory: 60Mi | |
| requests: | |
| cpu: 50m | |
| memory: 40Mi | |
| terminationMessagePath: /dev/termination-log |
🤖 Prompt for AI Agents
In `@testdata/oc_cli/deploymentconfig_with_quota.yaml` around lines 32 - 55, The
containers "hello-openshift" and "hello-openshift2" are missing an explicit
securityContext to prevent privilege escalation; add a securityContext block
under each container spec that sets allowPrivilegeEscalation: false (i.e., add
securityContext: { allowPrivilegeEscalation: false } for both container entries)
so PSA/SCC will not reject the pods.
| - mirrors: | ||
| - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release | ||
| source: quay.io/openshift-release-dev/ocp-v4.0-art-dev | ||
| - mirrors: | ||
| - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release | ||
| source: registry.ci.openshift.org/ocp/release |
There was a problem hiding this comment.
Replace hardcoded EC2 hostname with a stable placeholder.
The mirror URLs reference an ephemeral AWS EC2 public hostname (ec2-3-133-161-121.us-east-2.compute.amazonaws.com), which will become invalid when the instance terminates. For test fixtures, use localhost:5000 or a placeholder like registry.example.com:5000 to ensure tests remain portable and don't depend on transient infrastructure.
Proposed fix
repositoryDigestMirrors:
- mirrors:
- - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release
+ - localhost:5000/ocp/release
source: quay.io/openshift-release-dev/ocp-v4.0-art-dev
- mirrors:
- - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release
+ - localhost:5000/ocp/release
source: registry.ci.openshift.org/ocp/release📝 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.
| - mirrors: | |
| - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release | |
| source: quay.io/openshift-release-dev/ocp-v4.0-art-dev | |
| - mirrors: | |
| - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release | |
| source: registry.ci.openshift.org/ocp/release | |
| - mirrors: | |
| - localhost:5000/ocp/release | |
| source: quay.io/openshift-release-dev/ocp-v4.0-art-dev | |
| - mirrors: | |
| - localhost:5000/ocp/release | |
| source: registry.ci.openshift.org/ocp/release |
🤖 Prompt for AI Agents
In `@testdata/oc_cli/icspFile63855.yaml` around lines 7 - 12, Replace the
ephemeral EC2 hostname used in the mirror entries with a stable placeholder
(e.g., localhost:5000 or registry.example.com:5000): update both mirror URL
occurrences under the mirrors array (the lines containing
ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release) to use the
chosen placeholder so the test fixture in icspFile63855.yaml no longer depends
on a transient AWS hostname.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/testdata/fixtures.go`:
- Around line 13-29: The init() function assumes runtime.Caller(0) returns an
absolute filesystem path; add a check after obtaining filename from
runtime.Caller to verify filepath.IsAbs(filename) and if not, call os.Getwd()
and resolve filename by joining the working directory with filename or by
walking upward from the working dir to locate the "testdata" directory; update
the logic that computes repoRoot and testdataDir (the variables in init and the
runtime.Caller call) to use the resolved absolute path or the discovered
testdata directory as a fallback so the os.Stat check no longer panics under
-trimpath builds.
| func init() { | ||
| // Get the directory where this source file is located | ||
| _, filename, _, ok := runtime.Caller(0) | ||
| if !ok { | ||
| panic("failed to get current file path") | ||
| } | ||
|
|
||
| // This file is in test/testdata/fixtures.go | ||
| // We want to get the repo root and then point to testdata/ | ||
| testTestdataDir := filepath.Dir(filename) | ||
| repoRoot := filepath.Dir(filepath.Dir(testTestdataDir)) // Go up two levels: test/testdata -> test -> repo root | ||
| testdataDir = filepath.Join(repoRoot, "testdata") | ||
|
|
||
| // Verify the testdata directory exists | ||
| if _, err := os.Stat(testdataDir); os.IsNotExist(err) { | ||
| panic(fmt.Sprintf("testdata directory not found at %s", testdataDir)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for -trimpath usage in build/test configs and GOFLAGS
rg -i 'trimpath|GOFLAGS' --type-list | head -20
echo "---"
rg -i 'trimpath' . --type go --type sh --type yaml --type toml --type json -A 2Repository: openshift/oc
Length of output: 1936
🏁 Script executed:
#!/bin/bash
# Examine the full fixtures.go file to see context and usage
cat -n test/testdata/fixtures.goRepository: openshift/oc
Length of output: 2042
🏁 Script executed:
#!/bin/bash
# Search for usage of testdataDir variable across the codebase
rg 'testdataDir' -A 3 -B 1Repository: openshift/oc
Length of output: 1647
Check that runtime.Caller returns an absolute path; provide fallback for -trimpath builds.
At lines 15–24, runtime.Caller(0) is assumed to return an absolute filesystem path. With -trimpath (used in release/CI builds), it can return a module path or relative path, making subsequent filepath.Dir() calls produce invalid results and triggering a panic at line 27. Add an early check to validate the path is absolute, with a fallback that searches from os.Getwd() when it is not.
🔧 Corrected approach (robust against -trimpath)
func init() {
// Get the directory where this source file is located
_, filename, _, ok := runtime.Caller(0)
if !ok {
panic("failed to get current file path")
}
+ // -trimpath may return a module path instead of absolute path; check and fall back if needed
+ if !filepath.IsAbs(filename) {
+ if wd, err := os.Getwd(); err == nil {
+ dir := wd
+ for {
+ candidate := filepath.Join(dir, "testdata")
+ if _, err := os.Stat(candidate); err == nil {
+ testdataDir = candidate
+ return
+ }
+ parent := filepath.Dir(dir)
+ if parent == dir {
+ break
+ }
+ dir = parent
+ }
+ }
+ panic(fmt.Sprintf("testdata directory not found: runtime.Caller returned non-absolute path %q", filename))
+ }
+
// This file is in test/testdata/fixtures.go
// We want to get the repo root and then point to testdata/
testTestdataDir := filepath.Dir(filename)
repoRoot := filepath.Dir(filepath.Dir(testTestdataDir))
testdataDir = filepath.Join(repoRoot, "testdata")
// Verify the testdata directory exists
if _, err := os.Stat(testdataDir); os.IsNotExist(err) {
panic(fmt.Sprintf("testdata directory not found at %s", testdataDir))
}
}📝 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.
| func init() { | |
| // Get the directory where this source file is located | |
| _, filename, _, ok := runtime.Caller(0) | |
| if !ok { | |
| panic("failed to get current file path") | |
| } | |
| // This file is in test/testdata/fixtures.go | |
| // We want to get the repo root and then point to testdata/ | |
| testTestdataDir := filepath.Dir(filename) | |
| repoRoot := filepath.Dir(filepath.Dir(testTestdataDir)) // Go up two levels: test/testdata -> test -> repo root | |
| testdataDir = filepath.Join(repoRoot, "testdata") | |
| // Verify the testdata directory exists | |
| if _, err := os.Stat(testdataDir); os.IsNotExist(err) { | |
| panic(fmt.Sprintf("testdata directory not found at %s", testdataDir)) | |
| } | |
| func init() { | |
| // Get the directory where this source file is located | |
| _, filename, _, ok := runtime.Caller(0) | |
| if !ok { | |
| panic("failed to get current file path") | |
| } | |
| // -trimpath may return a module path instead of absolute path; check and fall back if needed | |
| if !filepath.IsAbs(filename) { | |
| if wd, err := os.Getwd(); err == nil { | |
| dir := wd | |
| for { | |
| candidate := filepath.Join(dir, "testdata") | |
| if _, err := os.Stat(candidate); err == nil { | |
| testdataDir = candidate | |
| return | |
| } | |
| parent := filepath.Dir(dir) | |
| if parent == dir { | |
| break | |
| } | |
| dir = parent | |
| } | |
| } | |
| panic(fmt.Sprintf("testdata directory not found: runtime.Caller returned non-absolute path %q", filename)) | |
| } | |
| // This file is in test/testdata/fixtures.go | |
| // We want to get the repo root and then point to testdata/ | |
| testTestdataDir := filepath.Dir(filename) | |
| repoRoot := filepath.Dir(filepath.Dir(testTestdataDir)) // Go up two levels: test/testdata -> test -> repo root | |
| testdataDir = filepath.Join(repoRoot, "testdata") | |
| // Verify the testdata directory exists | |
| if _, err := os.Stat(testdataDir); os.IsNotExist(err) { | |
| panic(fmt.Sprintf("testdata directory not found at %s", testdataDir)) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@test/testdata/fixtures.go` around lines 13 - 29, The init() function assumes
runtime.Caller(0) returns an absolute filesystem path; add a check after
obtaining filename from runtime.Caller to verify filepath.IsAbs(filename) and if
not, call os.Getwd() and resolve filename by joining the working directory with
filename or by walking upward from the working dir to locate the "testdata"
directory; update the logic that computes repoRoot and testdataDir (the
variables in init and the runtime.Caller call) to use the resolved absolute path
or the discovered testdata directory as a fallback so the os.Stat check no
longer panics under -trimpath builds.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/testdata/fixtures.go`:
- Around line 73-88: ListFixtures currently calls filepath.Walk on oc_cliDir and
ignores the returned error, which can hide permission/symlink issues; update the
call to capture and handle the error from filepath.Walk (and propagate or fail
the test) — for example, assign the return value of filepath.Walk to a variable,
check if err != nil and then either log/panic or return an error/empty result
consistently; also ensure errors from filepath.Rel are handled instead of being
discarded so that ListFixtures, oc_cliDir and the Walk callback surface failures
instead of silently returning partial results.
test/testdata/fixtures.go
Outdated
| func ListFixtures() []string { | ||
| var fixtures []string | ||
| oc_cliDir := filepath.Join(fixtureDir, "oc_cli") | ||
|
|
||
| if _, err := os.Stat(oc_cliDir); err == nil { | ||
| filepath.Walk(oc_cliDir, func(path string, info fs.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !info.IsDir() { | ||
| relPath, _ := filepath.Rel(fixtureDir, path) | ||
| fixtures = append(fixtures, relPath) | ||
| } | ||
| return nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
Error from filepath.Walk is silently ignored.
If Walk encounters an error (permission denied, broken symlinks, etc.), the function returns partial or empty results with no indication of failure. This could cause hard-to-debug test issues.
🐛 Proposed fix
// ListFixtures returns all available fixture paths
-func ListFixtures() []string {
+func ListFixtures() ([]string, error) {
var fixtures []string
oc_cliDir := filepath.Join(fixtureDir, "oc_cli")
if _, err := os.Stat(oc_cliDir); err == nil {
- filepath.Walk(oc_cliDir, func(path string, info fs.FileInfo, err error) error {
+ err := filepath.Walk(oc_cliDir, func(path string, info fs.FileInfo, err error) error {
if err != nil {
return err
}
if !info.IsDir() {
relPath, _ := filepath.Rel(fixtureDir, path)
fixtures = append(fixtures, relPath)
}
return nil
})
+ if err != nil {
+ return nil, fmt.Errorf("failed to list fixtures: %w", err)
+ }
}
- return fixtures
+ return fixtures, nil
}Alternatively, if changing the signature would require updates elsewhere, at minimum log or panic on Walk errors to surface issues during test development.
🤖 Prompt for AI Agents
In `@test/testdata/fixtures.go` around lines 73 - 88, ListFixtures currently calls
filepath.Walk on oc_cliDir and ignores the returned error, which can hide
permission/symlink issues; update the call to capture and handle the error from
filepath.Walk (and propagate or fail the test) — for example, assign the return
value of filepath.Walk to a variable, check if err != nil and then either
log/panic or return an error/empty result consistently; also ensure errors from
filepath.Rel are handled instead of being discarded so that ListFixtures,
oc_cliDir and the Walk callback surface failures instead of silently returning
partial results.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/testdata/oc_cli/idmsFile63855.yaml`:
- Around line 7-12: The mirror entries in idmsFile63855.yaml include a hardcoded
ephemeral EC2 hostname (ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000)
which breaks tests; update the two mirror URLs under the "mirrors" arrays (the
lines containing
ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release) to use the
stable placeholder "localhost:5000/ocp/release" so they match the pattern used
in idmsFile64921.yaml and ensure test portability.
In `@test/testdata/oc_cli/initContainer.yaml`:
- Around line 8-27: Add explicit non-escalation and non-root constraints to both
the init container "wait" (initContainers) and the app container "hello-pod"
(containers) by setting securityContext.allowPrivilegeEscalation: false and
securityContext.runAsNonRoot: true for each container; ensure you add or extend
the existing securityContext block under the initContainers entry and the
containers entry (for "hello-pod") rather than only setting Pod-level fields so
the containers comply with PSA/SCC restricted policies.
In `@test/testdata/oc_cli/only-rc.yaml`:
- Around line 14-27: The container spec for the replication controller (the
container with name "origin" and image "quay.io/openshifttest/base-alpine@...")
lacks a securityContext required for restricted PSA/SCC clusters; update the
container entry to include a securityContext that sets allowPrivilegeEscalation:
false and runAsNonRoot: true so the RC is accepted under restricted policies.
♻️ Duplicate comments (8)
test/testdata/oc_cli/auth.json (1)
2-4: Remove embedded credentials from the fixture.Hardcoding auth data in-repo is a security/compliance risk and will keep triggering secret scanners. Use a redacted placeholder and inject the real value at test runtime.
🔧 Suggested change
- "auth": "YWRtaW46MTIzNDU2" + "auth": "REDACTED_AUTH"test/testdata/oc_cli/icspFile63855.yaml (1)
7-12: Hardcoded EC2 hostname should be replaced with a stable placeholder.This issue was previously identified. The ephemeral AWS EC2 hostname will become invalid when the instance terminates. Use
localhost:5000for consistency with other fixtures in this PR.test/testdata/oc_cli/rc-match-service.yaml (1)
30-31: Incomplete volume definition will fail validation.The
configvolume only has anamebut no volume source. Kubernetes requires a source such assecret,configMap, oremptyDir.test/testdata/oc_cli/stable-storage.yaml (1)
35-45: Add allowPrivilegeEscalation/runAsNonRoot in container securityContext.
Same PSA/SCC compatibility concern as previously noted.✅ Suggested fix
securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true capabilities: {} privileged: falsetest/testdata/oc_cli/dc_66672.yaml (1)
16-19: Disable privilege escalation for the container.
This is required for PSA/SCC “restricted” compliance; same concern as previously noted.✅ Suggested fix
- image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 name: default-container + securityContext: + allowPrivilegeEscalation: false resources: {}test/testdata/oc_cli/deploymentconfig_with_quota.yaml (1)
32-55: Add allowPrivilegeEscalation: false to both containers.
Same PSA/SCC compatibility concern as previously noted.✅ Suggested fix
- image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83 imagePullPolicy: Always name: hello-openshift + securityContext: + allowPrivilegeEscalation: false resources: limits: cpu: 60m memory: 60Mi @@ - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 imagePullPolicy: Always name: hello-openshift2 + securityContext: + allowPrivilegeEscalation: false resources: limits: cpu: 60m memory: 60Mitest/testdata/fixtures.go (1)
117-128: Error fromfs.WalkDiris silently ignored.
ListFixturesdiscards errors returned by the walk callback. If the embedded FS has issues, the function returns partial results with no indication of failure, making test debugging difficult.🐛 Proposed fix
// ListFixtures returns all available fixture paths -func ListFixtures() []string { +func ListFixtures() ([]string, error) { 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 }) - return fixtures + if err != nil { + return nil, fmt.Errorf("failed to list fixtures: %w", err) + } + return fixtures, nil }test/testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
200-251: MissingvolumeMountsfor the secret volume.The
volumessection definesmytest-volumebacked by secretmy-secret, but containerruby-helloworldlacks avolumeMountsentry. Given the template name emphasizes "with-mount-secret", this appears unintentional.🐛 Proposed fix
"resources": {}, "terminationMessagePath": "/dev/termination-log", "imagePullPolicy": "IfNotPresent", + "volumeMounts": [ + { + "name": "mytest-volume", + "mountPath": "/etc/secret-volume", + "readOnly": true + } + ], "securityContext": {
🧹 Nitpick comments (1)
test/testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
244-251: Inconsistent indentation in volumes block.The
volumesarray uses tabs while the rest of the file uses spaces, making the JSON harder to read and maintain.
| - mirrors: | ||
| - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release | ||
| source: quay.io/openshift-release-dev/ocp-v4.0-art-dev | ||
| - mirrors: | ||
| - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release | ||
| source: registry.ci.openshift.org/ocp/release |
There was a problem hiding this comment.
Replace hardcoded EC2 hostname with a stable placeholder.
The mirror URLs reference an ephemeral AWS EC2 public hostname that will become invalid. For consistency with idmsFile64921.yaml and test portability, use localhost:5000.
Proposed fix
imageDigestMirrors:
- mirrors:
- - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release
+ - localhost:5000/ocp/release
source: quay.io/openshift-release-dev/ocp-v4.0-art-dev
- mirrors:
- - ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release
+ - localhost:5000/ocp/release
source: registry.ci.openshift.org/ocp/release🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/idmsFile63855.yaml` around lines 7 - 12, The mirror
entries in idmsFile63855.yaml include a hardcoded ephemeral EC2 hostname
(ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000) which breaks tests;
update the two mirror URLs under the "mirrors" arrays (the lines containing
ec2-3-133-161-121.us-east-2.compute.amazonaws.com:5000/ocp/release) to use the
stable placeholder "localhost:5000/ocp/release" so they match the pattern used
in idmsFile64921.yaml and ensure test portability.
| initContainers: | ||
| - name: wait | ||
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | ||
| command: ["/bin/sh", "-c", "sleep 10"] | ||
| containers: | ||
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | ||
| imagePullPolicy: IfNotPresent | ||
| name: hello-pod | ||
| ports: | ||
| - containerPort: 8080 | ||
| protocol: TCP | ||
| resources: {} | ||
| securityContext: | ||
| capabilities: {} | ||
| privileged: false | ||
| terminationMessagePath: /dev/termination-log | ||
| volumeMounts: | ||
| - mountPath: /tmp | ||
| name: tmp | ||
| dnsPolicy: ClusterFirst |
There was a problem hiding this comment.
Harden init and app containers for restricted policies.
Both containers lack allowPrivilegeEscalation: false and runAsNonRoot: true, which can fail under PSA/SCC “restricted”.
✅ Suggested fix
initContainers:
- name: wait
image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c
command: ["/bin/sh", "-c", "sleep 10"]
+ securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
containers:
- image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83"
imagePullPolicy: IfNotPresent
name: hello-pod
ports:
- containerPort: 8080
protocol: TCP
resources: {}
securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
capabilities: {}
privileged: false📝 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.
| initContainers: | |
| - name: wait | |
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | |
| command: ["/bin/sh", "-c", "sleep 10"] | |
| containers: | |
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | |
| imagePullPolicy: IfNotPresent | |
| name: hello-pod | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| resources: {} | |
| securityContext: | |
| capabilities: {} | |
| privileged: false | |
| terminationMessagePath: /dev/termination-log | |
| volumeMounts: | |
| - mountPath: /tmp | |
| name: tmp | |
| dnsPolicy: ClusterFirst | |
| initContainers: | |
| - name: wait | |
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | |
| command: ["/bin/sh", "-c", "sleep 10"] | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| containers: | |
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | |
| imagePullPolicy: IfNotPresent | |
| name: hello-pod | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| resources: {} | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| capabilities: {} | |
| privileged: false | |
| terminationMessagePath: /dev/termination-log | |
| volumeMounts: | |
| - mountPath: /tmp | |
| name: tmp | |
| dnsPolicy: ClusterFirst |
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/initContainer.yaml` around lines 8 - 27, Add explicit
non-escalation and non-root constraints to both the init container "wait"
(initContainers) and the app container "hello-pod" (containers) by setting
securityContext.allowPrivilegeEscalation: false and
securityContext.runAsNonRoot: true for each container; ensure you add or extend
the existing securityContext block under the initContainers entry and the
containers entry (for "hello-pod") rather than only setting Pod-level fields so
the containers comply with PSA/SCC restricted policies.
| containers: | ||
| - args: | ||
| - start | ||
| - master | ||
| - --config=/config/master-config.yaml | ||
| image: "quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c" | ||
| name: origin | ||
| ports: | ||
| - containerPort: 8443 | ||
| name: openshift | ||
| volumeMounts: | ||
| - mountPath: /config | ||
| name: config | ||
| readOnly: true |
There was a problem hiding this comment.
Add container securityContext for restricted PSA/SCC compatibility.
Without allowPrivilegeEscalation: false and runAsNonRoot: true, this RC can be rejected on restricted clusters, causing CLI tests to fail.
✅ Suggested fix
containers:
- args:
- start
- master
- --config=/config/master-config.yaml
image: "quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c"
name: origin
+ securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
ports:
- containerPort: 8443
name: openshift📝 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.
| containers: | |
| - args: | |
| - start | |
| - master | |
| - --config=/config/master-config.yaml | |
| image: "quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c" | |
| name: origin | |
| ports: | |
| - containerPort: 8443 | |
| name: openshift | |
| volumeMounts: | |
| - mountPath: /config | |
| name: config | |
| readOnly: true | |
| containers: | |
| - args: | |
| - start | |
| - master | |
| - --config=/config/master-config.yaml | |
| image: "quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c" | |
| name: origin | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| ports: | |
| - containerPort: 8443 | |
| name: openshift | |
| volumeMounts: | |
| - mountPath: /config | |
| name: config | |
| readOnly: true |
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/only-rc.yaml` around lines 14 - 27, The container spec
for the replication controller (the container with name "origin" and image
"quay.io/openshifttest/base-alpine@...") lacks a securityContext required for
restricted PSA/SCC clusters; update the container entry to include a
securityContext that sets allowPrivilegeEscalation: false and runAsNonRoot: true
so the RC is accepted under restricted policies.
|
/assign @ardaguclu |
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Dropped more comments. Also could you please remove the unused files under testdata directory.
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-Longduration-ConnectedOnly-NonPreRelease-Author:yinzhou-High-43032-oc adm release mirror generating correct imageContentSources when using --to and --to-release-image", func() { | ||
| if checkProxy(oc) { |
There was a problem hiding this comment.
Can we remove this check at all?. We don't run behind proxy?
There was a problem hiding this comment.
Yes, but these cases runs in conformance suite also, if those run proxy based cluster then it will fail
test/e2e/cli.go
Outdated
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-Author:yinzhou-High-44797-Could define a Command for DC", func() { | ||
| if isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.11") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") && !isEnabledCapability(oc, "DeploymentConfig") { |
There was a problem hiding this comment.
We don't need this check anymore?, we are in 4.22?
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-High-43034-should not show signature verify error msgs while trying to mirror OCP image repository to [Flaky]", func() { | ||
| if checkProxy(oc) { |
There was a problem hiding this comment.
Should be removed with all the others?
test/e2e/cli.go
Outdated
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("Author:yinzhou-Medium-33648-must gather pod should not schedule on windows node", func() { |
There was a problem hiding this comment.
This test is not run on Windows already?, can be removed?
| } | ||
|
|
||
| g.By("Create test pod") | ||
| pod48681.createDebugPodUsingDefinition(oc) |
There was a problem hiding this comment.
What are we asserting here?. We don't check the returned value?. Can we remove this test?
There was a problem hiding this comment.
The assertions are hidden inside the helper function
// The helper function verifies:
// 1. Pod reaches Running state
// 2. oc debug -f command succeeds
// 3. Debug output contains "Starting pod/pod48681-debug"
test/e2e/cli.go
Outdated
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(os.WriteFile(dockerTmpDirName, authContentBytes, 0640)).NotTo(o.HaveOccurred()) | ||
|
|
||
| //_, outErr, err := oc.WithoutNamespace().WithoutKubeconf().Run("adm").Args("release", "info", image).Outputs() |
There was a problem hiding this comment.
Can we remove these comments?
test/e2e/cli.go
Outdated
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(os.WriteFile(dockerTmpDirName, authContentBytes, 0640)).NotTo(o.HaveOccurred()) | ||
|
|
||
| //_, outErr, err := oc.WithoutNamespace().WithoutKubeconf().Run("adm").Args("release", "info", image).Outputs() |
|
updated pr |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@test/e2e/util.go`:
- Around line 1213-1219: getClusterRegion currently indexes
getWorkersList(oc)[0] which will panic if the returned slice is empty; update
the getClusterRegion function to check the length of the slice returned by
getWorkersList(oc) before indexing, log a clear message (using e2e.Logf or
process logger) and return an empty string (or a sentinel/error value) when no
worker nodes are found to avoid panics; keep the rest of the logic (calling
oc.AsAdmin().WithoutNamespace().Run(...)) only when a worker node is available.
- Around line 959-965: The checkPodmanCred function builds the auth.json path
incorrectly by concatenating XDG_RUNTIME_DIR and "containers/auth.json" without
a path separator; update checkPodmanCred to construct the path using
filepath.Join (or explicitly insert the separator) when forming the auth.json
path, then use os.Stat on that joined path and return false only if
os.IsNotExist(err); reference function name: checkPodmanCred and the auth.json
path construction.
In `@test/testdata/oc_cli/case72217/cr-cron-72217.yaml`:
- Around line 1-6: The CR uses an unsupported field spec.test which will be
pruned because the CronTab72217 CRD (crd-crontab-72217.yaml) only defines
spec.cronSpec, spec.image, and spec.replicas; fix by either removing spec.test
from the CR (cr-cron-72217.yaml) to match the CRD or by adding spec.test to the
CRD schema in crd-crontab-72217.yaml (update the structural OpenAPI schema under
CronTab72217 to include a test property with the appropriate type and
description) so the field is accepted and not pruned.
In `@test/testdata/oc_cli/case72217/cr-custom-72217.yaml`:
- Around line 1-6: The CR sets an undefined spec field `spec.test` on
Customtask72217 (object name my-new-customtask-object) which will be pruned;
either remove the `spec.test` entry from the CR or add `test` to the
Customtask72217 CRD (crd-customtask-72217.yaml) schema under the spec properties
(including type and any validation/defaults or preservation settings) so the
field is recognized and persisted by Kubernetes.
In `@test/testdata/oc_cli/config_74099.yaml`:
- Around line 65-137: The container spec for the container named
"rails-postgresql-example" is missing container-level securityContext entries
required for restricted PSA/SCC; update the container definition inside the pod
template (the container with name rails-postgresql-example) to include a
securityContext that sets allowPrivilegeEscalation: false and runAsNonRoot: true
(and optionally ensure a non-zero runAsUser is set elsewhere if required by
policy). This change should be applied to the container-level block (not the
pod-level securityContext) so the container enforces non-root and no privilege
escalation.
In `@test/testdata/oc_cli/initContainer66989.yaml`:
- Around line 8-22: The initContainer named "wait" and the app container
"hello-pod" lack securityContext settings to enforce non-root and disable
privilege escalation; update both container specs to add a securityContext that
sets allowPrivilegeEscalation: false and runAsNonRoot: true (optionally also set
a non-root runAsUser e.g., 1000) so both initContainers[0].securityContext and
containers[0].securityContext explicitly disable privilege escalation and
require running as non-root.
♻️ Duplicate comments (9)
test/testdata/oc_cli/only-rc.yaml (1)
14-27: Add restricted PSA/SCC controls to the RC container.
WithoutallowPrivilegeEscalation: falseandrunAsNonRoot: true, this RC can be rejected on restricted clusters.✅ Proposed fix
image: "quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c" name: origin + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true ports: - containerPort: 8443 name: openshifttest/testdata/oc_cli/initContainer.yaml (1)
8-22: Harden init and app containers for restricted PSA/SCC.
Both containers should explicitly disable privilege escalation and require non-root.✅ Proposed fix
initContainers: - name: wait image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c command: ["/bin/sh", "-c", "sleep 10"] + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true containers: - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" imagePullPolicy: IfNotPresent name: hello-pod ports: - containerPort: 8080 protocol: TCP resources: {} securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true capabilities: {} privileged: falsetest/testdata/oc_cli/stable-storage.yaml (1)
35-45: Add restricted PSA/SCC controls to the StatefulSet container.
WithoutallowPrivilegeEscalation: falseandrunAsNonRoot: true, this pod can be rejected on restricted clusters.✅ Proposed fix
securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true capabilities: {} privileged: falsetest/testdata/oc_cli/dc_66672.yaml (1)
16-19: AddallowPrivilegeEscalation: falseto avoid restricted PSA/SCC failures.This fixture may be rejected on locked-down clusters without explicitly disabling privilege escalation.
🔧 Proposed fix
containers: - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 name: default-container + securityContext: + allowPrivilegeEscalation: false resources: {}test/testdata/oc_cli/rc-match-service.yaml (1)
30-31: Fix invalidconfigvolume (missing volume source).A volume must specify a source (secret/configMap/emptyDir); otherwise this RC will fail validation.
🐛 Proposed fix (example using configMap)
volumes: - name: config + configMap: + name: master-configtest/testdata/oc_cli/deploymentconfig_with_quota.yaml (1)
32-55: Disable privilege escalation on both containers.Without this, restricted PSA/SCC policies can reject the pods and break CLI tests.
🔧 Proposed fix
- image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83 imagePullPolicy: Always name: hello-openshift + securityContext: + allowPrivilegeEscalation: false resources: limits: cpu: 60m @@ - image: quay.io/openshifttest/registry@sha256:1106aedc1b2e386520bc2fb797d9a7af47d651db31d8e7ab472f2352da37d1b3 imagePullPolicy: Always name: hello-openshift2 + securityContext: + allowPrivilegeEscalation: false resources: limits: cpu: 60mtest/testdata/fixtures.go (1)
116-127: SurfaceWalkDirerrors instead of ignoring them.Silent failures can return partial fixtures and make test failures harder to diagnose.
🔧 Proposed fix
func ListFixtures() []string { var fixtures []string - fs.WalkDir(embeddedFixtures, ".", func(path string, d fs.DirEntry, err error) error { + if 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 - }) + }); err != nil { + panic(fmt.Sprintf("failed to list fixtures: %v", err)) + } return fixtures }test/testdata/oc_cli/application-template-stibuild-with-mount-secret.json (1)
201-251: Mount the secret volume in the frontend container.
mytest-volumeis defined but never mounted, so the secret is not available in the container.🔧 Proposed fix
"resources": {}, "terminationMessagePath": "/dev/termination-log", "imagePullPolicy": "IfNotPresent", + "volumeMounts": [ + { + "name": "mytest-volume", + "mountPath": "/etc/secret-volume", + "readOnly": true + } + ], "securityContext": { "capabilities": {}, "privileged": false }test/e2e/util.go (1)
1141-1158: Deployment readiness check is effectively bypassed.
AdminKubeClient()returns a dummy client, so the readiness condition succeeds immediately and doesn’t observe real deployment state. This can make tests race/flaky.🔧 One way to check real status via oc CLI
- deployment, err := oc.AdminKubeClient().AppsV1().Deployments(namespace).Get(context.Background(), name, metav1.GetOptions{}) - if err != nil { - if apierrors.IsNotFound(err) { - e2e.Logf("Waiting for availability of deployment/%s\n", name) - return false, nil - } - return false, err - } - if deployment.Status.AvailableReplicas == *deployment.Spec.Replicas && deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas { - e2e.Logf("Deployment %s available (%d/%d)\n", name, deployment.Status.AvailableReplicas, *deployment.Spec.Replicas) + output, err := oc.AsAdmin().WithoutNamespace().Run("get").Args( + "deployment", name, "-n", namespace, + "-o=jsonpath={.status.availableReplicas} {.status.updatedReplicas} {.spec.replicas}", + ).Output() + if err != nil { + e2e.Logf("Waiting for availability of deployment/%s\n", name) + return false, nil + } + parts := strings.Fields(strings.TrimSpace(output)) + if len(parts) < 3 { + return false, nil + } + available, _ := strconv.Atoi(parts[0]) + updated, _ := strconv.Atoi(parts[1]) + desired, _ := strconv.Atoi(parts[2]) + if available == desired && updated == desired { + e2e.Logf("Deployment %s available (%d/%d)\n", name, available, desired) return true, nil } - e2e.Logf("Waiting for full availability of %s deployment (%d/%d)\n", name, deployment.Status.AvailableReplicas, *deployment.Spec.Replicas) + e2e.Logf("Waiting for full availability of %s deployment (%d/%d)\n", name, available, desired) return false, nil })
| func checkPodmanCred() bool { | ||
| currentRuntime := os.Getenv("XDG_RUNTIME_DIR") | ||
| _, err := os.Stat(currentRuntime + "containers/auth.json") | ||
| if os.IsNotExist(err) { | ||
| return false | ||
| } | ||
| return true |
There was a problem hiding this comment.
Fix incorrect path when checking Podman credentials.
The missing path separator makes the auth.json lookup incorrect and can return false negatives.
🔧 Proposed fix
func checkPodmanCred() bool {
currentRuntime := os.Getenv("XDG_RUNTIME_DIR")
- _, err := os.Stat(currentRuntime + "containers/auth.json")
+ if currentRuntime == "" {
+ return false
+ }
+ _, err := os.Stat(filepath.Join(currentRuntime, "containers", "auth.json"))
if os.IsNotExist(err) {
return false
}
return true
}🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 959 - 965, The checkPodmanCred function builds
the auth.json path incorrectly by concatenating XDG_RUNTIME_DIR and
"containers/auth.json" without a path separator; update checkPodmanCred to
construct the path using filepath.Join (or explicitly insert the separator) when
forming the auth.json path, then use os.Stat on that joined path and return
false only if os.IsNotExist(err); reference function name: checkPodmanCred and
the auth.json path construction.
| func getClusterRegion(oc *CLI) string { | ||
| e2e.Logf("Getting cluster region") | ||
| node := getWorkersList(oc)[0] | ||
| region, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("node", node, "-o=jsonpath={.metadata.labels.failure-domain\\.beta\\.kubernetes\\.io\\/region}").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| e2e.Logf("Cluster region: %s", region) | ||
| return region |
There was a problem hiding this comment.
Guard against empty worker node list to avoid panics.
getWorkersList(oc)[0] will panic when no worker nodes exist (e.g., SNO or unusual labeling).
🔧 Proposed fix
func getClusterRegion(oc *CLI) string {
e2e.Logf("Getting cluster region")
- node := getWorkersList(oc)[0]
+ workers := getWorkersList(oc)
+ if len(workers) == 0 {
+ masters, err := GetMasterNodes(oc)
+ if err != nil || len(masters) == 0 {
+ e2e.Warningf("No nodes found to determine cluster region")
+ return ""
+ }
+ workers = masters
+ }
+ node := workers[0]
region, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("node", node, "-o=jsonpath={.metadata.labels.failure-domain\\.beta\\.kubernetes\\.io\\/region}").Output()
o.Expect(err).NotTo(o.HaveOccurred())
e2e.Logf("Cluster region: %s", region)
return region
}🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 1213 - 1219, getClusterRegion currently
indexes getWorkersList(oc)[0] which will panic if the returned slice is empty;
update the getClusterRegion function to check the length of the slice returned
by getWorkersList(oc) before indexing, log a clear message (using e2e.Logf or
process logger) and return an empty string (or a sentinel/error value) when no
worker nodes are found to avoid panics; keep the rest of the logic (calling
oc.AsAdmin().WithoutNamespace().Run(...)) only when a worker node is available.
| apiVersion: "stable.example.com/v1" | ||
| kind: CronTab72217 | ||
| metadata: | ||
| name: my-new-cron-object | ||
| spec: | ||
| test: test |
There was a problem hiding this comment.
CR spec doesn’t match the CronTab72217 CRD schema.
Line 6 uses spec.test, but test/testdata/oc_cli/case72217/crd-crontab-72217.yaml only defines spec.cronSpec, spec.image, and spec.replicas. With structural schemas, spec.test will be pruned, which can break any test expecting it. Please align the CR to the schema or add test to the CRD.
💡 Option: extend the CRD schema to allow spec.test
# test/testdata/oc_cli/case72217/crd-crontab-72217.yaml
properties:
cronSpec:
type: string
image:
type: string
replicas:
type: integer
+ test:
+ type: string🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/case72217/cr-cron-72217.yaml` around lines 1 - 6, The CR
uses an unsupported field spec.test which will be pruned because the
CronTab72217 CRD (crd-crontab-72217.yaml) only defines spec.cronSpec,
spec.image, and spec.replicas; fix by either removing spec.test from the CR
(cr-cron-72217.yaml) to match the CRD or by adding spec.test to the CRD schema
in crd-crontab-72217.yaml (update the structural OpenAPI schema under
CronTab72217 to include a test property with the appropriate type and
description) so the field is accepted and not pruned.
| apiVersion: "example.com/v1" | ||
| kind: Customtask72217 | ||
| metadata: | ||
| name: my-new-customtask-object | ||
| spec: | ||
| test: test |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'crd-customtask-72217.yaml' test/Repository: openshift/oc
Length of output: 112
🏁 Script executed:
cat -n test/testdata/oc_cli/case72217/crd-customtask-72217.yamlRepository: openshift/oc
Length of output: 940
Remove spec.test from the CR or add it to the CRD schema.
The Customtask72217 CRD (crd-customtask-72217.yaml) only defines three spec fields: cronSpec, image, and replicas. The spec.test field set in line 6 is not in the CRD schema and lacks explicit preservation settings, so it will likely be pruned by Kubernetes, causing any test assertions on it to fail.
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/case72217/cr-custom-72217.yaml` around lines 1 - 6, The
CR sets an undefined spec field `spec.test` on Customtask72217 (object name
my-new-customtask-object) which will be pruned; either remove the `spec.test`
entry from the CR or add `test` to the Customtask72217 CRD
(crd-customtask-72217.yaml) schema under the spec properties (including type and
any validation/defaults or preservation settings) so the field is recognized and
persisted by Kubernetes.
| containers: | ||
| - env: | ||
| - name: DATABASE_SERVICE_NAME | ||
| value: postgresql | ||
| - name: POSTGRESQL_USER | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: database-user | ||
| name: rails-postgresql-example | ||
| - name: POSTGRESQL_PASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: database-password | ||
| name: rails-postgresql-example | ||
| - name: POSTGRESQL_DATABASE | ||
| value: root | ||
| - name: SECRET_KEY_BASE | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: keybase | ||
| name: rails-postgresql-example | ||
| - name: POSTGRESQL_MAX_CONNECTIONS | ||
| value: "100" | ||
| - name: POSTGRESQL_SHARED_BUFFERS | ||
| value: 12MB | ||
| - name: APPLICATION_DOMAIN | ||
| - name: APPLICATION_USER | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: application-user | ||
| name: rails-postgresql-example | ||
| - name: APPLICATION_PASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: application-password | ||
| name: rails-postgresql-example | ||
| - name: RAILS_ENV | ||
| value: production | ||
| image: image-registry.openshift-image-registry.svc:5000/testzy/rails-postgresql-example@sha256:1a3e6d79c6a0945898a895659b385fc8f59195233d07139a4de885d456bbffae | ||
| imagePullPolicy: IfNotPresent | ||
| livenessProbe: | ||
| failureThreshold: 3 | ||
| httpGet: | ||
| path: /articles | ||
| port: 8080 | ||
| scheme: HTTP | ||
| initialDelaySeconds: 10 | ||
| periodSeconds: 10 | ||
| successThreshold: 1 | ||
| timeoutSeconds: 3 | ||
| name: rails-postgresql-example | ||
| ports: | ||
| - containerPort: 8080 | ||
| protocol: TCP | ||
| readinessProbe: | ||
| failureThreshold: 3 | ||
| httpGet: | ||
| path: /articles | ||
| port: 8080 | ||
| scheme: HTTP | ||
| initialDelaySeconds: 5 | ||
| periodSeconds: 10 | ||
| successThreshold: 1 | ||
| timeoutSeconds: 3 | ||
| resources: | ||
| limits: | ||
| memory: 512Mi | ||
| terminationMessagePath: /dev/termination-log | ||
| terminationMessagePolicy: File | ||
| dnsPolicy: ClusterFirst | ||
| restartPolicy: Always | ||
| schedulerName: default-scheduler | ||
| securityContext: {} |
There was a problem hiding this comment.
Add container securityContext for restricted PSA/SCC.
This DeploymentConfig container lacks allowPrivilegeEscalation: false and runAsNonRoot: true.
✅ Proposed fix
name: rails-postgresql-example
+ securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
ports:
- containerPort: 8080
protocol: TCP📝 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.
| containers: | |
| - env: | |
| - name: DATABASE_SERVICE_NAME | |
| value: postgresql | |
| - name: POSTGRESQL_USER | |
| valueFrom: | |
| secretKeyRef: | |
| key: database-user | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_PASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| key: database-password | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_DATABASE | |
| value: root | |
| - name: SECRET_KEY_BASE | |
| valueFrom: | |
| secretKeyRef: | |
| key: keybase | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_MAX_CONNECTIONS | |
| value: "100" | |
| - name: POSTGRESQL_SHARED_BUFFERS | |
| value: 12MB | |
| - name: APPLICATION_DOMAIN | |
| - name: APPLICATION_USER | |
| valueFrom: | |
| secretKeyRef: | |
| key: application-user | |
| name: rails-postgresql-example | |
| - name: APPLICATION_PASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| key: application-password | |
| name: rails-postgresql-example | |
| - name: RAILS_ENV | |
| value: production | |
| image: image-registry.openshift-image-registry.svc:5000/testzy/rails-postgresql-example@sha256:1a3e6d79c6a0945898a895659b385fc8f59195233d07139a4de885d456bbffae | |
| imagePullPolicy: IfNotPresent | |
| livenessProbe: | |
| failureThreshold: 3 | |
| httpGet: | |
| path: /articles | |
| port: 8080 | |
| scheme: HTTP | |
| initialDelaySeconds: 10 | |
| periodSeconds: 10 | |
| successThreshold: 1 | |
| timeoutSeconds: 3 | |
| name: rails-postgresql-example | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| readinessProbe: | |
| failureThreshold: 3 | |
| httpGet: | |
| path: /articles | |
| port: 8080 | |
| scheme: HTTP | |
| initialDelaySeconds: 5 | |
| periodSeconds: 10 | |
| successThreshold: 1 | |
| timeoutSeconds: 3 | |
| resources: | |
| limits: | |
| memory: 512Mi | |
| terminationMessagePath: /dev/termination-log | |
| terminationMessagePolicy: File | |
| dnsPolicy: ClusterFirst | |
| restartPolicy: Always | |
| schedulerName: default-scheduler | |
| securityContext: {} | |
| containers: | |
| - env: | |
| - name: DATABASE_SERVICE_NAME | |
| value: postgresql | |
| - name: POSTGRESQL_USER | |
| valueFrom: | |
| secretKeyRef: | |
| key: database-user | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_PASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| key: database-password | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_DATABASE | |
| value: root | |
| - name: SECRET_KEY_BASE | |
| valueFrom: | |
| secretKeyRef: | |
| key: keybase | |
| name: rails-postgresql-example | |
| - name: POSTGRESQL_MAX_CONNECTIONS | |
| value: "100" | |
| - name: POSTGRESQL_SHARED_BUFFERS | |
| value: 12MB | |
| - name: APPLICATION_DOMAIN | |
| - name: APPLICATION_USER | |
| valueFrom: | |
| secretKeyRef: | |
| key: application-user | |
| name: rails-postgresql-example | |
| - name: APPLICATION_PASSWORD | |
| valueFrom: | |
| secretKeyRef: | |
| key: application-password | |
| name: rails-postgresql-example | |
| - name: RAILS_ENV | |
| value: production | |
| image: image-registry.openshift-image-registry.svc:5000/testzy/rails-postgresql-example@sha256:1a3e6d79c6a0945898a895659b385fc8f59195233d07139a4de885d456bbffae | |
| imagePullPolicy: IfNotPresent | |
| livenessProbe: | |
| failureThreshold: 3 | |
| httpGet: | |
| path: /articles | |
| port: 8080 | |
| scheme: HTTP | |
| initialDelaySeconds: 10 | |
| periodSeconds: 10 | |
| successThreshold: 1 | |
| timeoutSeconds: 3 | |
| name: rails-postgresql-example | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| readinessProbe: | |
| failureThreshold: 3 | |
| httpGet: | |
| path: /articles | |
| port: 8080 | |
| scheme: HTTP | |
| initialDelaySeconds: 5 | |
| periodSeconds: 10 | |
| successThreshold: 1 | |
| timeoutSeconds: 3 | |
| resources: | |
| limits: | |
| memory: 512Mi | |
| terminationMessagePath: /dev/termination-log | |
| terminationMessagePolicy: File | |
| dnsPolicy: ClusterFirst | |
| restartPolicy: Always | |
| schedulerName: default-scheduler | |
| securityContext: {} |
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/config_74099.yaml` around lines 65 - 137, The container
spec for the container named "rails-postgresql-example" is missing
container-level securityContext entries required for restricted PSA/SCC; update
the container definition inside the pod template (the container with name
rails-postgresql-example) to include a securityContext that sets
allowPrivilegeEscalation: false and runAsNonRoot: true (and optionally ensure a
non-zero runAsUser is set elsewhere if required by policy). This change should
be applied to the container-level block (not the pod-level securityContext) so
the container enforces non-root and no privilege escalation.
| initContainers: | ||
| - name: wait | ||
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | ||
| command: ["/bin/sh", "-c", "sleep 30"] | ||
| containers: | ||
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | ||
| imagePullPolicy: IfNotPresent | ||
| name: hello-pod | ||
| ports: | ||
| - containerPort: 8080 | ||
| protocol: TCP | ||
| resources: {} | ||
| securityContext: | ||
| capabilities: {} | ||
| privileged: false |
There was a problem hiding this comment.
Harden init and app containers for restricted PSA/SCC.
Both containers should explicitly disable privilege escalation and require non-root.
✅ Proposed fix
initContainers:
- name: wait
image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c
command: ["/bin/sh", "-c", "sleep 30"]
+ securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
containers:
- image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83"
imagePullPolicy: IfNotPresent
name: hello-pod
ports:
- containerPort: 8080
protocol: TCP
resources: {}
securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
capabilities: {}
privileged: false📝 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.
| initContainers: | |
| - name: wait | |
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | |
| command: ["/bin/sh", "-c", "sleep 30"] | |
| containers: | |
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | |
| imagePullPolicy: IfNotPresent | |
| name: hello-pod | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| resources: {} | |
| securityContext: | |
| capabilities: {} | |
| privileged: false | |
| initContainers: | |
| - name: wait | |
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | |
| command: ["/bin/sh", "-c", "sleep 30"] | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| containers: | |
| - image: "quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83" | |
| imagePullPolicy: IfNotPresent | |
| name: hello-pod | |
| ports: | |
| - containerPort: 8080 | |
| protocol: TCP | |
| resources: {} | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| capabilities: {} | |
| privileged: false |
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/initContainer66989.yaml` around lines 8 - 22, The
initContainer named "wait" and the app container "hello-pod" lack
securityContext settings to enforce non-root and disable privilege escalation;
update both container specs to add a securityContext that sets
allowPrivilegeEscalation: false and runAsNonRoot: true (optionally also set a
non-root runAsUser e.g., 1000) so both initContainers[0].securityContext and
containers[0].securityContext explicitly disable privilege escalation and
require running as non-root.
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
I've passed through some tests.
| }) | ||
|
|
||
| // author: knarra@redhat.com | ||
| g.It("MicroShiftBoth-ROSA-OSD_CCS-ARO-Author:knarra-High-63855-Medium-64944-Verify oc image extract and oc adm release info throws error when both --icsp-file and -idms-file flag is used", func() { |
There was a problem hiding this comment.
We can drop this, this is verified by unit tests.
| } | ||
| }) | ||
|
|
||
| g.It("Author:yinzhou-ROSA-OSD_CCS-ARO-High-76116-Make sure oc could run on rhel with fips on", func() { |
There was a problem hiding this comment.
This is an important one. Did you check that this is skipped or executed.
| }) | ||
| AssertWaitPollNoErr(err, "oc image mirror fails even after waiting for about 120 seconds") | ||
| }) | ||
| g.It("Author:yinzhou-ROSA-OSD_CCS-ARO-Medium-76150-Make sure oc debug node has set HOST env var", func() { |
| o.Expect(removeErr).NotTo(o.HaveOccurred()) | ||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-Author:yinzhou-Medium-72217-Should get warning when there is an identical short name for two or more resources", func() { |
| AssertWaitPollNoErr(err, "Failed to get assert the detailed document resource url") | ||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Low-54411-Low-21060-kubectl exec should work behind authenticated proxy", func() { |
| AssertWaitPollNoErr(err, "Failed to get assert the detailed document resource url") | ||
| }) | ||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Low-21115-Use kubelet explain to see detailed documentation of resources", func() { |
There was a problem hiding this comment.
In our CI, there is similar test already https://github.com/openshift/origin/blob/main/test/extended/cli/explain.go, we can remove this.
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Low-11202-Use oc explain to see detailed documentation of resources", func() { |
There was a problem hiding this comment.
| }) | ||
|
|
||
| // author: yinzhou@redhat.com | ||
| g.It("Author:yinzhou-NonHyperShiftHOST-ROSA-OSD_CCS-ARO-High-68405-oc process works well for cross-namespace template", func() { |
There was a problem hiding this comment.
Important one, we should keep it.
|
@gangwgr: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
This PR adds comprehensive CLI test infrastructure for the oc command, implementing a self-contained testing framework that avoids external test dependencies
New dependencies added:
Vendor updates: