Add non-admin ConfigMap migration test [MTC-291] and harden RBAC identity resolution#234
Merged
midays merged 9 commits intomigtools:mainfrom Apr 12, 2026
Merged
Conversation
…nch page as succeeded check Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
… name same as context name 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:
📝 WalkthroughWalkthroughThis PR adds a new end-to-end test for ConfigMap migration by non-admin users (MTC-291), updates the minikube user creation script to derive Kubernetes context credentials from context names instead of user names, and enhances the client framework to extract RBAC usernames from x509 client certificates. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Runner
participant SourceCluster as Source Cluster
participant Crane as Crane Pipeline
participant TargetCluster as Target Cluster
Test->>SourceCluster: Setup namespace-admin for non-admin user
Test->>SourceCluster: Prepare source app
Test->>Crane: Run export (source non-admin)
Crane->>SourceCluster: Export ConfigMap as non-admin
Test->>Crane: Run transform
Crane->>Crane: Render manifests to temp directory
Test->>Test: Validate ConfigMap YAML exists
Test->>TargetCluster: Apply rendered manifests (non-admin)
Test->>TargetCluster: Wait for target app validation
Test->>TargetCluster: Verify ConfigMap exists (non-admin kubectl)
TargetCluster-->>Test: ConfigMap data confirmed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Test Coverage ReportTotal: 20.6% Per-package coverage
Full function-level detailsPosted by CI |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/minikube-create-user.sh (1)
155-162:⚠️ Potential issue | 🔴 CriticalContext points to a non-existent kubeconfig user key.
set-context --user="$CONTEXT_NAME"references a different credentials entry than the one created byset-credentials "$USER_NAME". Since$USER_NAMEand$CONTEXT_NAMEare intentionally different (e.g.,"alice"vs"minikube-alice"), the generated context will be broken—kubectl will fail to find the credentials it needs.Fix
Change line 155 to use the same key that set-context references:
-kubectl config set-credentials "$USER_NAME" \ +kubectl config set-credentials "$CONTEXT_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 155 - 162, The context is being created with a user name that doesn't match the credentials entry: set-credentials creates credentials under "$USER_NAME" but set-context uses "$CONTEXT_NAME"; update the set-context call to use the same user key created by set-credentials (i.e., change the --user value to "$USER_NAME") so the context's user matches the credentials created by set-credentials (symbols: set-credentials, set-context, $USER_NAME, $CONTEXT_NAME).
🤖 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/tests/mtc_291_configmap_test.go`:
- Around line 76-83: The test currently reads only matches[0]; instead iterate
over all globbed matches returned by filepath.Glob and for each file path call
os.ReadFile and run the Expect assertions (Expect(err).NotTo(HaveOccurred()) and
Expect(string(content)).To(ContainSubstring(...))) so every matched ConfigMap
manifest is validated; update references to pattern, matches, configMap (or
content) and keep the same substring check but apply it inside a loop over
matches rather than a single file.
---
Outside diff comments:
In `@hack/minikube-create-user.sh`:
- Around line 155-162: The context is being created with a user name that
doesn't match the credentials entry: set-credentials creates credentials under
"$USER_NAME" but set-context uses "$CONTEXT_NAME"; update the set-context call
to use the same user key created by set-credentials (i.e., change the --user
value to "$USER_NAME") so the context's user matches the credentials created by
set-credentials (symbols: set-credentials, set-context, $USER_NAME,
$CONTEXT_NAME).
🪄 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: 6786be46-7a6c-4e2c-a8bb-e1938d36fe39
📒 Files selected for processing (3)
.github/workflows/run-e2e-tests.ymle2e-tests/tests/mtc_291_configmap_test.gohack/minikube-create-user.sh
msajidmansoori12
force-pushed
the
mtc_291
branch
from
52ecb0b to
a83db26
Compare
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
msajidmansoori12
force-pushed
the
mtc_291
branch
from
a83db26 to
5104238
Compare
Contributor
Author
|
@CodeRabbit summary |
✅ Actions performedSummary regeneration triggered. |
Contributor
Author
|
@CodeRabbit summary |
✅ Actions performedSummary regeneration triggered. |
msajidmansoori12
changed the title
…ntexts Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
msajidmansoori12
force-pushed
the
mtc_291
branch
from
6b37c65 to
aa8e1b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e-tests/tests/mtc_291_configmap_test.go (1)
82-91: Consider addingbreakafter finding the expected ConfigMap.The loop continues iterating after setting
foundExpectedConfigMap = true. While functionally correct, this is slightly inefficient and could cause the test to fail on a read error for an unrelated ConfigMap file even after the expected one was found.♻️ Proposed fix
for _, match := range matches { content, err := os.ReadFile(match) Expect(err).NotTo(HaveOccurred()) if strings.Contains(string(content), expectedData) { foundExpectedConfigMap = true + break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/tests/mtc_291_configmap_test.go` around lines 82 - 91, The loop reading files in the test uses matches and sets foundExpectedConfigMap to true when expectedData is found but continues iterating; after setting foundExpectedConfigMap = true inside the loop (the block that checks strings.Contains(string(content), expectedData)), add a break to exit the loop immediately so you avoid unnecessary file reads and potential unrelated read errors after the matching ConfigMap is found (update the loop that reads match, err, and checks expectedData).
🤖 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/tests/mtc_291_configmap_test.go`:
- Line 105: The log message uses the wrong variable: replace the format argument
`namespace` in the log.Printf call with the variable that holds the ConfigMap
name (the one used to validate "redis-config", e.g. `configMapName` or
`redisConfig`) so the message reads the ConfigMap name instead of the namespace;
keep the same format string and ensure the chosen identifier is in scope where
log.Printf is called.
---
Nitpick comments:
In `@e2e-tests/tests/mtc_291_configmap_test.go`:
- Around line 82-91: The loop reading files in the test uses matches and sets
foundExpectedConfigMap to true when expectedData is found but continues
iterating; after setting foundExpectedConfigMap = true inside the loop (the
block that checks strings.Contains(string(content), expectedData)), add a break
to exit the loop immediately so you avoid unnecessary file reads and potential
unrelated read errors after the matching ConfigMap is found (update the loop
that reads match, err, and checks expectedData).
🪄 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: 2c01a2ef-ad02-47a8-bde2-85183f363354
📒 Files selected for processing (2)
e2e-tests/framework/client.goe2e-tests/tests/mtc_291_configmap_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e-tests/framework/client.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e-tests/tests/mtc_291_configmap_test.go (1)
82-104: Deduplicate the expected ConfigMap payload literal.The same payload string is asserted twice. Reusing one variable avoids drift between output-dir and target-cluster assertions.
♻️ Proposed refactor
- expectedData := `redis-config: "maxmemory 2mb \nmaxmemory-policy allkeys-lru\n"` + expectedRedisConfigData := `redis-config: "maxmemory 2mb \nmaxmemory-policy allkeys-lru\n"` foundExpectedConfigMap := false for _, match := range matches { content, err := os.ReadFile(match) Expect(err).NotTo(HaveOccurred()) - if strings.Contains(string(content), expectedData) { + if strings.Contains(string(content), expectedRedisConfigData) { foundExpectedConfigMap = true } } Expect(foundExpectedConfigMap).To(BeTrue(), "expected at least one ConfigMap manifest to contain redis-config data") @@ - Expect(output).To(ContainSubstring(`redis-config: "maxmemory 2mb \nmaxmemory-policy allkeys-lru\n"`)) + Expect(output).To(ContainSubstring(expectedRedisConfigData))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/tests/mtc_291_configmap_test.go` around lines 82 - 104, The test repeats the same raw ConfigMap payload literal twice; keep a single canonical variable (expectedData) and reuse it for both checks to avoid drift: locate the expectedData declaration and the Expect(output).To(ContainSubstring(...)) assertion and replace the duplicated literal in the latter with the expectedData variable so both the file-content loop and the kubectl output assertion reference the same symbol (expectedData).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e-tests/tests/mtc_291_configmap_test.go`:
- Around line 82-104: The test repeats the same raw ConfigMap payload literal
twice; keep a single canonical variable (expectedData) and reuse it for both
checks to avoid drift: locate the expectedData declaration and the
Expect(output).To(ContainSubstring(...)) assertion and replace the duplicated
literal in the latter with the expectedData variable so both the file-content
loop and the kubectl output assertion reference the same symbol (expectedData).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ddbcc11-75cb-4c6e-bb19-cb928ab7ce6c
📒 Files selected for processing (1)
e2e-tests/tests/mtc_291_configmap_test.go
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
msajidmansoori12
force-pushed
the
mtc_291
branch
from
8391ca6 to
4e12e4c
Compare
midays
approved these changes
Apr 12, 2026
This was referenced Apr 16, 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
2 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 by CodeRabbit
Tests
Infrastructure