CLID-602: Test for enclave scenario#1411
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@nidangavali: This pull request references CLID-602 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds an enclave mirror-to-mirror integration test (two-step flow), two ISC templates (step-1 filters, step-2 full with placeholder), and test helpers to validate mirrored catalogs, image sets, excluded packages, IDMS, and registry parity. ChangesEnclave Integration Test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 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 (2)
tests/integration/helpers_test.go (1)
783-791: ⚡ Quick winEnsure extracted catalog dirs are always cleaned up on assertion failures.
configsDircleanup currently runs only if the loop reaches Line 790. Any earlier failedExpectskips that line and leaks temp dirs.Proposed fix
func expectPackagesNotInMirroredCatalog(ctx context.Context, reg registry.Registry, iscPath string, packages ...string) { cfg := parseImageSetConfig(iscPath) for _, op := range cfg.Mirror.Operators { configsDir := extractCatalogConfigs(ctx, reg, op.Catalog) + defer func(dir string) { + Expect(os.RemoveAll(dir)).To(Succeed()) + }(configsDir) + for _, pkg := range packages { pkgDir := filepath.Join(configsDir, pkg) _, err := os.Stat(pkgDir) Expect(os.IsNotExist(err)).To(BeTrue(), "package %q should not be present in mirrored catalog %s", pkg, op.Catalog) } - Expect(os.RemoveAll(configsDir)).To(Succeed()) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/helpers_test.go` around lines 783 - 791, The temp directory returned by extractCatalogConfigs (configsDir) is only removed at the end and is skipped on early test failures; immediately after creating configsDir in the test, register a cleanup (e.g., defer os.RemoveAll(configsDir) or use the test framework's cleanup helper) so configsDir is always removed even if Expect assertions fail; update the block that assigns configsDir (where extractCatalogConfigs(ctx, reg, op.Catalog) is called) to perform the cleanup registration right away and keep the final Expect(os.RemoveAll(...)) removal optional/harmless.tests/integration/enclave_test.go (1)
72-73: ⚡ Quick winAvoid hardcoding the rendered catalog repository/tag.
Hardcoding
oc-mirror/oc-mirror-dev:test-catalog-latestcouples step-2 rendering to one specific source template value. Passing the computed catalog ref from step-1 keeps both steps consistent and avoids drift.Proposed refactor
-iscFullPath := renderEnclaveFullISC(workDir2, iscFullTemplate, testRegistry.Endpoint()) +sourceCfg := parseImageSetConfig(iscSourcePath) +Expect(sourceCfg.Mirror.Operators).NotTo(BeEmpty(), "step-1 ISC has no operators") +step1Catalog := sourceCfg.Mirror.Operators[0].Catalog +catalogRef := testRegistry.Endpoint() + "/" + extractRepositoryName(step1Catalog) + ":" + extractTag(step1Catalog) +iscFullPath := renderEnclaveFullISC(workDir2, iscFullTemplate, catalogRef)-func renderEnclaveFullISC(workDir, templateRelPath, intermediateEndpoint string) string { +func renderEnclaveFullISC(workDir, templateRelPath, catalogRef string) string { ... - catalogRef := intermediateEndpoint + "/oc-mirror/oc-mirror-dev:test-catalog-latest" content := strings.Replace(string(templateData), placeholder, catalogRef, 1)Also applies to: 101-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/enclave_test.go` around lines 72 - 73, The rendered catalog ref is being hardcoded in the step-2 template; instead update the call to renderEnclaveFullISC (and any other uses around lines 101-108) to accept and use the computed catalog reference produced in step-1 (the variable returned/assigned there) rather than a literal like "oc-mirror/oc-mirror-dev:test-catalog-latest", so pass that computed catalog ref into iscFullTemplate and into renderEnclaveFullISC (and log it via GinkgoWriter.Printf) to keep step-1 and step-2 consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/enclave_test.go`:
- Around line 27-39: Test depends on an external quay.io catalog (referenced
from the ISC files isc-enclave-source.yaml / isc-enclave-full.yaml used via
iscSource and iscFullTemplate and the test "It should mirror operators to an
intermediate registry and then forward to a second registry"), making the test
flaky/offline-unfriendly; update the test setup to preload that catalog into a
local test registry fixture and point the ISC files (or the
iscSourcePath/iscFullTemplate usage) to the local registry address instead of
quay.io, or add logic in the test to detect offline mode and substitute a
locally seeded registry image before calling the mirroring flow so
expectedBundles remains unchanged and the test no longer depends on public
internet access.
- Around line 104-110: Before calling strings.Replace, assert the template
actually contains the placeholder to catch regressions: check
strings.Contains(string(templateData), placeholder) (or use
Expect(string(templateData)).To(ContainSubstring(placeholder), ...)) referencing
the existing templateData and placeholder variables; then perform the
replacement (content := strings.Replace(...)) and keep the existing
Expect(content).NotTo(ContainSubstring(placeholder), ...) assertion to verify
the placeholder was removed.
---
Nitpick comments:
In `@tests/integration/enclave_test.go`:
- Around line 72-73: The rendered catalog ref is being hardcoded in the step-2
template; instead update the call to renderEnclaveFullISC (and any other uses
around lines 101-108) to accept and use the computed catalog reference produced
in step-1 (the variable returned/assigned there) rather than a literal like
"oc-mirror/oc-mirror-dev:test-catalog-latest", so pass that computed catalog ref
into iscFullTemplate and into renderEnclaveFullISC (and log it via
GinkgoWriter.Printf) to keep step-1 and step-2 consistent.
In `@tests/integration/helpers_test.go`:
- Around line 783-791: The temp directory returned by extractCatalogConfigs
(configsDir) is only removed at the end and is skipped on early test failures;
immediately after creating configsDir in the test, register a cleanup (e.g.,
defer os.RemoveAll(configsDir) or use the test framework's cleanup helper) so
configsDir is always removed even if Expect assertions fail; update the block
that assigns configsDir (where extractCatalogConfigs(ctx, reg, op.Catalog) is
called) to perform the cleanup registration right away and keep the final
Expect(os.RemoveAll(...)) removal optional/harmless.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 47c1e7d1-6630-4bf6-8644-b21d03c4c5e0
📒 Files selected for processing (4)
tests/integration/enclave_test.gotests/integration/helpers_test.gotests/integration/testdata/imagesetconfigs/enclave/isc-enclave-full.yamltests/integration/testdata/imagesetconfigs/enclave/isc-enclave-source.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/enclave_test.go (1)
50-54:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRemove public internet dependency from the enclave test path.
Line 53 executes step-1 against an ISC that pulls a public
quay.iocatalog, which makes this scenario flaky/offline-unfriendly in CI. Please seed/use a local fixture catalog (or explicitly skip when offline) so the test is self-contained.As per coding guidelines "When new Ginkgo e2e tests are added, check whether they make assumptions about IPv4 networking or require connectivity to external/public internet services."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/enclave_test.go` around lines 50 - 54, The test calls runner.MirrorToMirror using iscSourcePath that points at a public quay.io catalog, causing flaky/offline CI runs; update the test to use a local fixture catalog (seed a local registry image catalog and set iscSourcePath to that fixture) or add an explicit offline skip guard before the call (e.g., a utility like SkipIfOffline or a simple network check) so the step-1 mirror does not pull from the public internet; change the reference passed into runner.MirrorToMirror (iscSourcePath) and/or the test setup that populates testRegistry so it points to the seeded local catalog, or wrap the runner.MirrorToMirror invocation with the offline-skip guard to avoid external network dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/integration/enclave_test.go`:
- Around line 50-54: The test calls runner.MirrorToMirror using iscSourcePath
that points at a public quay.io catalog, causing flaky/offline CI runs; update
the test to use a local fixture catalog (seed a local registry image catalog and
set iscSourcePath to that fixture) or add an explicit offline skip guard before
the call (e.g., a utility like SkipIfOffline or a simple network check) so the
step-1 mirror does not pull from the public internet; change the reference
passed into runner.MirrorToMirror (iscSourcePath) and/or the test setup that
populates testRegistry so it points to the seeded local catalog, or wrap the
runner.MirrorToMirror invocation with the offline-skip guard to avoid external
network dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0c406ab0-fc70-4753-ac02-451be4ed07a0
📒 Files selected for processing (4)
tests/integration/enclave_test.gotests/integration/helpers_test.gotests/integration/testdata/imagesetconfigs/enclave/isc-enclave-full.yamltests/integration/testdata/imagesetconfigs/enclave/isc-enclave-source.yaml
✅ Files skipped from review due to trivial changes (1)
- tests/integration/testdata/imagesetconfigs/enclave/isc-enclave-full.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/helpers_test.go (1)
770-777:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix sibling-repository false positives in
idmsSourceCoversRef.The current
strings.HasPrefix(ref, src)check incorrectly matches sibling paths (e.g.,quay.io/openshift/release-imagesmatches sourcequay.io/openshift/release), allowing IDMS validation to pass when the exact source is absent. This affects validation of platform releases, operator catalogs, and additional images.Use exact match or path-boundary check instead:
Proposed fix
func idmsSourceCoversRef(sources []string, ref string) bool { for _, src := range sources { - if strings.HasPrefix(ref, src) { + src = strings.TrimSuffix(src, "/") + if ref == src || strings.HasPrefix(ref, src+"/") { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/helpers_test.go` around lines 770 - 777, idmsSourceCoversRef currently uses strings.HasPrefix(ref, src) which yields false positives for sibling repository names; update the function to match only exact repo boundaries by returning true only if ref == src or ref starts with src + "/" or src + ":" (to allow tags) — i.e., replace the plain HasPrefix check in idmsSourceCoversRef with a boundary-aware check: ref == src || strings.HasPrefix(ref, src+"/") || strings.HasPrefix(ref, src+":").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/integration/helpers_test.go`:
- Around line 770-777: idmsSourceCoversRef currently uses strings.HasPrefix(ref,
src) which yields false positives for sibling repository names; update the
function to match only exact repo boundaries by returning true only if ref ==
src or ref starts with src + "/" or src + ":" (to allow tags) — i.e., replace
the plain HasPrefix check in idmsSourceCoversRef with a boundary-aware check:
ref == src || strings.HasPrefix(ref, src+"/") || strings.HasPrefix(ref,
src+":").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fafc70ed-f18d-445e-9f35-ab5c4fad5068
📒 Files selected for processing (4)
tests/integration/enclave_test.gotests/integration/helpers_test.gotests/integration/testdata/imagesetconfigs/enclave/isc-enclave-full.yamltests/integration/testdata/imagesetconfigs/enclave/isc-enclave-source.yaml
✅ Files skipped from review due to trivial changes (1)
- tests/integration/testdata/imagesetconfigs/enclave/isc-enclave-source.yaml
|
@aguidirh can you take a look at this one, since you are more familiar with how enclaves work? |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adolfo-ab, nidangavali 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 |
|
/retest |
1 similar comment
|
/retest |
|
@nidangavali: 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. |
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks for this PR, I added few comments.
Please let me know if you have question about them.
Also, please fill the PR description following the template on .github directory, it is important for future references. If possible include context to the commit message.
| By("verifying IDMS from the first mirror contains expected content") | ||
| expectCorrectIDMS(workDir, iscSourcePath) |
There was a problem hiding this comment.
I'm not sure if we should test IDMS here since the idea is not to test cluster resources generation.
| By("verifying the intermediate catalog contains only filtered operator bundles") | ||
| expectCatalogBundlesMatchISC(ctx, *testRegistry, iscSourcePath, expectedBundles) | ||
| expectPackagesNotInMirroredCatalog(ctx, *testRegistry, iscSourcePath, "baz") |
There was a problem hiding this comment.
Wondering if we should be more focused when testing one feature, for example, when testing enclave support, should we also test if the catalog was filtered or if cluster resources where generated correctly?
cc: @adolfo-ab what do you think about it?
There was a problem hiding this comment.
I think we should only test what's necessary and specific to each test. So if the enclave scenarios do not have any particularity wrt to catalog filtering, and we're already checking catalog filtering in other tests, then we don't need to verify that here. Same for cluster resources.
| apiVersion: mirror.openshift.io/v2alpha1 | ||
| mirror: | ||
| operators: | ||
| - catalog: __INTERMEDIATE_CATALOG__ |
There was a problem hiding this comment.
There is one missing piece here, when we are doing enclave support the catalog will be like the original one, something like - catalog: quay.io/oc-mirror/oc-mirror-dev:test-catalog-latest, using full: true is not the way of testing enclave support. Instead we need to place a registries.conf file on the expected directory and add the mirror map, which will tell the cluster, if you find quay.io/oc-mirror/oc-mirror-dev:test-catalog-latest get it from CUSTOMER_HOST/oc-mirror/oc-mirror-dev:test-catalog-latest
This is also true for other types like releases, helm and additional images.
| iscSourcePath := filepath.Join(iscDir, iscSource) | ||
|
|
||
| By("starting a second registry for the enclave destination") | ||
| secondRegistry, err := registry.Start(ctx, registryConfig, 5001) |
There was a problem hiding this comment.
Consider using a dynamic port allocation (OS assigned) to avoid conflicts
4 participants
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Github / Jira issue:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Expected Outcome
Please describe the outcome expected from the tests.
Summary by CodeRabbit