AGENT-1407: add support for InternalReleaseImage related resources #8600
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@andfasano: This pull request references AGENT-1407 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 story to target the "4.22.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. |
WalkthroughAdded a feature-gate-aware internal release-image patcher that updates oc-mirror manifests and bootstrap registries.conf when an InternalReleaseImage exists; wired the patcher into installer manifest generation and bootstrap update flows; bumped Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
size/L
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8600 +/- ##
==========================================
+ Coverage 43.49% 43.54% +0.04%
==========================================
Files 411 412 +1
Lines 71244 71489 +245
==========================================
+ Hits 30987 31127 +140
- Misses 37497 37566 +69
- Partials 2760 2796 +36
🚀 New features to boost your workflow:
|
openshift-ci
bot
added
size/XL
andfasano
force-pushed
the
iri-patch-manifests
branch
from
2343a8c to
759af35
Compare
andfasano
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| if err != nil { | ||
| return err | ||
| } | ||
| u := unstructured.Unstructured{} |
There was a problem hiding this comment.
we usually use 'yaml.Unmarshal' for reading the manifest
There was a problem hiding this comment.
To which type? In this loop we're reading all the extra manifests found - and they could be of any kind. Keeping a generic approach will also support the type switch below
| "regexp" | ||
|
|
||
| config_latest_types "github.com/coreos/ignition/v2/config/v3_2/types" | ||
| "github.com/go-openapi/swag" |
There was a problem hiding this comment.
to include the NoRegistryClusterInstall feature and the most update InternalReleaseImage api
andfasano
force-pushed
the
iri-patch-manifests
branch
from
759af35 to
475e29e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/ignition/internalreleaseimage_patcher.go (1)
20-21: Redundant duplicate import of the same package.Both
yaml(line 20) andk8syaml(line 21) importsigs.k8s.io/yaml. Remove one and use a single alias consistently.🔎 Proposed fix
- "sigs.k8s.io/yaml" - k8syaml "sigs.k8s.io/yaml" + "sigs.k8s.io/yaml"Then update line 81 to use
yaml.Marshalinstead ofk8syaml.Marshal.
📜 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 (35)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_scheduling.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/console/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/console/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/util.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_internalreleaseimage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_machineconfignode.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_osimagestream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_pinnedimageset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
go.modinternal/ignition/installmanifests.gointernal/ignition/internalreleaseimage_patcher.gointernal/ignition/internalreleaseimage_patcher_test.gointernal/manifests/api/interface.gointernal/manifests/api/mock_manifests_api.gointernal/manifests/api/mock_manifests_internal.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:
internal/manifests/api/mock_manifests_internal.gogo.modinternal/manifests/api/interface.gointernal/manifests/api/mock_manifests_api.gointernal/ignition/internalreleaseimage_patcher_test.gointernal/ignition/installmanifests.gointernal/ignition/internalreleaseimage_patcher.go
🧬 Code graph analysis (4)
internal/manifests/api/mock_manifests_internal.go (2)
internal/operators/manager.go (1)
Manifest(44-49)models/manifest.go (1)
Manifest(21-33)
internal/manifests/api/interface.go (2)
internal/operators/manager.go (1)
Manifest(44-49)models/manifest.go (1)
Manifest(21-33)
internal/manifests/api/mock_manifests_api.go (5)
mocks/mock_manifests.go (2)
MockManifestsAPI(17-20)MockManifestsAPIMockRecorder(23-25)client/manifests/v2_update_cluster_manifest_parameters.go (1)
V2UpdateClusterManifestParams(64-83)restapi/operations/manifests/v2_update_cluster_manifest_parameters.go (1)
V2UpdateClusterManifestParams(33-48)internal/operators/manager.go (1)
Manifest(44-49)models/manifest.go (1)
Manifest(21-33)
internal/ignition/internalreleaseimage_patcher_test.go (5)
internal/manifests/api/mock_manifests_api.go (2)
MockManifestsAPI(19-22)NewMockManifestsAPI(30-34)internal/ignition/internalreleaseimage_patcher.go (1)
NewInternalReleaseImagePatcher(56-65)models/update_manifest_params.go (1)
UpdateManifestParams(21-43)pkg/s3wrapper/client.go (1)
ObjectInfo(36-39)internal/releasesources/clients.go (1)
Node(40-42)
🔇 Additional comments (11)
go.mod (1)
53-53: LGTM!Dependency update for
github.com/openshift/apito include theNoRegistryClusterInstallfeature and updatedInternalReleaseImageAPI aligns with the PR objectives.internal/manifests/api/interface.go (1)
24-24: LGTM!The new
UpdateClusterManifestInternalmethod completes the CRUD pattern for cluster manifests and follows the established signature conventions of the interface.internal/manifests/api/mock_manifests_api.go (1)
100-113: LGTM!Auto-generated mock code correctly implements the new
UpdateClusterManifestInternalmethod, following the established gomock patterns.internal/manifests/api/mock_manifests_internal.go (1)
98-112: LGTM!Auto-generated mock code correctly implements the
UpdateClusterManifestInternalmethod forMockClusterManifestsInternals, consistent with the interface definition.internal/ignition/internalreleaseimage_patcher_test.go (2)
29-99: Test structure looks good.The test suite properly sets up gomock controller lifecycle and covers both positive (IRI found) and negative (IRI not found) scenarios. The custom
manifestContainsMatcherprovides readable assertions for manifest content validation.
140-220: Helper functions are well-designed.The test helpers effectively abstract common operations. The
sameRegistriesConfapproach of normalizing TOML through JSON comparison handles formatting differences between expected and actual configs appropriately.internal/ignition/internalreleaseimage_patcher.go (3)
153-278: LGTM!The manifest patching logic is well-structured with appropriate handling for different resource types. The idempotency check via annotations prevents re-patching, and error handling is comprehensive.
46-65: LGTM!The struct design and constructor are well-organized, properly initializing all dependencies needed for manifest patching operations.
86-93: Extract folder from S3 key path to match original manifest location.The S3 key format includes folder information (
clusterID/manifests/[manifests|openshift]/filenameas noted in the code comment), but the folder parameter is hardcoded toopenshiftinstead of being extracted from the key. This causes manifests stored in themanifestsfolder to be uploaded to the wrong location. Extract the folder from the path to preserve the manifest's original location.internal/ignition/installmanifests.go (2)
282-287: LGTM!The feature-gated manifest patching is correctly positioned to patch manifests in S3 before they are downloaded to the working directory. This ensures the patched versions are used during cluster creation.
808-813: LGTM!The bootstrap update is correctly integrated into the existing flow, applying InternalReleaseImage patches before the final ignition file is written.
andfasano
force-pushed
the
iri-patch-manifests
branch
2 times, most recently
from
8a19689 to
a4bba41
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/ignition/internalreleaseimage_patcher.go (1)
20-21: Duplicate import of the same package.
sigs.k8s.io/yamlis imported twice - once asyaml(line 20) and again ask8syaml(line 21). Only one is needed.🔎 Suggested fix
- "sigs.k8s.io/yaml" - k8syaml "sigs.k8s.io/yaml" + "sigs.k8s.io/yaml"Then update line 81 to use
yaml.Marshalinstead ofk8syaml.Marshal.
📜 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 selected for processing (6)
internal/ignition/installmanifests.gointernal/ignition/internalreleaseimage_patcher.gointernal/ignition/internalreleaseimage_patcher_test.gointernal/manifests/api/interface.gointernal/manifests/api/mock_manifests_api.gointernal/manifests/api/mock_manifests_internal.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/manifests/api/mock_manifests_internal.go
- internal/ignition/installmanifests.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:
internal/ignition/internalreleaseimage_patcher_test.gointernal/manifests/api/interface.gointernal/ignition/internalreleaseimage_patcher.gointernal/manifests/api/mock_manifests_api.go
🧬 Code graph analysis (3)
internal/manifests/api/interface.go (2)
internal/operators/manager.go (1)
Manifest(44-49)models/manifest.go (1)
Manifest(21-33)
internal/ignition/internalreleaseimage_patcher.go (3)
internal/manifests/api/interface.go (1)
ManifestsAPI(13-16)models/update_manifest_params.go (2)
UpdateManifestParams(21-43)UpdateManifestParamsFolderOpenshift(102-102)pkg/s3wrapper/client.go (1)
ObjectInfo(36-39)
internal/manifests/api/mock_manifests_api.go (4)
mocks/mock_manifests.go (2)
MockManifestsAPI(17-20)MockManifestsAPIMockRecorder(23-25)client/manifests/v2_update_cluster_manifest_parameters.go (1)
V2UpdateClusterManifestParams(64-83)restapi/operations/manifests/v2_update_cluster_manifest_parameters.go (1)
V2UpdateClusterManifestParams(33-48)models/manifest.go (1)
Manifest(21-33)
🔇 Additional comments (12)
internal/manifests/api/interface.go (1)
24-24: LGTM!The new
UpdateClusterManifestInternalmethod follows the established interface pattern, consistent with other methods likeCreateClusterManifestInternal.internal/manifests/api/mock_manifests_api.go (1)
100-113: Generated code - no review required.This is auto-generated mock code from MockGen that correctly implements the new
UpdateClusterManifestInternalinterface method.internal/ignition/internalreleaseimage_patcher_test.go (4)
29-32: LGTM!Test suite entry point is correctly set up with Ginkgo.
58-84: LGTM!Good test coverage for the IRI patching scenario - validates both bootstrap registries.conf updates and manifest patching with appropriate mock expectations.
101-138: LGTM!Well-implemented custom gomock matcher that validates manifest update parameters including base64-decoded content.
199-220: LGTM!The TOML comparison via JSON serialization is a clever approach to handle formatting differences in registries.conf comparisons.
internal/ignition/internalreleaseimage_patcher.go (6)
46-65: LGTM!Clean struct design with appropriate separation of concerns. The constructor properly initializes the registry domain from cluster metadata.
67-74: LGTM!The regex-based host replacement is a clean approach for rewriting mirror URLs while preserving the path component.
76-99: LGTM!The upload function correctly extracts the filename and encodes content for the manifests API.
156-278: LGTM!Well-structured manifest patching with proper idempotence via annotations. The type-switch approach handles all required manifest types cleanly.
303-327: LGTM!Bootstrap update correctly extracts, patches, and re-encodes the registries.conf with appropriate IRI mirrors.
329-370: LGTM!The TOML manipulation correctly appends new mirror entries for each existing registry mirror, preserving the original configuration while extending it.
andfasano
force-pushed
the
iri-patch-manifests
branch
from
a4bba41 to
a5e27fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/ignition/internalreleaseimage_patcher.go (2)
66-69: Consider validating mirror format or handling edge cases.The regex
^[^/]+replaces everything before the first/in the mirror string. If the input mirror doesn't follow the expectedhost:port/pathformat (e.g., missing path component, malformed URL), the replacement could produce invalid results.Consider adding validation or defensive checks:
func (i *internalReleaseImagePatcher) patchMirror(origMirror string, host string) string { if !strings.Contains(origMirror, "/") { i.log.Warnf("Mirror %s does not contain path separator, using as-is", origMirror) return fmt.Sprintf("%s:%d", host, iriRegistryPort) } re := regexp.MustCompile(`^[^/]+`) return re.ReplaceAllString(origMirror, fmt.Sprintf("%s:%d", host, iriRegistryPort)) }
186-227: Consider extracting common mirror-patching logic.The IDMS and ITMS cases have nearly identical logic for iterating mirrors and appending api-int/localhost variants. Extracting this into a helper would reduce duplication and improve maintainability.
🔎 Proposed refactor
Add a helper method:
func (i *internalReleaseImagePatcher) patchMirrorList(mirrors []configv1.ImageMirror) []configv1.ImageMirror { iriMirrors := []configv1.ImageMirror{} for _, m := range mirrors { iriMirrors = append(iriMirrors, i.patchImageMirror(m, i.iriRegistryDomain)) iriMirrors = append(iriMirrors, i.patchImageMirror(m, "localhost")) } return append(mirrors, iriMirrors...) }Then simplify the switch cases:
case "ImageDigestMirrorSet": i.log.Infof("Patching ImageDigestMirrorSet manifest %s", f.Path) var idms configv1.ImageDigestMirrorSet if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &idms); err != nil { return err } i.markAsPatched(&idms) for j, group := range idms.Spec.ImageDigestMirrors { - iriMirrors := []configv1.ImageMirror{} - for _, m := range group.Mirrors { - iriMirrors = append(iriMirrors, i.patchImageMirror(m, i.iriRegistryDomain)) - iriMirrors = append(iriMirrors, i.patchImageMirror(m, "localhost")) - } - idms.Spec.ImageDigestMirrors[j].Mirrors = append(idms.Spec.ImageDigestMirrors[j].Mirrors, iriMirrors...) + idms.Spec.ImageDigestMirrors[j].Mirrors = i.patchMirrorList(group.Mirrors) }Similar change for ImageTagMirrorSet.
📜 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 selected for processing (6)
internal/ignition/installmanifests.gointernal/ignition/internalreleaseimage_patcher.gointernal/ignition/internalreleaseimage_patcher_test.gointernal/manifests/api/interface.gointernal/manifests/api/mock_manifests_api.gointernal/manifests/api/mock_manifests_internal.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/manifests/api/mock_manifests_internal.go
- internal/manifests/api/interface.go
- internal/manifests/api/mock_manifests_api.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:
internal/ignition/internalreleaseimage_patcher_test.gointernal/ignition/internalreleaseimage_patcher.gointernal/ignition/installmanifests.go
🧬 Code graph analysis (3)
internal/ignition/internalreleaseimage_patcher_test.go (3)
internal/manifests/api/mock_manifests_api.go (2)
MockManifestsAPI(19-22)NewMockManifestsAPI(30-34)models/update_manifest_params.go (1)
UpdateManifestParams(21-43)pkg/s3wrapper/client.go (1)
ObjectInfo(36-39)
internal/ignition/internalreleaseimage_patcher.go (5)
internal/manifests/api/interface.go (1)
ManifestsAPI(13-16)models/update_manifest_params.go (2)
UpdateManifestParams(21-43)UpdateManifestParamsFolderOpenshift(102-102)pkg/jq/jq_variable.go (1)
String(8-13)pkg/s3wrapper/client.go (1)
ObjectInfo(36-39)internal/usage/manager.go (1)
Unmarshal(79-86)
internal/ignition/installmanifests.go (2)
internal/ignition/internalreleaseimage_patcher.go (1)
NewInternalReleaseImagePatcher(55-64)internal/installcfg/installcfg.go (1)
InstallerConfigBaremetal(242-286)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / assisted-service-saas-main-on-pull-request
- GitHub Check: Red Hat Konflux / assisted-service-rhel9-acm-ds-main-on-pull-request
🔇 Additional comments (13)
internal/ignition/internalreleaseimage_patcher.go (5)
75-98: LGTM!The uploadManifests method correctly marshals, encodes, and uploads patched manifests via the manifests API. The hardcoded "openshift" folder is appropriate for IRI-related resources.
229-273: LGTM!The ClusterCatalog and CatalogSource patching logic correctly uses the unstructured API to handle dynamic resources. Error messages include resource names for debugging.
371-381: Verify the hardcodedinsecure: falsesetting.The
newMirrorTreemethod always setsinsecure: falsefor IRI mirrors. Confirm whether this is intentional (for security reasons) or if it should inherit the insecure setting from the source mirror.If localhost/api-int mirrors should allow insecure connections in some scenarios, consider passing the insecure flag as a parameter or inheriting it from the source mirror configuration.
279-300: LGTM!The method correctly extracts and decodes registries.conf from bootstrap ignition, with appropriate error handling.
112-150: LGTM!The idempotence logic using annotations prevents duplicate patching. The getInternalReleaseImageManifest method appropriately handles decode errors with debug logging while searching for the IRI manifest.
internal/ignition/internalreleaseimage_patcher_test.go (4)
28-98: LGTM!The test suite is well-structured with proper setup/teardown and clear context organization. Tests cover both scenarios (IRI found/not found) and verify expected behavior using appropriate matchers.
100-137: LGTM!The
ManifestContainscustom matcher is well-implemented, properly decoding base64 content and verifying expected strings. TheString()method provides readable error messages.
139-219: LGTM!The test helpers are well-designed and reusable. The
sameRegistriesConffunction cleverly uses JSON marshaling to compare TOML structures, handling ordering differences effectively.
221-359: LGTM!The embedded test data is comprehensive and makes the tests self-contained. The expected registries.conf correctly shows api-int and localhost mirrors added to each registry.
internal/ignition/installmanifests.go (4)
146-147: LGTM!The new fields are cleanly integrated into the
installerGeneratorstruct, and the constructor properly initializes theiriPatcherwith required dependencies.Also applies to: 179-179
189-199: LGTM!The
patchInternalReleaseManifestsmethod appropriately guards the patching logic with a feature gate check and provides clear error logging.
216-216: LGTM!The integration points are well-placed: manifest patching occurs after retrieval but before pre-create hooks, and bootstrap patching is properly guarded by the feature gate check.
Also applies to: 293-296, 817-822
1360-1387: Consider using OpenShift API utilities for feature gate parsing.The method manually parses feature gate strings (splitting on
=). While thegithub.com/openshift/api/featurespackage is imported, verify whether it provides utilities for parsing feature gates from install config to avoid reinventing the wheel.If no standard utilities exist, the current implementation is acceptable but should improve error handling as noted in the previous comment.
| func (g *installerGenerator) isFeatureGateEnabled(feature v1.FeatureGateName) bool { | ||
| var installConfig installcfg.InstallerConfigBaremetal | ||
|
|
||
| if err := json.Unmarshal(g.rawInstallConfig, &installConfig); err != nil { | ||
| g.log.Errorf("cannot convert install config data while checking feature gate %s", string(feature)) | ||
| return false | ||
| } | ||
| for _, fg := range installConfig.FeatureGates { | ||
| // Parse feature gate | ||
| featureParts := strings.Split(fg, "=") | ||
| if len(featureParts) != 2 { | ||
| g.log.Debugf("Cannot parse feature gate %s, skipping", fg) | ||
| continue | ||
| } | ||
| featureName := featureParts[0] | ||
| featureEnabled, err := strconv.ParseBool(featureParts[1]) | ||
| if err != nil { | ||
| g.log.Debugf("Unsupported feature value %s, skipping", fg) | ||
| continue | ||
| } | ||
|
|
||
| if featureName == string(feature) && featureEnabled { | ||
| g.log.Debugf("Feature gate %s is enabled", string(feature)) | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Silent error handling could mask configuration issues.
The isFeatureGateEnabled method logs but doesn't return errors from json.Unmarshal (line 1363), causing it to return false even when the install config is malformed. This could mask configuration errors that should be surfaced.
Additionally, the manual parsing of feature gates (lines 1367-1379) via string splitting could be fragile. Consider whether there's a standard utility in the OpenShift API for parsing feature gates, or at least add more robust error handling for malformed feature gate strings.
🔎 Suggested improvements
- Consider returning errors from
isFeatureGateEnabled:
-func (g *installerGenerator) isFeatureGateEnabled(feature v1.FeatureGateName) bool {
+func (g *installerGenerator) isFeatureGateEnabled(feature v1.FeatureGateName) (bool, error) {
var installConfig installcfg.InstallerConfigBaremetal
if err := json.Unmarshal(g.rawInstallConfig, &installConfig); err != nil {
g.log.Errorf("cannot convert install config data while checking feature gate %s", string(feature))
- return false
+ return false, err
}Then update callers to handle the error.
- Add more robust parsing:
for _, fg := range installConfig.FeatureGates {
// Parse feature gate
- featureParts := strings.Split(fg, "=")
+ featureParts := strings.SplitN(fg, "=", 2)
if len(featureParts) != 2 {
g.log.Debugf("Cannot parse feature gate %s, skipping", fg)
continue
}
featureName := featureParts[0]
- featureEnabled, err := strconv.ParseBool(featureParts[1])
+ // strconv.ParseBool accepts "1", "t", "T", "TRUE", "true", "True", etc.
+ featureEnabled, err := strconv.ParseBool(strings.TrimSpace(featureParts[1]))
if err != nil {
g.log.Debugf("Unsupported feature value %s, skipping", fg)
continue
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/ignition/installmanifests.go around lines 1360 to 1387, the
isFeatureGateEnabled function swallows json.Unmarshal errors and returns false
for malformed configs and uses fragile string-splitting for feature gate
parsing; change the signature to return (bool, error) so callers can handle JSON
unmarshal failures (propagate and surface the actual error), log or wrap the
unmarshalling error with context instead of silently returning false, and
replace the manual split with stricter parsing/validation (or call the OpenShift
feature-gate parsing utility if available) so malformed entries produce a clear
error/log and are skipped safely; update all callers to handle the new (bool,
error) return and adjust unit tests accordingly.
oc mirror manifests generated by the appliance are patched with additional mirrors on localhost and api-int
andfasano
force-pushed
the
iri-patch-manifests
branch
from
a5e27fd to
6e0a31a
Compare
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 selected for processing (6)
internal/ignition/installmanifests.gointernal/ignition/internalreleaseimage_patcher.gointernal/ignition/internalreleaseimage_patcher_test.gointernal/manifests/api/interface.gointernal/manifests/api/mock_manifests_api.gointernal/manifests/api/mock_manifests_internal.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/manifests/api/mock_manifests_internal.go
- internal/ignition/internalreleaseimage_patcher_test.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:
internal/manifests/api/interface.gointernal/ignition/internalreleaseimage_patcher.gointernal/ignition/installmanifests.gointernal/manifests/api/mock_manifests_api.go
🧬 Code graph analysis (4)
internal/manifests/api/interface.go (2)
internal/operators/manager.go (1)
Manifest(44-49)models/manifest.go (1)
Manifest(21-33)
internal/ignition/internalreleaseimage_patcher.go (3)
internal/manifests/api/interface.go (1)
ManifestsAPI(13-16)models/update_manifest_params.go (2)
UpdateManifestParams(21-43)UpdateManifestParamsFolderOpenshift(102-102)pkg/s3wrapper/client.go (1)
ObjectInfo(36-39)
internal/ignition/installmanifests.go (3)
internal/ignition/internalreleaseimage_patcher.go (1)
NewInternalReleaseImagePatcher(55-64)pkg/s3wrapper/client.go (1)
ObjectInfo(36-39)internal/installcfg/installcfg.go (1)
InstallerConfigBaremetal(242-286)
internal/manifests/api/mock_manifests_api.go (4)
mocks/mock_manifests.go (2)
MockManifestsAPI(17-20)MockManifestsAPIMockRecorder(23-25)client/manifests/v2_update_cluster_manifest_parameters.go (1)
V2UpdateClusterManifestParams(64-83)restapi/operations/manifests/v2_update_cluster_manifest_parameters.go (1)
V2UpdateClusterManifestParams(33-48)models/manifest.go (1)
Manifest(21-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / assisted-service-saas-main-on-pull-request
- GitHub Check: Red Hat Konflux / assisted-service-rhel9-acm-ds-main-on-pull-request
🔇 Additional comments (11)
internal/manifests/api/interface.go (1)
24-24: LGTM! Interface extension follows established patterns.The new
UpdateClusterManifestInternalmethod maintains consistency with existing interface methods in naming, signature structure, and return types, properly supporting manifest update operations.internal/manifests/api/mock_manifests_api.go (1)
100-113: LGTM! Generated mock code follows gomock conventions.The auto-generated mock implementation for
UpdateClusterManifestInternalcorrectly follows the established gomock pattern used throughout the file.internal/ignition/internalreleaseimage_patcher.go (6)
31-64: LGTM! Clean initialization and constants.The patcher type definition and constructor correctly initialize the internal release image patcher with appropriate domain construction and field initialization.
66-73: LGTM! Mirror patching logic is sound.The regex-based mirror patching correctly replaces the host:port portion while preserving the path components.
100-150: LGTM! Manifest retrieval and idempotence checks are well implemented.The manifest content retrieval, annotation-based idempotence checking, and InternalReleaseImage discovery logic are correctly implemented with appropriate error handling.
152-277: LGTM! Manifest patching logic correctly handles multiple resource types.The method appropriately handles ImageDigestMirrorSet, ImageTagMirrorSet, ClusterCatalog, and CatalogSource resources, adding api-int and localhost mirrors or updating image references as needed, with proper idempotence checks.
279-326: LGTM! Bootstrap registries.conf update logic is correctly implemented.The extraction of registries.conf from bootstrap ignition, content update, and re-encoding logic properly handles the ignition data URL format.
371-381: No action needed. The hardcodedinsecure: falsesetting for internal mirrors is correct and matches the expected test behavior.The test suite in
internalreleaseimage_patcher_test.goexplicitly validates that all mirror entries for port 22625 registries (api-int and localhost) should haveinsecure = false. This indicates the internal registries are designed to use TLS with valid certificates, not self-signed or unencrypted connections.Likely an incorrect or invalid review comment.
internal/ignition/installmanifests.go (3)
16-16: LGTM! Appropriate imports and field additions.The new imports support feature gate checking and internal release image patching. The added fields (
iriPatcherandrawInstallConfig) are properly typed for their respective purposes.Also applies to: 24-25, 31-31, 146-147
179-179: LGTM! Feature-gated patching correctly integrated.The
iriPatcherinitialization and the feature-gate-awarepatchInternalReleaseManifestsmethod properly integrate the internal release image patching capability into the installer generator.Also applies to: 189-199
216-216: LGTM! Integration points properly ordered and feature-gated.The install config storage, manifest patching invocation, and bootstrap update are correctly positioned in the generation workflow with appropriate feature gate checks and error handling.
Also applies to: 293-296, 817-822
| func (i *internalReleaseImagePatcher) uploadManifests(ctx context.Context, key string, obj interface{}) error { | ||
| // manifest has full path as object-key on s3: clusterID/manifests/[manifests|openshift]/filename | ||
| fileName := path.Base(key) | ||
| i.log.Infof("Updating resource %s as %s", key, fileName) | ||
|
|
||
| data, err := k8syaml.Marshal(obj) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| params := operations.V2UpdateClusterManifestParams{ | ||
| ClusterID: *i.cluster.ID, | ||
| UpdateManifestParams: &models.UpdateManifestParams{ | ||
| FileName: fileName, | ||
| Folder: models.UpdateManifestParamsFolderOpenshift, | ||
| UpdatedContent: swag.String(base64.StdEncoding.EncodeToString(data)), | ||
| }, | ||
| } | ||
| _, err = i.manifestApi.UpdateClusterManifestInternal(ctx, params) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Extract folder from manifest path instead of hardcoding.
Line 89 hardcodes the folder to openshift, but the manifest could be in either manifests or openshift folders per the comment at line 76. Using path.Base(key) at line 77 discards the folder information, potentially causing manifests to be updated in the wrong location.
🔎 Recommended fix to extract folder from path
func (i *internalReleaseImagePatcher) uploadManifests(ctx context.Context, key string, obj interface{}) error {
- // manifest has full path as object-key on s3: clusterID/manifests/[manifests|openshift]/filename
- fileName := path.Base(key)
+ // manifest has full path as object-key on s3: clusterID/manifests/[manifests|openshift]/filename
+ pathParts := strings.Split(key, "/")
+ if len(pathParts) < 4 {
+ return fmt.Errorf("unexpected manifest path format: %s", key)
+ }
+ folder := pathParts[2] // "manifests" or "openshift"
+ fileName := pathParts[3]
i.log.Infof("Updating resource %s as %s", key, fileName)
data, err := k8syaml.Marshal(obj)
if err != nil {
return err
}
params := operations.V2UpdateClusterManifestParams{
ClusterID: *i.cluster.ID,
UpdateManifestParams: &models.UpdateManifestParams{
FileName: fileName,
- Folder: models.UpdateManifestParamsFolderOpenshift,
+ Folder: folder,
UpdatedContent: swag.String(base64.StdEncoding.EncodeToString(data)),
},
}📝 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 (i *internalReleaseImagePatcher) uploadManifests(ctx context.Context, key string, obj interface{}) error { | |
| // manifest has full path as object-key on s3: clusterID/manifests/[manifests|openshift]/filename | |
| fileName := path.Base(key) | |
| i.log.Infof("Updating resource %s as %s", key, fileName) | |
| data, err := k8syaml.Marshal(obj) | |
| if err != nil { | |
| return err | |
| } | |
| params := operations.V2UpdateClusterManifestParams{ | |
| ClusterID: *i.cluster.ID, | |
| UpdateManifestParams: &models.UpdateManifestParams{ | |
| FileName: fileName, | |
| Folder: models.UpdateManifestParamsFolderOpenshift, | |
| UpdatedContent: swag.String(base64.StdEncoding.EncodeToString(data)), | |
| }, | |
| } | |
| _, err = i.manifestApi.UpdateClusterManifestInternal(ctx, params) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| func (i *internalReleaseImagePatcher) uploadManifests(ctx context.Context, key string, obj interface{}) error { | |
| // manifest has full path as object-key on s3: clusterID/manifests/[manifests|openshift]/filename | |
| pathParts := strings.Split(key, "/") | |
| if len(pathParts) < 4 { | |
| return fmt.Errorf("unexpected manifest path format: %s", key) | |
| } | |
| folder := pathParts[2] // "manifests" or "openshift" | |
| fileName := pathParts[3] | |
| i.log.Infof("Updating resource %s as %s", key, fileName) | |
| data, err := k8syaml.Marshal(obj) | |
| if err != nil { | |
| return err | |
| } | |
| params := operations.V2UpdateClusterManifestParams{ | |
| ClusterID: *i.cluster.ID, | |
| UpdateManifestParams: &models.UpdateManifestParams{ | |
| FileName: fileName, | |
| Folder: folder, | |
| UpdatedContent: swag.String(base64.StdEncoding.EncodeToString(data)), | |
| }, | |
| } | |
| _, err = i.manifestApi.UpdateClusterManifestInternal(ctx, params) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
internal/ignition/internalreleaseimage_patcher.go lines 75-98: the code
currently discards the manifest folder and hardcodes FolderOpenshift; instead
parse the key path to extract the folder segment (the directory immediately
containing the filename), map that segment to the appropriate
models.UpdateManifestParamsFolder enum (e.g.,
models.UpdateManifestParamsFolderOpenshift or
models.UpdateManifestParamsFolderManifests), and use that value in the params;
keep using path.Base(key) for fileName, validate or default the folder if the
segment is missing or unrecognized, and return an error if necessary.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano, danielerez The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@andfasano: The following tests failed, say
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. |
3 participants
oc mirror manifests generated by the appliance are patched with additional mirrors on localhost and api-int.
See openshift/enhancements#1821
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist