Add namespace-admin support in E2E test framework and automate MTC-290 initContainer migration test#222
Merged
msajidmansoori12 merged 9 commits intomigtools:mainfrom Apr 7, 2026
Conversation
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds tooling and CI to create Minikube client-cert users/contexts, wire new inputs through the composite action and setup script, and introduce RBAC e2e tests and a workflow that verifies authorization before and after applying namespace RoleBindings. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions"
participant Action as "setup-minikube-clusters\n(composite action)"
participant Setup as "setup-minikube-same-network.sh"
participant Minikube as "Minikube profiles"
participant Script as "minikube-create-user.sh"
participant OpenSSL as "openssl"
participant Kubectl as "kubectl / kubeconfig"
GH->>Action: workflow triggers (create_users=true, src/tgt user)
Action->>Setup: invoke setup script with env vars
Setup->>Minikube: provision src & tgt clusters/profiles
Setup->>Script: create_user_context(profile,user,context)
Script->>Minikube: read CA cert/key for profile
Script->>OpenSSL: generate key/CSR and sign client cert
OpenSSL-->>Script: return key/cert
Script->>Kubectl: set-credentials (embed certs)
Script->>Kubectl: set-context (new user context)
GH->>Kubectl: run auth can-i (pre-binding)
GH->>Kubectl: apply RoleBinding
GH->>Kubectl: run auth can-i (post-binding)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (3)
hack/minikube-create-user.sh (3)
138-146: Suppressing stderr hides useful diagnostic information on failure.If
opensslcommands fail,set -ewill exit the script but with no indication of what went wrong. Consider showing errors or adding explicit error handling.🔧 Proposed improvement
-openssl genrsa -out "$USER_KEY" 2048 >/dev/null 2>&1 +if ! openssl genrsa -out "$USER_KEY" 2048 2>&1; then + printf 'Error: failed to generate private key\n' >&2 + exit 1 +fiApply similar pattern to other openssl commands, or at minimum redirect only stdout while preserving stderr:
-openssl genrsa -out "$USER_KEY" 2048 >/dev/null 2>&1 +openssl genrsa -out "$USER_KEY" 2048 >/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/minikube-create-user.sh` around lines 138 - 146, The openssl commands (openssl genrsa, openssl req, openssl x509) are redirecting both stdout and stderr to /dev/null which hides useful diagnostics; change them to only silence stdout (redirect only > /dev/null) or add explicit error handling: run each command for USER_KEY, USER_CSR and USER_CRT, check its exit status ($?), and on failure write a clear error message including the failing command and relevant variables (USER_NAME, ORG_NAME, CA_CERT, CA_KEY, DAYS) to stderr and exit non‑zero so failures are visible instead of silently swallowed.
154-161: Consider warning if user or context already exists in kubeconfig.If credentials for
$USER_NAMEor context$CONTEXT_NAMEalready exist, they will be silently overwritten. This may be intentional for idempotency, but a warning could prevent accidental data loss.💡 Optional: Add overwrite warning
+if kubectl config get-contexts "$CONTEXT_NAME" >/dev/null 2>&1; then + printf 'Warning: context "%s" already exists and will be overwritten\n' "$CONTEXT_NAME" >&2 +fi + kubectl config set-credentials "$USER_NAME" \ --client-certificate="$USER_CRT" \ --client-key="$USER_KEY" \ --embed-certs=true >/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/minikube-create-user.sh` around lines 154 - 161, The script currently unconditionally calls "kubectl config set-credentials" for "$USER_NAME" and "kubectl config set-context" for "$CONTEXT_NAME", which will silently overwrite existing entries; add a pre-check that queries the kubeconfig for existing credentials and contexts (e.g., via "kubectl config get-users" and "kubectl config get-contexts" or suitable kubectl config view jsonpath), and if "$USER_NAME" or "$CONTEXT_NAME" is found emit a clear warning to stdout/stderr before proceeding (optionally prompt or require a FORCE/OVERWRITE env var to continue); update the block around the kubectl calls so it warns when an existing user/context is detected and only proceeds to run "kubectl config set-credentials" / "kubectl config set-context" after the warning/confirmation.
56-59: Missing argument value causes silent misbehavior or cryptic error.If a flag like
--profileis passed without a value (e.g.,--profile --user dev),PROFILEsilently gets set to--user. If it's the last argument,shift 2fails with an unhelpful error.🛡️ Proposed fix to validate argument values
--profile) + if [[ -z "${2:-}" || "${2:-}" == --* ]]; then + printf 'Error: --profile requires a value\n' >&2 + exit 1 + fi PROFILE="${2:-}" shift 2 ;;Apply the same pattern to
--user,--context,--org,--days, and--output-dir.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/minikube-create-user.sh` around lines 56 - 59, The flag parsing for --profile currently assigns the next positional argument to PROFILE without validation, causing silent misassignment or shift errors; update the parsing logic in the argument-handling case blocks (the --profile branch and similarly for --user, --context, --org, --days, --output-dir) to first check that the next argument exists and does not begin with "-" (i.e., is not another flag), and if it is missing or is another flag, emit a clear error and exit; otherwise assign the value to the respective variable (e.g., PROFILE, USER, CONTEXT, ORG, DAYS, OUTPUT_DIR) and then shift 2 as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/minikube-create-user.sh`:
- Around line 102-105: The validation for DAYS currently accepts "0" (using
regex ^[0-9]+$) which would produce an immediately expired cert; update the
check in the DAYS validation block to require a positive integer > 0 (e.g.,
change the regex to ^[1-9][0-9]*$ or test numerically with an integer parse and
a DAYS -gt 0 check) and ensure the error message still reads something like
"--days must be a positive integer" when the validation fails; locate the check
using the DAYS variable and the if ! [[ "$DAYS" =~ ... ]] conditional to make
this change.
- Line 138: The private key created by the openssl genrsa command (openssl
genrsa -out "$USER_KEY" 2048) may inherit a permissive umask; restrict file
access by either setting umask 077 immediately before the key generation or by
applying chmod 600 "$USER_KEY" right after the openssl command and checking for
errors; ensure you reference the USER_KEY variable and preserve any existing
error handling around openssl.
---
Nitpick comments:
In `@hack/minikube-create-user.sh`:
- Around line 138-146: The openssl commands (openssl genrsa, openssl req,
openssl x509) are redirecting both stdout and stderr to /dev/null which hides
useful diagnostics; change them to only silence stdout (redirect only >
/dev/null) or add explicit error handling: run each command for USER_KEY,
USER_CSR and USER_CRT, check its exit status ($?), and on failure write a clear
error message including the failing command and relevant variables (USER_NAME,
ORG_NAME, CA_CERT, CA_KEY, DAYS) to stderr and exit non‑zero so failures are
visible instead of silently swallowed.
- Around line 154-161: The script currently unconditionally calls "kubectl
config set-credentials" for "$USER_NAME" and "kubectl config set-context" for
"$CONTEXT_NAME", which will silently overwrite existing entries; add a pre-check
that queries the kubeconfig for existing credentials and contexts (e.g., via
"kubectl config get-users" and "kubectl config get-contexts" or suitable kubectl
config view jsonpath), and if "$USER_NAME" or "$CONTEXT_NAME" is found emit a
clear warning to stdout/stderr before proceeding (optionally prompt or require a
FORCE/OVERWRITE env var to continue); update the block around the kubectl calls
so it warns when an existing user/context is detected and only proceeds to run
"kubectl config set-credentials" / "kubectl config set-context" after the
warning/confirmation.
- Around line 56-59: The flag parsing for --profile currently assigns the next
positional argument to PROFILE without validation, causing silent misassignment
or shift errors; update the parsing logic in the argument-handling case blocks
(the --profile branch and similarly for --user, --context, --org, --days,
--output-dir) to first check that the next argument exists and does not begin
with "-" (i.e., is not another flag), and if it is missing or is another flag,
emit a clear error and exit; otherwise assign the value to the respective
variable (e.g., PROFILE, USER, CONTEXT, ORG, DAYS, OUTPUT_DIR) and then shift 2
as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d79ea81-86b7-4c8a-bd2c-3522490ef78e
📒 Files selected for processing (1)
hack/minikube-create-user.sh
msajidmansoori12
force-pushed
the
add-ns-user
branch
from
5402ada to
875b8d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test-minikube-user-rbac.yml (1)
45-55: Don't blanket-ignore RoleBinding creation errors.These clusters are recreated for each run, so
AlreadyExistsshould not be the normal path here.|| truewill also swallow real API/auth failures and make the post-check harder to diagnose. Prefer an idempotentapplyflow, or only ignore the specificAlreadyExistscase.🔧 Safer idempotent pattern
- kubectl --context src create rolebinding sajid-admin \ - --clusterrole=admin \ - --user=sajid \ - --namespace=default || true + kubectl --context src create rolebinding sajid-admin \ + --clusterrole=admin \ + --user=sajid \ + --namespace=default \ + --dry-run=client -o yaml | kubectl --context src apply -f - - kubectl --context tgt create rolebinding sajid-admin \ - --clusterrole=admin \ - --user=sajid \ - --namespace=default || true + kubectl --context tgt create rolebinding sajid-admin \ + --clusterrole=admin \ + --user=sajid \ + --namespace=default \ + --dry-run=client -o yaml | kubectl --context tgt apply -f -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-minikube-user-rbac.yml around lines 45 - 55, The current kubectl create rolebinding commands for sajid-admin (contexts src and tgt, namespace default) use "|| true" which silences all failures; change to an idempotent approach: either switch to kubectl apply with a RoleBinding manifest or perform a create and explicitly handle the AlreadyExists case by checking for that specific error (or testing for existence with kubectl get rolebinding sajid-admin --namespace=default) and only ignore that condition while letting other API/auth errors surface; remove the blanket "|| true" so real failures are not swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/setup-minikube-same-network.sh`:
- Around line 46-52: The kubeconfig credentials are being keyed by raw username
(USER_NAME) so calling create_user_context twice with the same SRC_USER/TGT_USER
("dev") causes the second set-credentials call to overwrite the first; change
the kubectl config set-credentials invocation in create_user_context to use the
context identifier (CONTEXT_NAME) instead of USER_NAME so credentials are stored
under distinct keys (e.g., src-dev and tgt-dev) and no longer collide when
SRC_USER and TGT_USER default to the same value.
---
Nitpick comments:
In @.github/workflows/test-minikube-user-rbac.yml:
- Around line 45-55: The current kubectl create rolebinding commands for
sajid-admin (contexts src and tgt, namespace default) use "|| true" which
silences all failures; change to an idempotent approach: either switch to
kubectl apply with a RoleBinding manifest or perform a create and explicitly
handle the AlreadyExists case by checking for that specific error (or testing
for existence with kubectl get rolebinding sajid-admin --namespace=default) and
only ignore that condition while letting other API/auth errors surface; remove
the blanket "|| true" so real failures are not swallowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 874b831a-e8ff-4a60-ad70-8286b5dfbbf4
📒 Files selected for processing (3)
.github/actions/setup-minikube-clusters/action.yml.github/workflows/test-minikube-user-rbac.ymlhack/setup-minikube-same-network.sh
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
msajidmansoori12
force-pushed
the
add-ns-user
branch
from
875b8d6 to
8aa59c6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/setup-minikube-same-network.sh (1)
55-55: Redundantset -xstatement.This duplicates line 3. Consider removing it for cleaner code.
♻️ Proposed fix
USER_CERT_DAYS="${USER_CERT_DAYS:-365}" RESET_PROFILES="${RESET_PROFILES:-true}" RECREATE_NETWORK="${RECREATE_NETWORK:-true}" -set -x🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/setup-minikube-same-network.sh` at line 55, Remove the redundant shell debug flag: delete the duplicate "set -x" statement (the one repeated later in the script) so only the initial "set -x" remains; locate the second occurrence of "set -x" in the hack/setup-minikube-same-network.sh script and remove that line to avoid duplicate debugging toggles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/setup-minikube-same-network.sh`:
- Line 55: Remove the redundant shell debug flag: delete the duplicate "set -x"
statement (the one repeated later in the script) so only the initial "set -x"
remains; locate the second occurrence of "set -x" in the
hack/setup-minikube-same-network.sh script and remove that line to avoid
duplicate debugging toggles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8a9408e-5832-434b-a8fc-f69831ac440a
📒 Files selected for processing (3)
.github/actions/setup-minikube-clusters/action.yml.github/workflows/test-minikube-user-rbac.ymlhack/setup-minikube-same-network.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test-minikube-user-rbac.yml
…t framework Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
e2e-tests/framework/client.go (1)
90-117: Consider deduplicating kubeconfig loading logic.Line 90–117 repeats the loading/env handling pattern already present in
NewClientSetForContext(Line 38–46). A small shared helper would reduce drift risk and keep error handling consistent across framework clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/framework/client.go` around lines 90 - 117, The kubeconfig loading and env handling in ResolveUsernameForContext is duplicated from NewClientSetForContext; extract that logic into a shared helper (e.g., LoadRawKubeConfig or ResolveRawKubeConfig) and have both ResolveUsernameForContext and NewClientSetForContext call it. The helper should read KUBECONFIG env, set loadingRules.ExplicitPath when present, call loadingRules.Load(), and return the rawConfig or wrapped error so both functions reuse identical error handling and avoid drift..github/workflows/test-minikube-user-rbac.yml (1)
53-61: Avoid suppressing all rolebinding creation errors with|| true.At Line 53–61, blanket suppression hides real failures (RBAC API errors, context issues) and makes root-cause diagnosis harder. Prefer idempotent apply behavior instead.
Suggested change
- kubectl --context src create rolebinding dev-admin \ - --clusterrole=admin \ - --user=dev \ - --namespace=default || true + kubectl --context src create rolebinding dev-admin \ + --clusterrole=admin \ + --user=dev \ + --namespace=default \ + --dry-run=client -o yaml | kubectl --context src apply -f - - kubectl --context tgt create rolebinding dev-admin \ - --clusterrole=admin \ - --user=dev \ - --namespace=default || true + kubectl --context tgt create rolebinding dev-admin \ + --clusterrole=admin \ + --user=dev \ + --namespace=default \ + --dry-run=client -o yaml | kubectl --context tgt apply -f -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-minikube-user-rbac.yml around lines 53 - 61, Remove the blanket "|| true" suppression and make the rolebinding creation idempotent: instead of the two lines that run "kubectl --context <ctx> create rolebinding dev-admin --clusterrole=admin --user=dev --namespace=default || true" (for contexts "src" and "tgt"), generate the RoleBinding YAML via "kubectl create rolebinding ... --dry-run=client -o yaml" and pipe it to "kubectl apply -f -" (or apply a static manifest), and remove the "|| true" so failures surface; update both the "kubectl --context src ..." and "kubectl --context tgt ..." invocations accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-minikube-user-rbac.yml:
- Around line 45-46: Update the two echo log lines that currently print
"src-sajid" and "tgt-sajid" to reflect the actual user names used by the
workflow (src-dev and tgt-dev); locate the echo statements (the lines that
output "${SRC_CAN_I} (rc=${SRC_ALLOWED_RC})" and "${TGT_CAN_I}
(rc=${TGT_ALLOWED_RC})") and change their static labels from "src-sajid" →
"src-dev" and "tgt-sajid" → "tgt-dev" so CI logs accurately represent the
provisioned users.
In `@e2e-tests/framework/rbac.go`:
- Around line 77-80: In SetupNamespaceAdminUser, the returned KubectlRunner is
hardcoded to Bin: "kubectl" which ignores the validated adminKubectl.Bin and can
break subsequent CanI calls; update the construction of userKubectl to use
adminKubectl.Bin (i.e., set Bin to adminKubectl.Bin) so the runner uses the same
kubectl binary/path validated earlier (ensure you update the KubectlRunner
instance referenced by userKubectl in that function).
In `@e2e-tests/tests/mtc_rbac_ns_admin_test.go`:
- Around line 14-17: The test sets adminContext := config.SourceContext but does
not validate it, which can cause commands to run in an unintended kube context;
add an explicit guard similar to the existing nonadminContext check by asserting
adminContext (or config.SourceContext) is not empty before using it to build the
admin runner (the symbol adminContext and config.SourceContext should be
validated), and update any code that constructs the admin runner to rely on the
validated adminContext.
---
Nitpick comments:
In @.github/workflows/test-minikube-user-rbac.yml:
- Around line 53-61: Remove the blanket "|| true" suppression and make the
rolebinding creation idempotent: instead of the two lines that run "kubectl
--context <ctx> create rolebinding dev-admin --clusterrole=admin --user=dev
--namespace=default || true" (for contexts "src" and "tgt"), generate the
RoleBinding YAML via "kubectl create rolebinding ... --dry-run=client -o yaml"
and pipe it to "kubectl apply -f -" (or apply a static manifest), and remove the
"|| true" so failures surface; update both the "kubectl --context src ..." and
"kubectl --context tgt ..." invocations accordingly.
In `@e2e-tests/framework/client.go`:
- Around line 90-117: The kubeconfig loading and env handling in
ResolveUsernameForContext is duplicated from NewClientSetForContext; extract
that logic into a shared helper (e.g., LoadRawKubeConfig or
ResolveRawKubeConfig) and have both ResolveUsernameForContext and
NewClientSetForContext call it. The helper should read KUBECONFIG env, set
loadingRules.ExplicitPath when present, call loadingRules.Load(), and return the
rawConfig or wrapped error so both functions reuse identical error handling and
avoid drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c9174b8-30ce-4dba-b10d-149614c8aa3c
📒 Files selected for processing (10)
.github/workflows/e2e-pr-tester.yaml.github/workflows/test-minikube-user-rbac.ymle2e-tests/config/config.goe2e-tests/framework/client.goe2e-tests/framework/kubectl.goe2e-tests/framework/rbac.goe2e-tests/testdata/rolebinding_namespace_admin.yamle2e-tests/tests/e2e_suite_test.goe2e-tests/tests/mtc_rbac_ns_admin_test.gohack/minikube-create-user.sh
✅ Files skipped from review due to trivial changes (1)
- e2e-tests/testdata/rolebinding_namespace_admin.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/minikube-create-user.sh
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Test Coverage ReportTotal: 8.0% Per-package coverage
Full function-level detailsPosted by CI |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
e2e-tests/framework/rbac.go (1)
77-80:⚠️ Potential issue | 🟠 MajorUse the validated admin kubectl binary for the returned non-admin runner.
At Line 77–80,
userKubectl.Binis hardcoded to"kubectl"even thoughadminKubectl.Binwas already validated. This can fail when tests use a custom kubectl path.Suggested fix
userKubectl := KubectlRunner{ - Bin: "kubectl", + Bin: adminKubectl.Bin, Context: nonAdminContext, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/framework/rbac.go` around lines 77 - 80, The non-admin KubectlRunner is hardcoded to "kubectl" which ignores the validated admin binary; change the user runner construction so userKubectl.Bin uses the already-validated adminKubectl.Bin (i.e., set Bin to adminKubectl.Bin) while keeping Context set to nonAdminContext so the returned KubectlRunner uses the same validated kubectl binary as adminKubectl.
🧹 Nitpick comments (1)
e2e-tests/framework/scenario.go (1)
45-60: Prefer explicit non-admin context inputs over package-global reads.At Line 45–60,
NewMigrationScenariopullsconfig.SourceNonAdminContext/config.TargetNonAdminContextdirectly, which makes scenario construction depend on global state and can defer empty-context failures to later RBAC setup. Passing non-admin contexts as constructor args would make setup deterministic.Refactor sketch
-func NewMigrationScenario(appName, namespace, k8sDeployBin, craneBin, srcCtx, tgtCtx string) MigrationScenario { +func NewMigrationScenario(appName, namespace, k8sDeployBin, craneBin, srcCtx, tgtCtx, srcNonAdminCtx, tgtNonAdminCtx string) MigrationScenario { return MigrationScenario{ ... SrcAppNonAdmin: K8sDeployApp{ ... - Context: config.SourceNonAdminContext, + Context: srcNonAdminCtx, }, TgtAppNonAdmin: K8sDeployApp{ ... - Context: config.TargetNonAdminContext, + Context: tgtNonAdminCtx, }, ... - KubectlSrcNonAdmin: KubectlRunner{Bin: "kubectl", Context: config.SourceNonAdminContext}, - KubectlTgtNonAdmin: KubectlRunner{Bin: "kubectl", Context: config.TargetNonAdminContext}, + KubectlSrcNonAdmin: KubectlRunner{Bin: "kubectl", Context: srcNonAdminCtx}, + KubectlTgtNonAdmin: KubectlRunner{Bin: "kubectl", Context: tgtNonAdminCtx}, ... CraneNonAdmin: CraneRunner{ Bin: craneBin, - SourceContext: config.SourceNonAdminContext, + SourceContext: srcNonAdminCtx, }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/framework/scenario.go` around lines 45 - 60, NewMigrationScenario currently reads package-global config.SourceNonAdminContext and config.TargetNonAdminContext when initializing SrcAppNonAdmin, TgtAppNonAdmin, KubectlSrcNonAdmin and KubectlTgtNonAdmin, causing hidden/global-state failures; change NewMigrationScenario signature to accept explicit non-admin context parameters (e.g., srcNonAdminCtx, tgtNonAdminCtx) and use those values to populate the Context fields for K8sDeployApp (SrcAppNonAdmin, TgtAppNonAdmin) and KubectlRunner (KubectlSrcNonAdmin, KubectlTgtNonAdmin) instead of referencing config.SourceNonAdminContext/config.TargetNonAdminContext so scenario construction is deterministic and test inputs are explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e-tests/framework/client.go`:
- Around line 14-21: The function newKubeConfigLoadingRules currently reads
KUBECONFIG and assigns it to loadingRules.ExplicitPath, which disables the
default multi-file merge/precedence behavior; remove the manual override so the
loading rules produced by clientcmd.NewDefaultClientConfigLoadingRules() are
returned unmodified. In practice, delete the KUBECONFIG lookup and any
assignment to loadingRules.ExplicitPath in newKubeConfigLoadingRules so it
simply returns the default loadingRules.
In `@e2e-tests/framework/rbac.go`:
- Around line 82-88: The RBAC preflight error path after calling
userKubectl.CanI leaves the previously created RoleBinding/grant in place;
update the error branch so that before returning you revoke/delete the
RoleBinding that was created earlier (the grant made at Line ~73) using the same
kubectl runner or cleanup helper, and if the cleanup fails include that cleanup
error (or log it) while still returning the original CanI error; reference the
CanI call and the earlier RoleBinding/grant creation and ensure the cleanup
invocation runs in the error path so no permission is left behind.
---
Duplicate comments:
In `@e2e-tests/framework/rbac.go`:
- Around line 77-80: The non-admin KubectlRunner is hardcoded to "kubectl" which
ignores the validated admin binary; change the user runner construction so
userKubectl.Bin uses the already-validated adminKubectl.Bin (i.e., set Bin to
adminKubectl.Bin) while keeping Context set to nonAdminContext so the returned
KubectlRunner uses the same validated kubectl binary as adminKubectl.
---
Nitpick comments:
In `@e2e-tests/framework/scenario.go`:
- Around line 45-60: NewMigrationScenario currently reads package-global
config.SourceNonAdminContext and config.TargetNonAdminContext when initializing
SrcAppNonAdmin, TgtAppNonAdmin, KubectlSrcNonAdmin and KubectlTgtNonAdmin,
causing hidden/global-state failures; change NewMigrationScenario signature to
accept explicit non-admin context parameters (e.g., srcNonAdminCtx,
tgtNonAdminCtx) and use those values to populate the Context fields for
K8sDeployApp (SrcAppNonAdmin, TgtAppNonAdmin) and KubectlRunner
(KubectlSrcNonAdmin, KubectlTgtNonAdmin) instead of referencing
config.SourceNonAdminContext/config.TargetNonAdminContext so scenario
construction is deterministic and test inputs are explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e61edd5-4b6f-417e-935c-d0ac08eefef4
📒 Files selected for processing (5)
e2e-tests/framework/client.goe2e-tests/framework/crane.goe2e-tests/framework/pipeline.goe2e-tests/framework/rbac.goe2e-tests/framework/scenario.go
💤 Files with no reviewable changes (1)
- e2e-tests/framework/crane.go
msajidmansoori12
force-pushed
the
add-ns-user
branch
4 times, most recently
from
93cef81 to
850af96
Compare
…iner app Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
msajidmansoori12
force-pushed
the
add-ns-user
branch
from
850af96 to
afdd9d0
Compare
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
msajidmansoori12
changed the title
Contributor
stillalearner
left a comment
stillalearner
left a comment
There was a problem hiding this comment.
Please add appropriate title,summary
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
nachandr
approved these changes
Apr 6, 2026
This was referenced Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds support for running Crane E2E scenarios with non-admin (namespace-admin) users and extends CI/workflows to pass non-admin contexts.
It also adds automated coverage for MTC-290 to validate migration of an app with an init container using namespace-admin permissions.
Changes Included
--source-nonadmin-context--target-nonadmin-contextkubectlk8sdeployapp handlescranerunnere2e-tests/testdata/rolebinding_namespace_admin.yamlApplyOutputToTargetNonAdmin(...)(apply without namespace creation)e2e-tests/tests/mtc_290_initcontainer_test.go.github/workflows/e2e-pr-tester.yaml.github/workflows/run-e2e-tests.ymle2e-tests/README.mdnow reflects namespace-admin/non-admin supportWhy
This improves E2E coverage to better match restricted real-world permission models, while still validating migration behavior end-to-end.
Follow-up (Optional)
In a follow-up PR, we can incrementally update more already-automated scenarios to also run under namespace-admin user contexts.