MGMT-22081: Add proxy, NTP, network config, and rendezvous IP to OVE #8144
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@omer-vishlitzky: This pull request explicitly references no jira issue. In 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. |
openshift-ci
bot
added
the
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8144 +/- ##
==========================================
+ Coverage 43.23% 43.38% +0.15%
==========================================
Files 405 405
Lines 70506 70797 +291
==========================================
+ Hits 30480 30716 +236
- Misses 37292 37339 +47
- Partials 2734 2742 +8
🚀 New features to boost your workflow:
|
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/cc @andfasano |
|
/retest |
internal/ignition/disconnected.go
Outdated
| if infraEnv.AdditionalNtpSources != "" { | ||
| // AdditionalNtpSources is a comma-separated string, convert to slice | ||
| ntpSources := strings.Split(infraEnv.AdditionalNtpSources, ",") | ||
| for i, source := range ntpSources { |
There was a problem hiding this comment.
We handle this a bit differently in discovery.go. See:
| } | ||
|
|
||
| nmStateConfig := &v1beta1.NMStateConfig{ | ||
| TypeMeta: metav1.TypeMeta{ |
There was a problem hiding this comment.
iirc, it's redundant to specify TypeMeta
internal/ignition/disconnected.go
Outdated
| return "", errors.New("bootstrap host has no network interfaces") | ||
| } | ||
|
|
||
| // Find the first interface with an IPv4 address |
There was a problem hiding this comment.
Looks like something we can extract to network/utils.go for reusability.
Egg, maybe we can have generic utility func based on GetInventoryIPAddresses:
omer-vishlitzky
force-pushed
the
MGMT-20830-impl
branch
from
21c5a0a to
3833c11
Compare
|
/retest |
|
Looks good to me. |
omer-vishlitzky
force-pushed
the
MGMT-20830-impl
branch
from
3833c11 to
fbd423b
Compare
|
/retest |
omer-vishlitzky
force-pushed
the
MGMT-20830-impl
branch
from
fbd423b to
f145e52
Compare
internal/ignition/disconnected.go
Outdated
| APIVersion: "v1alpha1", | ||
| Kind: "AgentConfig", | ||
| Metadata: Metadata{ | ||
| Name: cluster.Name, |
There was a problem hiding this comment.
agent-config metadata does not need to be same as install-config, so there is no need to pass cluster object.
internal/ignition/disconnected.go
Outdated
| return errors.Wrap(err, "failed to create NMStateConfig manifests") | ||
| } | ||
|
|
||
| if err := createAgentConfigManifest(cluster, manifestsDir, log); err != nil { |
There was a problem hiding this comment.
cluster obj is not required
internal/bminventory/inventory.go
Outdated
| case "discovery.ign": | ||
| if common.ImageTypeValue(infraEnv.Type) == models.ImageTypeDisconnectedIso { | ||
| cluster, clusterErr := common.GetClusterFromDB(b.db, infraEnv.ClusterID, common.SkipEagerLoading) | ||
| cluster, clusterErr := common.GetClusterFromDB(b.db, infraEnv.ClusterID, common.UseEagerLoading) |
There was a problem hiding this comment.
With https://issues.redhat.com/browse/AGENT-1358, the OVE will use the late binding of infraenv so should this be kept unchanged to SkipEagerLoading?
There was a problem hiding this comment.
will it only use late binding? if yes then yes we should change it back to SkipEagerLoading
There was a problem hiding this comment.
Also this one, how does it come in play in the OVE scenario? Note that from the web console the user is not really "creating" a new cluster, but just downloading an ISO with a customized ignition
There was a problem hiding this comment.
So the UI creates only a "configuration cluster", just as a placeholder for the needed properties.
See more details in the enhancement:
There was a problem hiding this comment.
yes, @omer-vishlitzky it will only use late binding. The SaaS UI should just call agent create unconfigured-ignition command ( without the --interactive flag), create the manifests such as infraenv.yaml, cluster-image-set.yaml and pull-secret.yaml in cluster-manifests directory while the agent-config.yaml should be outside the cluster-manifests directory, more precisely at the current directory from where the command is run.
Also, for the ABI to know what is the user scenario ( OVE or not) and then to show the below the sea level UI by running agent-installer-ui container, the SaaS UI should also embed an empty sentinel file /etc/assisted/interactive-ui See https://issues.redhat.com/browse/MGMT-21982
There was a problem hiding this comment.
We'll also need the IRI resource (as an extra manifest) to activate the NoRegistryClusterOperations feature (and I think the same should be done at the appliance level for the cases where it's not used via the SaaS @danielerez, to use the new approach)
There was a problem hiding this comment.
We'll also need the IRI resource (as an extra manifest) to activate the
NoRegistryClusterOperationsfeature (and I think the same should be done at the appliance level for the cases where it's not used via the SaaS @danielerez, to use the new approach)
Right, but for later on of course, when IRI is supported:)
internal/bminventory/inventory.go
Outdated
| return common.GenerateErrorResponder(clusterErr) | ||
| } | ||
| content, err = b.disconnectedIgnitionGenerator.GenerateDisconnectedIgnition(ctx, infraEnv, cluster.OpenshiftVersion) | ||
| content, err = b.disconnectedIgnitionGenerator.GenerateDisconnectedIgnition(ctx, infraEnv, cluster) |
There was a problem hiding this comment.
cluster obj is not required, only cluster.OpenshiftVersion should be sufficient , reason mentioned below.
internal/ignition/disconnected.go
Outdated
| return errors.Wrap(err, "failed to create NMStateConfig manifests") | ||
| } | ||
|
|
||
| if err := createAgentConfigManifest(cluster, manifestsDir, log); err != nil { |
There was a problem hiding this comment.
info: agent-config.yaml does not need to be present in the cluster-manifests dir like infraenv.yaml
omer-vishlitzky
force-pushed
the
MGMT-20830-impl
branch
3 times, most recently
from
cdf97da to
77dcf9b
Compare
omer-vishlitzky
force-pushed
the
MGMT-20830-impl
branch
from
35aefcc to
f9e2051
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: 0
♻️ Duplicate comments (1)
internal/bminventory/inventory.go (1)
6414-6414: Optional: provide a fallback clusterName if empty when generating disconnected ignitionIf cluster.Name can be empty for the “configuration-only” cluster, pass a safe fallback (e.g., infraEnv.Name or "infraenv-") to the generator to keep file content stable.
- content, err = b.disconnectedIgnitionGenerator.GenerateDisconnectedIgnition(ctx, infraEnv, cluster.OpenshiftVersion, cluster.Name) + cname := cluster.Name + if cname == "" { + if infraEnv.Name != nil { cname = *infraEnv.Name } else { cname = fmt.Sprintf("infraenv-%s", infraEnv.ID.String()) } + } + content, err = b.disconnectedIgnitionGenerator.GenerateDisconnectedIgnition(ctx, infraEnv, cluster.OpenshiftVersion, cname)
🧹 Nitpick comments (2)
internal/bminventory/inventory.go (2)
5541-5557: Update path: trim + validate + explicit clear semanticsLogic is correct and user-friendly (empty string clears). One nit: mutating params.InfraEnvUpdateParams.RendezvousIP as a side effect can surprise callers; consider passing a local trimmed value to the validator and leaving params untouched.
5667-5687: Unify RendezvousIP validation across create/update (optional)Create and update paths duplicate validation/gating. Consider extracting a shared helper (e.g., validateRendezvousIP(ip, imageType) -> (string, error)) used by both to keep behavior and messages consistent.
📜 Review details
Configuration used: CodeRabbit 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 (5)
internal/bminventory/inventory.go(6 hunks)internal/bminventory/inventory_test.go(2 hunks)internal/ignition/disconnected.go(9 hunks)internal/ignition/disconnected_test.go(7 hunks)subsystem/disconnected_cluster_test.go(3 hunks)
🔥 Files not summarized due to errors (1)
- internal/bminventory/inventory_test.go: Error: Server error: no LLM provider could handle the message
🧰 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:
subsystem/disconnected_cluster_test.gointernal/bminventory/inventory.gointernal/ignition/disconnected_test.gointernal/ignition/disconnected.gointernal/bminventory/inventory_test.go
🔇 Additional comments (20)
internal/bminventory/inventory_test.go (2)
9284-9346: RegisterInfraEnv RendezvousIP validation tests are comprehensive.The three test cases properly verify that RendezvousIP is restricted to disconnected-iso infra-envs, validates IP format, and trims whitespace. Event mocking confirms proper error event generation on validation failures.
9617-9704: UpdateInfraEnv RendezvousIP tests comprehensively address the previous review gap.The new "RendezvousIP validation" context adds five well-structured test cases that mirror and extend the RegisterInfraEnv coverage:
- Error cases (non-disconnected-iso, invalid IP) align with RegisterInfraEnv tests
- Success case verifies both API response and persisted state via
GetInfraEnvInternal()- Edge cases tested: whitespace trimming, clearing with empty string, auto-clearing on type change to non-disconnected-iso
- Helper function
setInfraEnvType()provides clean, reusable test setup- Proper mock expectations and error message verification
This directly resolves the prior concern about missing UpdateInfraEnv test coverage.
subsystem/disconnected_cluster_test.go (4)
30-36: Helper function correctly normalizes version strings.The
majorMinorfunction properly extracts the major.minor version for comparison, handles edge cases by falling back to the original version, and is appropriately scoped to this test since it's only used once.
39-62: Comprehensive cluster creation and persistence verification.The test properly creates a disconnected cluster, verifies its initial properties, and confirms persistence by re-fetching. The assertions correctly validate Status, Kind, StatusInfo, and OpenshiftVersion fields.
64-98: Excellent test coverage of new InfraEnv features.The test comprehensively validates the new RendezvousIP, proxy configuration, and additional NTP sources functionality. The use of
majorMinoron line 90 for version comparison is particularly good practice, as it handles potential differences in API version formatting.
118-121: Good improvement to file cleanup logic.Explicitly closing the file before removal prevents potential file descriptor or file lock issues, especially on Windows. This is a solid defensive programming practice.
internal/bminventory/inventory.go (4)
5046-5046: Persisting a trimmed RendezvousIP is correctUsing normalizeRendezvousIP here prevents storing whitespace-surrounded values. LGTM.
5512-5521: Auto-clearing RendezvousIP when moving away from disconnected-isoGood defensive behavior: clearing a stored rendezvous_ip when image type is updated to non‑disconnected and no override is provided.
Please ensure we have tests for:
- ImageType change away from disconnected-iso clears rendezvous_ip when not provided.
- ImageType kept as disconnected-iso preserves rendezvous_ip unless explicitly cleared.
Also applies to: 5558-5560
5656-5665: normalizeRendezvousIP helper is cleanSimple, nil/empty safe normalization. LGTM.
5186-5195: I need to search more specifically for unit tests covering the validation logic ininventory.goat those lines.Now let me search for unit tests specific to the inventory function:
Based on my verification of the codebase:
Code validation is solid; unit tests for IPv6/whitespace are a best practice suggestion, not a requirement
The implementation correctly handles:
- Whitespace removal with strings.TrimSpace
- IP validation using net.ParseIP, which supports both IPv4 and IPv6 formats
- Disconnected-ISO image type gating
The model layer already enforces both IPv4 and IPv6 validation via regex patterns (
infra_env_create_params.go,infra_env.go), providing defense-in-depth. Existing integration tests cover the IPv4 path; adding unit tests for IPv6 and whitespace edge cases (e.g.," 2001:db8::1 "," ") would strengthen test coverage but is not required for correctness. The current validation logic is defensively sound.internal/ignition/disconnected_test.go (3)
40-107: LGTM: Test setup properly reflects cluster-bound infraEnv requirement.The updated test setup correctly initializes both infraEnv and cluster objects with appropriate cross-references, aligning with the disconnected ignition requirement that infraEnv must be bound to a cluster.
146-146: LGTM: Test calls correctly updated for new signature.All test invocations properly pass
cluster.OpenshiftVersionandcluster.Nameparameters, matching the updatedGenerateDisconnectedIgnitionsignature.
339-608: LGTM: Comprehensive test coverage for new features.The new test cases thoroughly validate:
- Proxy and NTP configuration propagation to InfraEnv manifest
- Whitespace trimming for NTP sources
- NMStateConfig manifest generation from StaticNetworkConfig
- Label selector integration between InfraEnv and NMStateConfig
- Conditional AgentConfig creation based on RendezvousIP presence
The tests verify manifest structure and content, providing strong validation of the implementation.
internal/ignition/disconnected.go (7)
5-40: LGTM: Imports and constants properly added.The new imports support JSON unmarshaling, string manipulation, and model handling required by the enhanced disconnected ignition generation. The
nmStateConfigInfraEnvLabelKeyconstant follows Kubernetes conventions for label selectors.
61-111: LGTM: Function signature and flow properly updated.The addition of
clusterNameparameter enables agent-config.yaml generation, and the orchestration of manifest creation, agent config, and ignition generation is well-structured with appropriate error handling.
113-136: LGTM: TypeMeta consistency addressed.The explicit TypeMeta initialization ensures the generated pull-secret.yaml includes proper
apiVersionandkindfields, maintaining consistency with other manifest creation functions.Based on learnings
138-195: LGTM: InfraEnv manifest generation properly enhanced.The implementation correctly:
- Builds the spec with conditional proxy configuration
- Trims and filters NTP sources to eliminate whitespace and empty entries
- Sets
NMStateConfigLabelSelectorto link with NMStateConfig manifests when static network config is provided- Includes TypeMeta for proper Kubernetes manifest format
The whitespace handling and label selector integration address past review feedback effectively.
245-280: LGTM: AgentConfig creation properly implemented.The function correctly:
- Returns early when RendezvousIP is not provided, avoiding unnecessary file creation
- Uses
swag.StringValuefor safe nil-pointer handling- Sets APIVersion to "v1beta1" (correct for agent-config.yaml consumed by installer)
- Uses clusterName in metadata as expected by the agent-based installer
Based on learnings
328-399: LGTM: NMStateConfig manifest generation properly implemented.The function correctly:
- Checks for whitespace-only static network config consistently with InfraEnv manifest handling
- Validates infraEnv.ID to prevent nil pointer issues
- Unmarshals StaticNetworkConfig JSON into typed models
- Creates label selector matching the key used in InfraEnv.NMStateConfigLabelSelector
- Includes defensive nil checks for config and macInterface entries
- Sets TypeMeta for proper Kubernetes CRD format
- Writes manifests with appropriate permissions and error handling
The implementation properly integrates NMStateConfig manifests with the InfraEnv through label selectors.
211-243: LGTM: Manifest creation flow properly orchestrated.The updated
createManifestsfunction correctly integrates NMStateConfig manifest creation into the flow, and the logger parameter enables appropriate logging. The octal notation for file permissions improves code clarity.
omer-vishlitzky
force-pushed
the
MGMT-20830-impl
branch
from
f9e2051 to
3d6963f
Compare
internal/bminventory/inventory.go
Outdated
| } | ||
|
|
||
| params.InfraEnvUpdateParams.RendezvousIP = swag.String(requested) | ||
| normalizedRendezvousIP, err := b.normalizeAndValidateRendezvousIPForUpdate(infraEnv, params) |
There was a problem hiding this comment.
Why not split the validation from the code we call in the update function?
If you look at UpdateInfraEnvInternal it has a bunch of validations before we call updateInfraEnvData.
validateAndUpdateInfraEnvParams seems like a good fit or a new validation function (we already have validateInfraEnvIgnitionParams, validateProxySettings, validateKernelArguments, and validateClusterArchitectureAndVersion)
Additionally it feels like we could deduplicate some of these checks between create and update. Nothing in normalizeAndValidateRendezvousIPForUpdate seems like it would be specific to update except for the parameter that is being passed (you could just as easily pass the type and ip from create).
internal/bminventory/inventory.go
Outdated
|
|
||
| func (b *bareMetalInventory) applyRendezvousIPUpdates(infraEnv *common.InfraEnv, params installer.UpdateInfraEnvParams, currentImageType models.ImageType, updates map[string]interface{}) error { | ||
| if params.InfraEnvUpdateParams.RendezvousIP != nil { | ||
| requested := strings.TrimSpace(swag.StringValue(params.InfraEnvUpdateParams.RendezvousIP)) |
There was a problem hiding this comment.
In the spirit of deduplication we also seem to call TrimSpace in three separate places even though there is a function called normalizeRendezvousIP. Shouldn't that be used here and in the create code?
internal/bminventory/inventory.go
Outdated
| currentImageType := common.ImageTypeValue(infraEnv.Type) | ||
| if params.InfraEnvUpdateParams.ImageType != "" && params.InfraEnvUpdateParams.ImageType != currentImageType { | ||
| updates["type"] = params.InfraEnvUpdateParams.ImageType | ||
| currentImageType = params.InfraEnvUpdateParams.ImageType |
There was a problem hiding this comment.
Why reassign currentImageType here? Seems confusing.
| } | ||
|
|
||
| if currentImageType != models.ImageTypeDisconnectedIso && swag.StringValue(infraEnv.RendezvousIP) != "" { | ||
| updates["rendezvous_ip"] = gorm.Expr("NULL") |
There was a problem hiding this comment.
Isn't this the same condition from https://github.com/openshift/assisted-service/pull/8144/files#diff-747a429a0e12307ef65d754c83b6898ce3858b621005e1e7af6e420da1f1657cR5516-R5520? Why do it in two places?
omer-vishlitzky
force-pushed
the
MGMT-20830-impl
branch
2 times, most recently
from
3c51257 to
1675e01
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
models/infra_env_create_params.go (1)
65-69: LGTM - Past review concerns have been addressed.The RendezvousIP validation implementation properly addresses the concerns raised in previous reviews:
- ✅ IP validation strengthened: Pattern now correctly validates IPv4 (with proper 0-255 octet ranges) and IPv6 addresses (including compressed, link-local, and IPv4-mapped formats)
- ✅ Documentation clarified: Changed from "required" to "optional for disconnected-iso image type", removing the ambiguity about conditional requirements
- ✅ Validation integrated: Pattern validation properly implemented in the generated validation method
The implementation is consistent with the other parameter models and follows go-swagger generation patterns correctly.
Also applies to: 114-116, 274-284
internal/bminventory/inventory.go (1)
6434-6434: Confirm cluster name is always non-empty for disconnected ignitionGenerator call threads cluster.Name. If there’s any path where Name can be empty, consider a fallback (e.g., infraEnv.Name or infraEnv.ID-derived).
Would you like a small patch to add a safe fallback?
🧹 Nitpick comments (8)
swagger.yaml (2)
7721-7728: Deduplicate the rendezvous_ip schema via a shared definitionThe long IPv6+zone regex is copy-pasted 3 times; this is brittle. Define a single reusable type (e.g., rendezvous-ip) and $ref it to keep the schema consistent.
Example:
+ rendezvous-ip: + type: string + x-nullable: true + description: IP address (IPv4/IPv6). IPv6 zone indices allowed, e.g., fe80::1%eth0. + pattern: '^(?:(?:(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])|(?:([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:(:[0-9a-fA-F]{1,4}){1,6}|:(:[0-9a-fA-F]{1,4}){1,7}|:|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9]))$' ... - rendezvous_ip: - type: string - description: ... - x-nullable: true - pattern: '...' + rendezvous_ip: + $ref: '#/definitions/rendezvous-ip'Also applies to: 7846-7852, 7905-7911
7724-7727: Clarify and enforce when rendezvous_ip is validSpec says “optional for disconnected-iso”. Ensure server-side validation rejects rendezvous_ip for non-disconnected image types and, optionally, verify it matches one of the discovered host IPs or static network config subnets to prevent misconfiguration.
Would you like me to add validation notes in the handler and a brief negative test?
Also applies to: 7848-7851, 7907-7910
subsystem/disconnected_cluster_test.go (1)
75-79: Align HTTPS proxy scheme for consistencyHere HTTPSProxy uses “http://…:8443” while unit tests use “https://…:8443”. Suggest switching to https for realism and consistency.
- HTTPSProxy: swag.String("http://proxy.example.com:8443"), + HTTPSProxy: swag.String("https://proxy.example.com:8443"),Also applies to: 95-97
internal/ignition/disconnected_test.go (3)
535-575: AgentConfig creation gated by rendezvous IPNice validation of presence/absence behavior and metadata name wiring. Consider adding an IPv6 (with zone) rendezvous test to match the swagger contract.
+ It("should create agent-config.yaml when IPv6 rendezvous IP (with zone) is provided", func() { + infraEnv.RendezvousIP = swag.String("fe80::1%eth0") + // ...reuse stubs... + mockExecuter.EXPECT().Execute(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ string, _ ...string) (string, string, int) { + oveDir := /* args[5] */ + ac, _ := os.ReadFile(filepath.Join(oveDir, "agent-config.yaml")) + var cfg AgentConfig + _ = yaml.Unmarshal(ac, &cfg) + Expect(cfg.RendezvousIP).To(Equal("fe80::1%eth0")) + _ = os.WriteFile(filepath.Join(oveDir, "unconfigured-agent.ign"), []byte("mock-ignition-content"), 0600) + return "success", "", 0 + }) + _, err := generator.GenerateDisconnectedIgnition(ctx, infraEnv, cluster.OpenshiftVersion, cluster.Name) + Expect(err).NotTo(HaveOccurred()) + })Also applies to: 577-608
115-125: Reduce stub duplication with a small helperMultiple tests repeat the same release image + cache expectations. Extract a local helper (e.g., stubRelease()) to DRY the setup and keep tests focused on assertions.
+ func stubRelease(tCtx context.Context) *models.ReleaseImage { + ri := &models.ReleaseImage{ + CPUArchitecture: swag.String(common.DefaultCPUArchitecture), + OpenshiftVersion: swag.String(clusterVersion), + URL: swag.String("quay.io/openshift-release-dev/ocp-release:" + clusterVersion + "-x86_64"), + Version: swag.String(clusterVersion), + } + mockVersionsHandler.EXPECT().GetReleaseImage(tCtx, clusterVersion, common.DefaultCPUArchitecture, infraEnv.PullSecret).Return(ri, nil) + mockEvents.EXPECT().V2AddMetricsEvent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() + mockInstallerCache.EXPECT().Get(tCtx, *ri.URL, "", infraEnv.PullSecret, gomock.Any(), clusterVersion, infraEnv.ClusterID).Return(mockRelease, nil) + return ri + }Also applies to: 146-149, 198-200, 270-337
202-208: Validate clusterName input as wellYou validate empty version; add a negative test for empty cluster name if generator relies on it for AgentConfig metadata (to avoid generating invalid kubernetes object names).
internal/bminventory/inventory.go (2)
5529-5536: Minor readability: rename variables to clarify directioncurrent/target can be ambiguous. Consider fromType/toType for intent clarity.
- currentImageType := common.ImageTypeValue(infraEnv.Type) - targetImageType := currentImageType + fromType := common.ImageTypeValue(infraEnv.Type) + toType := fromType - if requestedType := params.InfraEnvUpdateParams.ImageType; requestedType != "" { - targetImageType = requestedType - if requestedType != currentImageType { + if requestedType := params.InfraEnvUpdateParams.ImageType; requestedType != "" { + toType = requestedType + if requestedType != fromType { updates["type"] = requestedType } }
5665-5679: Optional: prefer netip.ParseAddr for stricter/modern IP parsingnet.ParseIP works, but net/netip offers better validation and performance. Not required.
- if net.ParseIP(swag.StringValue(ip)) == nil { + if _, err := netip.ParseAddr(swag.StringValue(ip)); err != nil { return errors.Errorf("Invalid rendezvous IP: %s (must be a valid IPv4 or IPv6 address)", swag.StringValue(ip)) }
📜 Review details
Configuration used: CodeRabbit 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 (9)
api/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (10)
internal/bminventory/inventory.go(6 hunks)internal/bminventory/inventory_test.go(2 hunks)internal/ignition/disconnected.go(9 hunks)internal/ignition/disconnected_test.go(7 hunks)models/infra_env.go(3 hunks)models/infra_env_create_params.go(3 hunks)models/infra_env_update_params.go(3 hunks)restapi/embedded_spec.go(6 hunks)subsystem/disconnected_cluster_test.go(3 hunks)swagger.yaml(3 hunks)
🧰 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/bminventory/inventory_test.gomodels/infra_env_create_params.gomodels/infra_env_update_params.gosubsystem/disconnected_cluster_test.gointernal/ignition/disconnected.gointernal/ignition/disconnected_test.gomodels/infra_env.gorestapi/embedded_spec.gointernal/bminventory/inventory.goswagger.yaml
⏰ 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-rhel9-acm-ds-main-on-pull-request
- GitHub Check: Red Hat Konflux / assisted-service-saas-main-on-pull-request
🔇 Additional comments (24)
internal/bminventory/inventory_test.go (2)
9284-9346: Thorough registration validation coverage.The failure cases and trimming persistence checks line up nicely with the new validation path, so regressions around RendezvousIP handling in RegisterInfraEnv should surface quickly.
9617-9701: Great UpdateInfraEnv safeguards.Exercising the reject, trim, and clear paths ensures the update workflow can’t drift from the intended RendezvousIP contract.
models/infra_env.go (1)
95-99: LGTM - Validation significantly improved from past review.The RendezvousIP field validation has been properly implemented with a comprehensive IPv4/IPv6 regex pattern that correctly validates:
- IPv4 addresses with proper octet ranges (0-255)
- Full and compressed IPv6 addresses
- Link-local IPv6 with zone IDs
- IPv4-mapped IPv6 addresses
One observation: the pattern explicitly allows empty strings (
^(?:$|...). For the main InfraEnv model, this means a non-nil pointer to an empty string will pass validation. This behavior might be intentional to support field clearing semantics, but it's worth noting that an empty string is semantically different from nil for an optional IP address field.Also applies to: 164-166, 364-374
models/infra_env_update_params.go (1)
48-52: LGTM - Proper implementation for update parameters.The RendezvousIP validation in update parameters correctly uses the same comprehensive IPv4/IPv6 pattern. For update operations, allowing empty strings via the pattern (
^(?:$|...) is appropriate, as it enables field clearing semantics (empty string = clear the field, nil = no change).Also applies to: 81-83, 160-170
restapi/embedded_spec.go (1)
9263-9268: LGTM—consistent schema additions across all InfraEnv surfaces.The
rendezvous_ipfield is uniformly defined across all relevant schemas (read, create, update) with proper IPv4/IPv6 pattern validation and nullable semantics. Since this is a generated file, any further adjustments should be made in the sourceswagger.yamland regenerated.Also applies to: 9365-9370, 9426-9431, 20779-20784, 20882-20887, 20943-20948
subsystem/disconnected_cluster_test.go (3)
30-36: LGTM: version normalization helper is pragmaticKeeps assertions stable across patch-level formatting.
65-81: Great coverage of rendezvous IP, proxy, and NTP on InfraEnv creationEnd-to-end assertions look good and exercise the new API surface.
Also applies to: 85-99
118-121: Nice cleanup safetyClosing before removal avoids Windows handle leaks and flakiness.
internal/ignition/disconnected_test.go (3)
146-149: Generator tests exercise happy path and major failures wellGood coverage across success and key failure modes; assertions are specific and actionable.
Also applies to: 198-200, 202-208, 210-224, 226-248, 250-268
339-388: Proxy and NTP manifest assertions are clear and usefulValidates Spec.Proxy and AdditionalNTPSources mapping with trimming.
Also applies to: 390-431
433-491: NMStateConfig generation and selector wiring verifiedLabeling and selector checks look right; good to test the whitespace skip case.
Also applies to: 493-533
internal/bminventory/inventory.go (5)
5068-5068: Good: persist a normalized RendezvousIPTrimming and normalizing before persistence prevents whitespace surprises and keeps DB clean.
5208-5213: Create-time gating and validation look correctNormalization + image-type gating + IP parse on create are solid and match update behavior.
5556-5559: Update path handles set/clear and gatingapplyRendezvousIPUpdates centralizes validation, supports clearing via empty input, and auto-clears on non-disconnected types.
5654-5664: Helper is cleannormalizeRendezvousIP correctly trims and converts empty to nil; avoids persisting blanks.
5681-5707: Robust update behavior for RendezvousIP
- Honors explicit clear (sets NULL).
- Validates when provided.
- Clears automatically when switching away from disconnected ISO.
Looks good.internal/ignition/disconnected.go (8)
5-6: LGTM - Imports and constant are appropriate.The added imports support the new functionality (JSON unmarshaling for static network config, string manipulation for parsing, swag helpers, and models). The
nmStateConfigInfraEnvLabelKeyconstant follows OpenShift labeling conventions and is used consistently throughout the file.Also applies to: 9-9, 12-12, 20-20, 40-40
61-61: LGTM - Function signature appropriately updated.Adding the
clusterNameparameter enables agent-config.yaml generation with proper cluster metadata, as needed for the rendezvous IP feature.
245-280: LGTM - Agent config generation is well-implemented.The
createAgentConfigfunction correctly handles the optional rendezvous IP:
- Uses
swag.StringValueto safely handle nil pointers- Skips agent-config.yaml generation when rendezvous IP is not provided
- Uses secure file permissions (0o600)
- The
apiVersion: "v1beta1"format is correct for agent-config.yaml consumed by the installer
139-195: LGTM - InfraEnv manifest creation is comprehensive and correct.The updated
createInfraEnvManifestfunction properly builds the spec with all required configurations:
- Proxy configuration (lines 147-154): Safely checks for presence and uses pointer values
- NTP sources (lines 156-163): Correctly parses CSV, trims whitespace, and filters empty entries
- Static network binding (lines 165-174): Properly sets the label selector to wire NMStateConfig manifests when static networking is configured
- TypeMeta (lines 177-180): Added for manifest consistency
All past review concerns (whitespace handling, nil safety, TypeMeta) have been addressed.
115-118: LGTM - TypeMeta added for consistency.The pull secret manifest now includes explicit
TypeMetawith correctapiVersion: v1andkind: Secret, ensuring consistency across all generated manifests as recommended in past reviews.
328-399: LGTM - NMStateConfig manifest generation is robust and well-implemented.The
createNMStateConfigManifestsfunction handles static network configuration comprehensively:
- Input validation: Properly trims whitespace and validates infraEnv.ID presence
- Defensive nil checks: Guards against nil configs (line 348) and nil macInterface entries (line 355)
- Interface validation: Skips configs with no valid interfaces and logs warnings (lines 363-366)
- Proper labeling: Labels each NMStateConfig with the infraEnv ID (line 375), matching the selector set in InfraEnv (line 171) to ensure the controller can bind them
- Manifest consistency: Includes TypeMeta for all CRs (lines 369-372)
- Secure permissions: Writes manifests with 0o600 permissions
All concerns from past reviews (whitespace handling, nil safety, TypeMeta, labeling) have been properly addressed.
211-230: LGTM - Manifest creation flow properly integrates NMStateConfig generation.The
createManifestsfunction now accepts a logger for better observability and correctly wires in NMStateConfig manifest creation with appropriate error wrapping.
132-132: LGTM - Octal notation improves clarity.The explicit octal prefix (
0o) for file and directory permissions is clearer and aligns with modern Go style conventions.Also applies to: 191-191, 199-199, 213-213, 234-234, 238-238
internal/bminventory/inventory.go
Outdated
| return nil | ||
| } | ||
|
|
||
| if err := validateRendezvousIP(targetImageType, normalizedRendezvousIP); err != nil { |
There was a problem hiding this comment.
I still think we should validate earlier in UpdateInfraEnvInternal rather than during the update function
…nv-update-params definitions in swagger.yaml
omer-vishlitzky
force-pushed
the
MGMT-20830-impl
branch
from
1675e01 to
f36e5f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
models/infra_env_create_params.go (1)
274-284: RendezvousIP validator permits empty string; fix in swagger and regenerate.Generated validator accepts "" due to the
^(?:$|...)pattern. Combined with*stringthis yields two “unset” encodings (nil and ""), complicating validation/persistence.
- Remove empty-string allowance from the regex in swagger.
- Prefer format-based validation or a stricter pattern there.
- For clear semantics on UpdateInfraEnv, use
x-omitempty: false(in swagger) so clients can sendnullto clear the field.I can provide the swagger diffs (see comments in swagger.yaml below). Regenerate models after updating the spec.
internal/bminventory/inventory.go (1)
6452-6455: Provide a safe fallback for clusterName (optional)If cluster.Name is ever empty (placeholder cluster, edge bootstraps), pass a deterministic fallback to the generator to keep manifest filenames stable.
- content, err = b.disconnectedIgnitionGenerator.GenerateDisconnectedIgnition(ctx, infraEnv, cluster.OpenshiftVersion, cluster.Name) + clusterName := cluster.Name + if clusterName == "" { + if infraEnv.Name != nil && *infraEnv.Name != "" { + clusterName = *infraEnv.Name + } else { + clusterName = fmt.Sprintf("infraenv-%s", infraEnv.ID.String()) + } + } + content, err = b.disconnectedIgnitionGenerator.GenerateDisconnectedIgnition(ctx, infraEnv, cluster.OpenshiftVersion, clusterName)
🧹 Nitpick comments (5)
internal/bminventory/inventory_test.go (1)
9664-9681: Excellent edge case coverage for RendezvousIP lifecycle.The tests cover two important scenarios:
- Explicit clearing via whitespace-only string (lines 9664-9681)
- Implicit clearing when image type changes away from disconnected-iso (lines 9683-9701)
These ensure data consistency and prevent orphaned RendezvousIP values on incompatible image types.
Consider adding a test case for overwriting an existing RendezvousIP with a different valid IP to ensure the update path works for non-empty → non-empty transitions:
It("should overwrite existing RendezvousIP with new valid IP", func() { setInfraEnvType(models.ImageTypeDisconnectedIso) mockInfraEnvUpdateSuccess() Expect(db.Model(&common.InfraEnv{}).Where("id = ?", i.ID).Update("rendezvous_ip", "192.168.1.100").Error).ToNot(HaveOccurred()) reply := bm.UpdateInfraEnv(ctx, installer.UpdateInfraEnvParams{ InfraEnvID: *i.ID, InfraEnvUpdateParams: &models.InfraEnvUpdateParams{ RendezvousIP: swag.String("10.0.0.50"), }, }) Expect(reply).To(BeAssignableToTypeOf(installer.NewUpdateInfraEnvCreated())) i, err = bm.GetInfraEnvInternal(ctx, installer.GetInfraEnvParams{InfraEnvID: *i.ID}) Expect(err).ToNot(HaveOccurred()) Expect(swag.StringValue(i.RendezvousIP)).To(Equal("10.0.0.50")) })Also applies to: 9683-9701
subsystem/disconnected_cluster_test.go (3)
89-91: Avoid internal/common dependency in tests; assert the enum directly.You can compare the enum without importing internal/common. This reduces coupling of subsystem tests to internal packages.
- Expect(common.ImageTypeValue(infraEnv.Type)).To(Equal(models.ImageTypeDisconnectedIso)) + Expect(infraEnv.Type).To(Equal(models.ImageTypeDisconnectedIso))
129-132: Prefer typed API error over brittle "404" substring match.Check the specific swagger-generated error (e.g., installer.NewV2DownloadInfraEnvFilesNotFound) or response code on the client, instead of parsing the error string.
// Example: var notFound *installer.V2DownloadInfraEnvFilesNotFound Expect(errors.As(err, ¬Found)).To(BeTrue())
64-81: Add an update flow test for RendezvousIP.PR feedback asked about allowing update via API. Add a test that patches infraenv.rendezvous_ip and verifies persistence and validation (including clearing).
It("updates infraenv RendezvousIP via UpdateInfraEnv", func() { // Arrange // ... create bound infraenv as above ... // Act: update to a new valid IP upd, err := utils_test.TestContext.UserBMClient.Installer.UpdateInfraEnv(ctx, &installer.UpdateInfraEnvParams{ InfraEnvID: *infraEnv.ID, InfraEnvUpdateParams: &models.InfraEnvUpdateParams{ RendezvousIP: swag.String("192.168.1.101"), }, }) Expect(err).NotTo(HaveOccurred()) Expect(swag.StringValue(upd.Payload.RendezvousIP)).To(Equal("192.168.1.101")) // Act: clear (null) if API supports it; otherwise ensure empty string rejects once swagger is fixed upd, err = utils_test.TestContext.UserBMClient.Installer.UpdateInfraEnv(ctx, &installer.UpdateInfraEnvParams{ InfraEnvID: *infraEnv.ID, InfraEnvUpdateParams: &models.InfraEnvUpdateParams{ RendezvousIP: nil, // expect clear semantics when x-omitempty:false is added }, }) Expect(err).NotTo(HaveOccurred()) Expect(upd.Payload.RendezvousIP).To(BeNil()) })internal/bminventory/inventory.go (1)
5649-5660: Helper funcs are cohesive and reusable; minor polish only (optional)
- normalizeRendezvousIP/validateRendezvousIP/applyRendezvousIPUpdates compose well and avoid duplication.
- Optional: make validateRendezvousIP accept a netip.Addr to avoid reparsing and ease unit testing, but current approach is fine.
Also applies to: 5675-5684, 5686-5700, 5702-5725
📜 Review details
Configuration used: CodeRabbit 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 (9)
api/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (10)
internal/bminventory/inventory.go(10 hunks)internal/bminventory/inventory_test.go(2 hunks)internal/ignition/disconnected.go(9 hunks)internal/ignition/disconnected_test.go(7 hunks)models/infra_env.go(3 hunks)models/infra_env_create_params.go(3 hunks)models/infra_env_update_params.go(3 hunks)restapi/embedded_spec.go(6 hunks)subsystem/disconnected_cluster_test.go(3 hunks)swagger.yaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- restapi/embedded_spec.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/bminventory/inventory_test.gomodels/infra_env.gosubsystem/disconnected_cluster_test.gomodels/infra_env_create_params.gomodels/infra_env_update_params.goswagger.yamlinternal/bminventory/inventory.gointernal/ignition/disconnected.gointernal/ignition/disconnected_test.go
🔇 Additional comments (7)
models/infra_env_update_params.go (1)
48-170: RendezvousIP validation aligns with the create params flowNice job threading the new field through the update schema and mirroring the strict IPv4/IPv6 pattern check. This keeps update payloads consistent with create-time validation and prevents malformed rendezvous targets from slipping through.
models/infra_env.go (1)
95-374: Consistent model enforcementAdding the RendezvousIP field with the same pattern guard and wiring it into
Validatekeeps the persisted model aligned with the API payload rules. This should help catch bad data early while staying optional for existing infra-envs.internal/bminventory/inventory_test.go (3)
9284-9346: Comprehensive test coverage for RegisterInfraEnv RendezvousIP validation.The three test cases properly cover the critical scenarios: constraint validation (disconnected-iso only), IP format validation, and normalization (trimming). The tests consistently verify both the HTTP response and the persisted database state.
9647-9662: Good coverage for successful RendezvousIP update with trimming.The test verifies both the successful update response and the trimmed value persisted in the database. This complements the RegisterInfraEnv trimming test and ensures consistency.
9617-9703: Verify event expectations for UpdateInfraEnv validation failures.The inconsistency is confirmed: UpdateInfraEnv failure tests (lines 9623–9645) lack event expectations, whereas RegisterInfraEnv failure tests explicitly expect
SendInfraEnvEventwithInfraEnvRegistrationFailedEventName(lines 9285, 9304). UpdateInfraEnv success tests only mockImageInfoUpdatedEventName, not failure events.Verify whether this observability gap is intentional (Update API intentionally doesn't emit failure events) or if UpdateInfraEnv should send
InfraEnvRegistrationFailedEventNameevents on validation errors to match RegisterInfraEnv behavior.internal/bminventory/inventory.go (2)
5208-5213: Create-path RendezvousIP: normalization + gating look correct
- Normalizes input, validates IPv4/IPv6, and persists the normalized value. This prevents whitespace artifacts and enforces disconnected-iso-only usage. LGTM.
Also applies to: 5068-5069
5416-5427: RendezvousIP validation is well-tested and verifiedTest coverage confirms all requested scenarios:
- IPv6 acceptance: Regex pattern in
models/infra_env_update_params.go(line 51) andmodels/infra_env_create_params.go(line 68) accepts both IPv4 and IPv6 addresses- Explicit clear via empty string:
inventory_test.goline 9664–9681 validates field is cleared whenRendezvousIP=""is provided- ImageType transitions:
inventory_test.goline 9683–9701 confirms field clears when image type changes away fromdisconnected-iso; related tests confirm validation gates the field based on target image typeCode implementation at lines 5416–5427, 5452–5454, 5509–5513, 5542–5544, 5564–5567 correctly uses target image type for validation and applies clear-on-empty and clear-on-type-change semantics.
| rendezvous_ip: | ||
| type: string | ||
| description: |- | ||
| The IP address of the host that will act as the rendezvous (bootstrap) node for agent-based installations. | ||
| This is optional for disconnected-iso image type and specifies which host will run the assisted service | ||
| during the bootstrap phase. All other hosts will connect to this IP to coordinate the installation. | ||
| x-nullable: true | ||
| pattern: '^(?:$|(?:(?:(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])|(?:([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:(:[0-9a-fA-F]{1,4}){1,6}|:(:[0-9a-fA-F]{1,4}){1,7}|:|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9]))))$' | ||
| type: |
There was a problem hiding this comment.
Disallow empty-string rendezvous_ip; enable proper null clearing on update.
Make the field unambiguous (null = unset). Remove ^(?:$| from the regex, and allow null in update requests by disabling omitempty.
Apply these diffs:
# 1) infra-env.rendezvous_ip (public object)
- pattern: '^(?:$|(?:(?:(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])|(?:([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:(:[0-9a-fA-F]{1,4}){1,6}|:(:[0-9a-fA-F]{1,4}){1,7}|:|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9]))))$'
+ pattern: '^(?:(?:(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])|(?:([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:(:[0-9a-fA-F]{1,4}){1,6}|:(:[0-9a-fA-F]{1,4}){1,7}|:|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9]))$'
# 2) infra-env-create-params.rendezvous_ip
- pattern: '^(?:$|(?:(?:(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])|(?:([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:(:[0-9a-fA-F]{1,4}){1,6}|:(:[0-9a-fA-F]{1,4}){1,7}|:|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9]))))$'
+ pattern: '^(?:(?:(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])|(?:([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:(:[0-9a-fA-F]{1,4}){1,6}|:(:[0-9a-fA-F]{1,4}){1,7}|:|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9]))$'
# 3) infra-env-update-params.rendezvous_ip
- pattern: '^(?:$|(?:(?:(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])|(?:([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:(:[0-9a-fA-F]{1,4}){1,6}|:(:[0-9a-fA-F]{1,4}){1,7}|:|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9]))))$'
+ pattern: '^(?:(?:(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])|(?:([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:(:[0-9a-fA-F]{1,4}){1,6}|:(:[0-9a-fA-F]{1,4}){1,7}|:|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1?[0-9])?[0-9])\.){3}(25[0-5]|(2[0-4]|1?[0-9])?[0-9]))$'
+ x-omitempty: false # allow explicit null to clear on PATCHAlso applies to: 7846-7853, 7905-7912
🤖 Prompt for AI Agents
In swagger.yaml around lines 7721-7729 (also apply the same changes at 7846-7853
and 7905-7912): the rendezvous_ip pattern currently allows an empty string via
the leading ^(?:$| alternative and the field is treated as omitempty on updates
so clients cannot clear it to null; remove the empty-string branch by deleting
the leading ^(?:$| and its matching closing group so the pattern only matches
valid IPv4/IPv6 addresses, keep x-nullable: true, and in the update request
schemas remove the omitempty behavior for this property so null can be sent to
clear the value (ensure the generated models/field tags or schema options do not
include omitempty for rendezvous_ip).
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
I'm thinking we don't need normalizeRendezvousIP at all and that it is actively making things worse by confusing the difference between provided-as-empty (i.e. unset the ip) and nil (no change)
This then allows for some other simplifications.
internal/bminventory/inventory.go
Outdated
| } | ||
|
|
||
| func (b *bareMetalInventory) updateInfraEnvData(infraEnv *common.InfraEnv, params installer.UpdateInfraEnvParams, internalIgnitionConfig *string, db *gorm.DB, log logrus.FieldLogger) error { | ||
| func (b *bareMetalInventory) updateInfraEnvData(infraEnv *common.InfraEnv, params installer.UpdateInfraEnvParams, internalIgnitionConfig *string, db *gorm.DB, log logrus.FieldLogger, currentImageType, targetImageType models.ImageType, requestedRendezvousIP bool, normalizedRendezvousIP *string) error { |
There was a problem hiding this comment.
Isn't requestedRendezvousIP just normalizedRendezvousIP != nil? Why pass both here?
internal/bminventory/inventory.go
Outdated
| } | ||
| trimmed := strings.TrimSpace(swag.StringValue(ipPtr)) | ||
| if trimmed == "" { | ||
| return nil |
There was a problem hiding this comment.
Wouldn't it be more accurate here (if it's possible to send just whitespace in) for this to return a pointer to an empty string? I'd expect someone sending in an empty value to this API to clear the IP, right?
internal/bminventory/inventory.go
Outdated
| func (b *bareMetalInventory) applyRendezvousIPUpdates(infraEnv *common.InfraEnv, requestedRendezvousIP bool, normalizedRendezvousIP *string, targetImageType models.ImageType, updates map[string]interface{}) error { | ||
| // 1. Field requested: user explicitly changed rendezvous IP -> set or clear accordingly. | ||
| // 2. Field not requested: keep existing value unless image type changed to a variant that forbids rendezvous IP. | ||
| if requestedRendezvousIP { | ||
| if normalizedRendezvousIP == nil { | ||
| if swag.StringValue(infraEnv.RendezvousIP) != "" { | ||
| updates["rendezvous_ip"] = gorm.Expr("NULL") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| if swag.StringValue(normalizedRendezvousIP) != swag.StringValue(infraEnv.RendezvousIP) { | ||
| updates["rendezvous_ip"] = normalizedRendezvousIP | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // Image type changed away from disconnected ISO -> clear rendezvous IP | ||
| if targetImageType != models.ImageTypeDisconnectedIso && swag.StringValue(infraEnv.RendezvousIP) != "" { | ||
| updates["rendezvous_ip"] = gorm.Expr("NULL") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
I think this can be a lot simpler. It never returns an error, and with the pointer/empty change I suggested I think this just becomes:
| func (b *bareMetalInventory) applyRendezvousIPUpdates(infraEnv *common.InfraEnv, requestedRendezvousIP bool, normalizedRendezvousIP *string, targetImageType models.ImageType, updates map[string]interface{}) error { | |
| // 1. Field requested: user explicitly changed rendezvous IP -> set or clear accordingly. | |
| // 2. Field not requested: keep existing value unless image type changed to a variant that forbids rendezvous IP. | |
| if requestedRendezvousIP { | |
| if normalizedRendezvousIP == nil { | |
| if swag.StringValue(infraEnv.RendezvousIP) != "" { | |
| updates["rendezvous_ip"] = gorm.Expr("NULL") | |
| } | |
| return nil | |
| } | |
| if swag.StringValue(normalizedRendezvousIP) != swag.StringValue(infraEnv.RendezvousIP) { | |
| updates["rendezvous_ip"] = normalizedRendezvousIP | |
| } | |
| return nil | |
| } | |
| // Image type changed away from disconnected ISO -> clear rendezvous IP | |
| if targetImageType != models.ImageTypeDisconnectedIso && swag.StringValue(infraEnv.RendezvousIP) != "" { | |
| updates["rendezvous_ip"] = gorm.Expr("NULL") | |
| } | |
| return nil | |
| } | |
| func (b *bareMetalInventory) applyRendezvousIPUpdates(infraEnv *common.InfraEnv, normalizedRendezvousIP *string, targetImageType models.ImageType, updates map[string]interface{}) { | |
| if swag.StringValue(infraEnv.RendezvousIP) != "" && (normalizedRendezvousIP == "" || targetImageType != models.ImageTypeDisconnectedIso) { | |
| updates["rendezvous_ip"] = gorm.Expr("NULL") | |
| return | |
| } | |
| if swag.StringValue(normalizedRendezvousIP) != swag.StringValue(infraEnv.RendezvousIP) { | |
| updates["rendezvous_ip"] = normalizedRendezvousIP | |
| } | |
| } |
internal/bminventory/inventory.go
Outdated
| if ipPtr == nil { | ||
| return nil | ||
| } | ||
| trimmed := strings.TrimSpace(swag.StringValue(ipPtr)) |
There was a problem hiding this comment.
Wouldn't the swagger validation prevent the value coming in with whitespace? Isn't it checking that the thing is already formatted like an ip?
omer-vishlitzky
force-pushed
the
MGMT-20830-impl
branch
from
f36e5f2 to
ef2ff9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/ignition/disconnected.go (1)
147-154: Preserve NoProxy-only proxy settings.Right now
spec.Proxyis only populated when either HTTP or HTTPS proxy is supplied. If the user sets onlyno_proxy, we silently drop it from the generated InfraEnv manifest, so installations that rely on exclusions without an outbound proxy lose their configuration. Please populate the proxy block whenever any of the three fields is non-empty.- if infraEnv.Proxy != nil && (swag.StringValue(infraEnv.Proxy.HTTPProxy) != "" || swag.StringValue(infraEnv.Proxy.HTTPSProxy) != "") { - spec.Proxy = &v1beta1.Proxy{ - HTTPProxy: swag.StringValue(infraEnv.Proxy.HTTPProxy), - HTTPSProxy: swag.StringValue(infraEnv.Proxy.HTTPSProxy), - NoProxy: swag.StringValue(infraEnv.Proxy.NoProxy), - } - } + if infraEnv.Proxy != nil { + httpProxy := swag.StringValue(infraEnv.Proxy.HTTPProxy) + httpsProxy := swag.StringValue(infraEnv.Proxy.HTTPSProxy) + noProxy := swag.StringValue(infraEnv.Proxy.NoProxy) + if httpProxy != "" || httpsProxy != "" || noProxy != "" { + spec.Proxy = &v1beta1.Proxy{ + HTTPProxy: httpProxy, + HTTPSProxy: httpsProxy, + NoProxy: noProxy, + } + } + }
📜 Review details
Configuration used: CodeRabbit 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 (5)
internal/bminventory/inventory.go(10 hunks)internal/bminventory/inventory_test.go(2 hunks)internal/ignition/disconnected.go(9 hunks)internal/ignition/disconnected_test.go(7 hunks)subsystem/disconnected_cluster_test.go(3 hunks)
🧰 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/bminventory/inventory_test.gosubsystem/disconnected_cluster_test.gointernal/bminventory/inventory.gointernal/ignition/disconnected_test.gointernal/ignition/disconnected.go
🔇 Additional comments (6)
internal/bminventory/inventory.go (6)
5208-5214: Create-time RendezvousIP validation looks goodNil/empty normalization plus image-type gating and centralized validation is sound.
5417-5425: Pre-validate update target image type and RendezvousIPDeriving current/target image types and validating RendezvousIP up-front is the right place; reduces surprises in the write path.
5068-5069: Persisting RendezvousIP on createWiring the field into the InfraEnv model at registration aligns with the new validation; no issues spotted.
5539-5541: Update image type when changedSetting updates["type"] only on change is correct and avoids unnecessary writes.
5665-5680: Shared RendezvousIP validatorSingle helper that gates to disconnected-iso and accepts IPv4/IPv6 via net.ParseIP is appropriate here.
6417-6418: Pass cluster name to disconnected ignition generatorAdditional clusterName argument is reasonable; call site change looks correct.
…oxy server, rendezvous ip and nmstate config to the ignition
omer-vishlitzky
force-pushed
the
MGMT-20830-impl
branch
from
ef2ff9b to
a7acb01
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
internal/bminventory/inventory.go (4)
5068-5068: Normalize RendezvousIP before persisting (trim + empty-as-nil).Avoid storing whitespace-surrounded values; treat whitespace-only as unset.
- RendezvousIP: params.InfraenvCreateParams.RendezvousIP, + // Normalize rendezvous_ip + RendezvousIP: func() *string { + if params.InfraenvCreateParams.RendezvousIP == nil { + return nil + } + ip := strings.TrimSpace(swag.StringValue(params.InfraenvCreateParams.RendezvousIP)) + if ip == "" { + return nil + } + return swag.String(ip) + }(),
5208-5213: Creation-time: treat whitespace as empty and validate the trimmed value.Pre-trim ensures “ ” clears the field and avoids false negatives during IP parse.
- if params.InfraenvCreateParams.RendezvousIP != nil && swag.StringValue(params.InfraenvCreateParams.RendezvousIP) == "" { - params.InfraenvCreateParams.RendezvousIP = nil - } - if err = validateRendezvousIP(params.InfraenvCreateParams.ImageType, params.InfraenvCreateParams.RendezvousIP); err != nil { + if params.InfraenvCreateParams.RendezvousIP != nil { + trimmed := strings.TrimSpace(swag.StringValue(params.InfraenvCreateParams.RendezvousIP)) + if trimmed == "" { + params.InfraenvCreateParams.RendezvousIP = nil + } else { + params.InfraenvCreateParams.RendezvousIP = swag.String(trimmed) + } + } + if err = validateRendezvousIP(params.InfraenvCreateParams.ImageType, params.InfraenvCreateParams.RendezvousIP); err != nil { return err }
5644-5651: Normalize in validateRendezvousIPUpdate to support whitespace-clears.Trim first; if empty after trim, treat as “no value”; otherwise validate the trimmed IP.
func (b *bareMetalInventory) validateRendezvousIPUpdate(ip *string, targetImageType models.ImageType) error { - if swag.StringValue(ip) == "" { + if ip == nil { return nil } - - return validateRendezvousIP(targetImageType, ip) + trimmed := strings.TrimSpace(*ip) + if trimmed == "" { + return nil + } + return validateRendezvousIP(targetImageType, &trimmed) }
5665-5679: Harden validateRendezvousIP with TrimSpace.Avoid rejecting valid inputs with incidental whitespace and align create/update behavior.
func validateRendezvousIP(imageType models.ImageType, ip *string) error { if ip == nil { return nil } if imageType != models.ImageTypeDisconnectedIso { return errors.Errorf("RendezvousIP is only supported for disconnected ISO infra-envs") } - if net.ParseIP(swag.StringValue(ip)) == nil { - return errors.Errorf("Invalid rendezvous IP: %s (must be a valid IPv4 or IPv6 address)", swag.StringValue(ip)) + v := strings.TrimSpace(swag.StringValue(ip)) + if net.ParseIP(v) == nil { + return errors.Errorf("Invalid rendezvous IP: %s (must be a valid IPv4 or IPv6 address)", swag.StringValue(ip)) } return nil }
🧹 Nitpick comments (3)
internal/bminventory/inventory_test.go (1)
9284-9320: Consider adding a positive test case for RegisterInfraEnv for symmetry.While the negative validation tests are solid, adding a positive test case that verifies successful registration with a valid RendezvousIP on a disconnected-iso infraenv would provide symmetric coverage with the UpdateInfraEnv tests. The UpdateInfraEnv suite includes a positive test at line 9622-9637, and having a matching one for RegisterInfraEnv would ensure both APIs are validated consistently.
Example test case:
It("should succeed when setting valid RendezvousIP on disconnected-iso infraenv", func() { mockEvents.EXPECT().SendInfraEnvEvent(ctx, eventstest.NewEventMatcher( eventstest.WithNameMatcher(eventgen.InfraEnvRegisteredEventName))).Times(1) reply := bm.RegisterInfraEnv(ctx, installer.RegisterInfraEnvParams{ InfraenvCreateParams: &models.InfraEnvCreateParams{ Name: swag.String("infra-env-with-valid-rendezvous"), OpenshiftVersion: common.TestDefaultConfig.OpenShiftVersion, PullSecret: swag.String(fakePullSecret), ImageType: models.ImageTypeDisconnectedIso, ClusterID: &clusterID, RendezvousIP: swag.String("192.168.1.100"), }, }) Expect(reply).Should(BeAssignableToTypeOf(&installer.RegisterInfraEnvCreated{})) actual := reply.(*installer.RegisterInfraEnvCreated) Expect(swag.StringValue(actual.Payload.RendezvousIP)).To(Equal("192.168.1.100")) })internal/bminventory/inventory.go (2)
5417-5424: Update path looks good; prefer to normalize once in the validator.Gating to disconnected-iso and passing the requested value down is correct. To handle whitespace-only clears consistently, add TrimSpace inside validateRendezvousIPUpdate (see below) so call sites stay simple.
Also applies to: 5450-5450
6432-6432: Provide a safe fallback for clusterName in disconnected ignition.In rare cases cluster.Name may be empty (e.g., placeholder/misconfigured “configuration cluster”). Use infraEnv.Name or ID as fallback to keep generation stable.
- content, err = b.disconnectedIgnitionGenerator.GenerateDisconnectedIgnition(ctx, infraEnv, cluster.OpenshiftVersion, cluster.Name) + clusterName := cluster.Name + if clusterName == "" { + if infraEnv.Name != nil && *infraEnv.Name != "" { + clusterName = *infraEnv.Name + } else { + clusterName = fmt.Sprintf("infraenv-%s", infraEnv.ID.String()) + } + } + content, err = b.disconnectedIgnitionGenerator.GenerateDisconnectedIgnition(ctx, infraEnv, cluster.OpenshiftVersion, clusterName)
📜 Review details
Configuration used: CodeRabbit 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 (5)
internal/bminventory/inventory.go(10 hunks)internal/bminventory/inventory_test.go(2 hunks)internal/ignition/disconnected.go(9 hunks)internal/ignition/disconnected_test.go(7 hunks)subsystem/disconnected_cluster_test.go(3 hunks)
🧰 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/bminventory/inventory_test.gointernal/ignition/disconnected.gosubsystem/disconnected_cluster_test.gointernal/bminventory/inventory.gointernal/ignition/disconnected_test.go
🔇 Additional comments (25)
internal/bminventory/inventory_test.go (2)
9284-9320: Good negative validation coverage for RegisterInfraEnv.The tests properly verify that RendezvousIP is restricted to disconnected-iso infraenvs and must be a valid IP address. Event expectations and error assertions are correct.
9592-9697: Excellent comprehensive test coverage for UpdateInfraEnv RendezvousIP validation.This test suite directly addresses the previous review concern about missing UpdateInfraEnv API tests. The coverage is thorough and includes:
- Negative validation (non-disconnected-iso, invalid IP format)
- Positive scenario (valid IP on disconnected-iso)
- Edge cases (clearing via empty string, auto-clearing on type change, preservation on unrelated updates)
The helper function
setInfraEnvTypekeeps the tests clean, and all assertions properly verify both API responses and persisted state.subsystem/disconnected_cluster_test.go (4)
30-36: LGTM!The
majorMinorhelper function correctly extracts major.minor version components with appropriate defensive checks.
39-63: LGTM!The cluster creation flow properly verifies both the immediate response and persistence by re-fetching the cluster.
64-99: LGTM!Comprehensive test coverage for the new InfraEnv fields including RendezvousIP, Proxy settings, and AdditionalNtpSources.
118-121: LGTM!Proper resource cleanup pattern with deferred close and removal of temporary file.
internal/ignition/disconnected_test.go (8)
40-40: LGTM!The cluster object setup provides a complete test fixture with appropriate fields including a bootstrap host with network inventory.
Also applies to: 77-104
146-146: LGTM!All test calls to
GenerateDisconnectedIgnitionhave been consistently updated to use the new signature withcluster.OpenshiftVersionandcluster.Nameparameters.Also applies to: 198-198, 205-205, 221-221, 229-229, 245-245, 265-265, 334-334, 386-386, 429-429, 489-489, 531-531, 573-573, 606-606
339-388: LGTM!Thorough test coverage for proxy and NTP configuration propagation to the InfraEnv manifest.
390-431: LGTM!Good test coverage for whitespace handling in NTP sources, ensuring proper trimming and empty entry removal.
433-491: LGTM!Excellent test coverage for NMStateConfig manifest generation, including proper validation of the label-based selector linking InfraEnv to NMStateConfig.
493-533: LGTM!Important edge case test ensuring whitespace-only static network config is properly skipped.
535-575: LGTM!Proper test coverage for AgentConfig creation when RendezvousIP is provided, validating all fields including the use of cluster name in metadata.
577-608: LGTM!Important negative test ensuring agent-config.yaml is not created when RendezvousIP is not provided.
internal/ignition/disconnected.go (9)
5-5: LGTM!New imports and constant are appropriate for the enhanced functionality. The label key follows OpenShift naming conventions.
Also applies to: 6-6, 9-9, 12-12, 20-20, 40-40
61-61: LGTM!The additional
clusterNameparameter is necessary for AgentConfig generation. All call sites have been updated appropriately.
88-90: LGTM!The
createAgentConfigcall is properly integrated with appropriate error handling.
115-118: LGTM!TypeMeta is correctly set for the Secret manifest with the appropriate APIVersion and Kind.
148-154: Verify NoProxy-only configuration handling.The current condition only adds the Proxy spec when
HTTPProxyorHTTPSProxyis non-empty. If an InfraEnv has onlyNoProxyconfigured (without HTTP/HTTPS proxies), the proxy configuration will be omitted from the manifest. While this might be an edge case, consider whether NoProxy-only configurations should also be preserved.If NoProxy-only configurations are valid use cases, apply this diff:
- if infraEnv.Proxy != nil && (swag.StringValue(infraEnv.Proxy.HTTPProxy) != "" || swag.StringValue(infraEnv.Proxy.HTTPSProxy) != "") { + httpProxy := swag.StringValue(infraEnv.Proxy.HTTPProxy) + httpsProxy := swag.StringValue(infraEnv.Proxy.HTTPSProxy) + noProxy := swag.StringValue(infraEnv.Proxy.NoProxy) + if infraEnv.Proxy != nil && (httpProxy != "" || httpsProxy != "" || noProxy != "") { spec.Proxy = &v1beta1.Proxy{ - HTTPProxy: swag.StringValue(infraEnv.Proxy.HTTPProxy), - HTTPSProxy: swag.StringValue(infraEnv.Proxy.HTTPSProxy), - NoProxy: swag.StringValue(infraEnv.Proxy.NoProxy), + HTTPProxy: httpProxy, + HTTPSProxy: httpsProxy, + NoProxy: noProxy, } }
156-174: LGTM!The NTP source handling properly trims whitespace and filters empty entries. StaticNetworkConfig is correctly checked with
TrimSpace, and the NMStateConfig label selector is appropriately set. TypeMeta is properly included, and file permissions use the correct octal notation.Also applies to: 177-194
211-229: LGTM!The updated
createManifestssignature properly includes the logger parameter for NMStateConfig manifest creation.
245-280: LGTM!The AgentConfig types and creation function are well-structured. The early return when RendezvousIP is empty provides clean conditional generation, and the APIVersion "v1beta1" is correct for agent-config.yaml files consumed by the installer tool.
328-399: LGTM!The
createNMStateConfigManifestsfunction is well-implemented with proper validation, nil checks, whitespace handling, and label-based linking to InfraEnv. All concerns from past reviews have been addressed.internal/bminventory/inventory.go (2)
5506-5514: RendezvousIP update semantics: LGTM.
- Clears on image type change away from disconnected-iso
- Preserves when omitted
- Allows explicit clear with empty string
Also applies to: 5539-5541, 5561-5562
5681-5705: applyRendezvousIPUpdates logic is correct.Only updates when present, clears on type change or explicit empty; preserves when omitted. Nice.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, omer-vishlitzky The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
@omer-vishlitzky: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
Enhance disconnected ignition generation to include proxy configuration, additional NTP sources, static network config, and rendezvous IP fields.