SPLAT-2172: AWS dedicate host support#10079
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-2172 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.21.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. |
|
/cc |
vr4manta
force-pushed
the
SPLAT-2172
branch
3 times, most recently
from
24500b6 to
342ab2d
Compare
|
/retest-required |
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
Cool feature 👍 🚀
I have quite some comments 😅 Sorry if I review a little too soon...
| switch *p.HostPlacement.Affinity { | ||
| case aws.HostAffinityAnyAvailable: | ||
| if p.HostPlacement.DedicatedHost != nil { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("dedicatedHost"), "dedicatedHost is required when 'affinity' is set to DedicatedHost, and forbidden otherwise")) |
There was a problem hiding this comment.
Did you mean hostPlacement.dedicatedHost is forbidden if affinity is AnyAvailable?
allErrs = append(allErrs, field.Forbidden(fldPath.Child("dedicatedHost"), "dedicatedHost must not be set when affinity is AnyAvailable"))There was a problem hiding this comment.
@tthvo So this was an interesting one. In openshift/api and openshift/machine-api-operator, it was suggested to error this way for this scenario. I was doing this to keep it consistent.
- https://github.com/openshift/api/blob/0598ae66682433c49adc29b1ff14a201487f3afe/machine/v1beta1/types_awsprovider.go#L406
- openshift/machine-api-operator@f669914#diff-30c686f7a9dbd2aa917e5e410401d720a2a4bf4a11163495f5610c8f4b98edaeR917
Now that said, I am happy to make your suggestion if you prefer installer say it this way. Just let me know.
| if err != nil { | ||
| allErrs = append(allErrs, field.InternalError(placementPath.Child("dedicatedHost"), err)) | ||
| } else { | ||
| // Check the returned configured hosts to see if the dedicated hosts defined in install-config exists. |
There was a problem hiding this comment.
💡 ❓ I got 2 questions here:
-
Do the zones of dedicated hosts need to match machinepool zone field if defined?
installer/pkg/types/aws/machinepool.go
Lines 5 to 9 in 7c9b71a
-
Do the user-input zones for dedicated hosts need to match with the actual zones returned from AWS?
// If user specified a zone, validate it matches AWS if host.Zone != "" && host.Zone != hostDetails.Zone { allErrs = append(allErrs, field.Invalid( fldPath.Child("hostPlacement", "dedicatedHost").Index(i).Child("zone"), host.Zone, fmt.Sprintf("specified zone %s does not match actual host zone %s", host.Zone, hostDetails.Zone))) }
There was a problem hiding this comment.
Do the user-input zones for dedicated hosts need to match with the actual zones returned from AWS?
On that point, I'm curious if users should need to specify the zone for the dedicated host? It seems like we can just look up the dedicated host by id and then use that to determine the zone (which the pr is already doing IIUC). Seems like we might be able to shed zone from the install config.
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
vr4manta
force-pushed
the
SPLAT-2172
branch
5 times, most recently
from
82915c2 to
8ef8679
Compare
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
/payload-job periodic-ci-openshift-release-master-nightly-4.21-e2e-aws-ovn-dedicated |
|
@vr4manta: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/1c5c6d80-c717-11f0-93e9-d918c346504c-0 |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
vr4manta
force-pushed
the
SPLAT-2172
branch
2 times, most recently
from
cc8a0b9 to
4b1d68a
Compare
mtulio
left a comment
mtulio
left a comment
There was a problem hiding this comment.
Hey @vr4manta , good progress!
A few questions related to tagging and covered use cases. It would be nice to know what would be the tagging behavior of a Day-2 instance created with BYO DH ID: do we need to set kubernetes cluster tags (kubernetes/cluster...) to it (mapa/capa component) or would be optional?
vr4manta
force-pushed
the
SPLAT-2172
branch
3 times, most recently
from
35cdfe9 to
d687fa9
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds AWS Dedicated Hosts support across schema, types, validation, install-config helpers, machine-set wiring, permission groups, tagging during PreTerraform, and dedicated-host release on destroy. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Installer
participant IC as InstallConfig Validator
participant Meta as AWS Metadata Cache
participant EC2 as EC2 API
participant MS as MachineSet Generator
CLI->>IC: validate install-config (hostPlacement)
IC->>Meta: DedicatedHosts(ctx, hostIDs)
Meta->>EC2: DescribeHosts (paginated)
EC2-->>Meta: Hosts list
Meta->>Meta: cache Hosts map
IC-->>CLI: validation result (existence/zone/duplicates)
CLI->>MS: generate machine sets
MS->>Meta: read Hosts map
MS->>MS: DedicatedHost(hosts, placement, zone)
MS-->>EC2: provision instances with HostTenancy + DedicatedHost (provider)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@vr4manta: This pull request references SPLAT-2172 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/asset/machines/aws/machinesets.go (1)
91-109:⚠️ Potential issue | 🟠 MajorDon’t silently drop host placement for unmatched zones.
If
DedicatedHost()cannot find a host for the current AZ,dedicatedHostbecomes empty and this code still emits a machineset. ForAffinity=DedicatedHost, that creates a regular machineset for that zone instead of honoring the install-config.Suggested fix
dedicatedHost := DedicatedHost(in.Hosts, mpool.HostPlacement, az) + if mpool.HostPlacement != nil && + mpool.HostPlacement.Affinity != nil && + *mpool.HostPlacement.Affinity == aws.HostAffinityDedicatedHost && + dedicatedHost == "" { + return nil, errors.Errorf("no dedicated host configured for machine pool %q in zone %s", in.Pool.Name, az) + } provider, err := provider(&machineProviderInput{ clusterID: in.ClusterID,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/aws/machinesets.go` around lines 91 - 109, The code currently continues to build and emit a machineset even when DedicatedHost(in.Hosts, mpool.HostPlacement, az) returns an empty dedicatedHost, which silently drops host placement and creates a regular machineset; update the logic around calling provider(&machineProviderInput{...}) to check the pool's affinity (mpool.HostPlacement or the field that indicates Affinity=DedicatedHost) and if affinity is DedicatedHost and dedicatedHost == "" then skip creating/emitting the machineset (return or continue) so we don't produce a non-dedicated machineset for that AZ; reference DedicatedHost(), the dedicatedHost variable, machineProviderInput and mpool.HostPlacement when making the change.
♻️ Duplicate comments (2)
pkg/destroy/aws/ec2helpers.go (1)
973-985:⚠️ Potential issue | 🟠 MajorReturn early on unauthorized
DescribeHosts.The
UnauthorizedOperation/AccessDeniedbranch says “skipping” and then still returns an error, so destroy will fail anyway whenec2:DescribeHostsis missing.Suggested fix
if err != nil { errCode := HandleErrorCode(err) switch errCode { case "InvalidHostID.NotFound": // Host doesn't exist, nothing to do return nil case "UnauthorizedOperation", "AccessDenied": // User doesn't have permission to describe dedicated hosts // This is expected when dedicated host permissions are not configured logger.Warn("User does not have permission to describe dedicated hosts, skipping") + return nil } return fmt.Errorf("failed to describe dedicated host %s: %w", id, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/destroy/aws/ec2helpers.go` around lines 973 - 985, The DescribeHosts error handling in the block using HandleErrorCode(err) incorrectly logs a warning for "UnauthorizedOperation" and "AccessDenied" but still returns an error, causing destroy to fail when ec2:DescribeHosts is missing; update the switch in the same function so that for cases "UnauthorizedOperation" and "AccessDenied" you log the warning via logger.Warn("User does not have permission to describe dedicated hosts, skipping") and then return nil instead of falling through to the final fmt.Errorf return; keep the existing behavior for "InvalidHostID.NotFound" (return nil) and for other errors continue to return fmt.Errorf("failed to describe dedicated host %s: %w", id, err).pkg/asset/installconfig/aws/metadata.go (1)
379-391:⚠️ Potential issue | 🟡 MinorCache the empty dedicated-host result too.
len(m.Hosts) == 0conflates “not loaded yet” with “loaded, but there are no hosts”. In an account with zero dedicated hosts, this will callDescribeHostson every access and defeat the cache.Suggested fix
- if len(m.Hosts) == 0 { + if m.Hosts == nil { client, err := m.EC2Client(ctx) if err != nil { return nil, err } - m.Hosts, err = dedicatedHosts(ctx, client) + hosts, err := dedicatedHosts(ctx, client) if err != nil { return nil, fmt.Errorf("error listing dedicated hosts: %w", err) } + m.Hosts = hosts }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/metadata.go` around lines 379 - 391, The cache check uses len(m.Hosts) == 0 which treats “not loaded” and “loaded-but-empty” the same and causes repeated DescribeHosts calls; change the guard to check for nil (if m.Hosts == nil) so an empty slice returned by dedicatedHosts is cached, and ensure dedicatedHosts or the assignment leaves m.Hosts as a non-nil empty slice when no hosts are found (refer to m.Hosts, EC2Client, and dedicatedHosts).
🧹 Nitpick comments (1)
pkg/types/aws/validation/machinepool.go (1)
111-121: Missing validation for duplicate host IDs.Per past review discussion, duplicate host IDs in the
DedicatedHostlist should be flagged. Currently, duplicates are silently accepted but would cause logical issues downstream (e.g., the map inDedicatedHost()helper would deduplicate them).♻️ Proposed fix to add uniqueness validation
case aws.HostAffinityDedicatedHost: if len(p.HostPlacement.DedicatedHost) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("dedicatedHost"), "dedicatedHost is required when 'affinity' is set to DedicatedHost, and forbidden otherwise")) } else { + seenIDs := make(map[string]int) for index, host := range p.HostPlacement.DedicatedHost { hostPath := fldPath.Child("dedicatedHost").Index(index) if len(host.ID) == 0 { allErrs = append(allErrs, field.Required(hostPath.Child("id"), "a hostID must be specified when configuring 'dedicatedHost'")) + } else if prevIdx, seen := seenIDs[host.ID]; seen { + allErrs = append(allErrs, field.Duplicate(hostPath.Child("id"), host.ID)) + } else { + seenIDs[host.ID] = index } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/validation/machinepool.go` around lines 111 - 121, The DedicatedHost branch in Validate (case aws.HostAffinityDedicatedHost) lacks a check for duplicate host IDs in p.HostPlacement.DedicatedHost; add uniqueness validation by iterating the slice (the same loop that checks host.ID) and track seen IDs (e.g., with a map[string]bool) to append a field.Duplicate or field.Invalid error to fldPath.Child("dedicatedHost").Index(index).Child("id") when an ID repeats, ensuring behavior matches the DedicatedHost() helper which deduplicates IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 202-251: The schema allows affinity: DedicatedHost with an empty
dedicatedHost array because has(self.dedicatedHost) only checks presence; update
the dedicatedHost array definition (the dedicatedHost property under
hostPlacement with type: array and item schema containing id) to include
minItems: 1 so empty lists are rejected, and ensure the existing
x-kubernetes-validations rule that references self.dedicatedHost remains
unchanged; apply the same minItems: 1 addition to the other duplicated
dedicatedHost array blocks referenced in the comment (the other two
occurrences).
In `@pkg/asset/cluster/aws/aws.go`:
- Around line 253-269: The collector currently only reads compute.Platform.AWS
from installConfig.Config.Compute for the "worker" pool and misses values
inherited from installConfig.Config.AWS.DefaultMachinePlatform; update the logic
that builds dhNames to first start from
installConfig.Config.AWS.DefaultMachinePlatform (create an awstypes.MachinePool
and call mpool.Set on the default), then overlay the worker compute (if present)
by calling mpool.Set(compute.Platform.AWS) as done in
tagSharedIAMRoles/tagSharedIAMProfiles, handle the compute == nil fallback, and
then read mpool.HostPlacement.DedicatedHost to append name.ID into dhNames so
inherited dedicated hosts are included.
- Around line 284-288: The error wrapper for the ec2Client.CreateTags call is
misleading (mentions subnets); update the wrapped error message in the
CreateTags error return to correctly reference dedicated hosts (e.g., change
"could not add tags to subnets" to "could not add tags to dedicated hosts" or
similar) so callers of ec2Client.CreateTags (using dhNames, tagKey, tagValue)
see the correct resource in the error text.
In `@pkg/asset/installconfig/aws/dedicatedhosts.go`:
- Line 36: Fix the typo in the debug log call by updating the log message string
used in the logrus.Debugf invocation (currently logrus.Debugf("Found dedicatd
host: %s", id)) to the correct wording, e.g. "Found dedicated host: %s",
preserving the formatting and the id argument.
In `@pkg/asset/installconfig/aws/permissions.go`:
- Around line 442-445: RequiredPermissionGroups currently includes
PermissionDedicatedHosts but not PermissionDynamicHostAllocation, so delete-time
permission assembly omits the ec2:ReleaseHosts action; update the code that
builds the destroy/delete permission set (the logic that assembles
RequiredPermissionGroups for destroy) to also include
PermissionDynamicHostAllocation alongside PermissionDedicatedHosts so
ec2:ReleaseHosts is present at destroy time; make the same change where the
delete-time groups are assembled later in the file (the second occurrence around
the 581-583 area). Ensure you reference PermissionDynamicHostAllocation,
PermissionDedicatedHosts and RequiredPermissionGroups when making the change.
- Around line 795-804: The includesDedicatdHosts helper currently only checks
pools named exactly types.MachinePoolComputeRoleName and misses other compute
pools with AWS.HostPlacement; change the loop condition to scan all compute
pools except edge pools (i.e., skip only when mpool.Name ==
types.MachinePoolEdgeRoleName) and return true if any mpool.Platform.AWS != nil
&& mpool.Platform.AWS.HostPlacement != nil; update includesDedicatdHosts
accordingly so DescribeHosts/ReleaseHosts run for any non-edge compute pool with
host placement.
In `@pkg/asset/installconfig/aws/validation.go`:
- Around line 556-560: The validation currently allows a dedicated-host pool
when pool.Zones is empty, which makes zone-to-host mapping non-deterministic;
add an explicit check in the same validation block to reject dedicated-host
pools that don't specify Zones: if len(pool.Zones) == 0 (and this is a
dedicated-host pool context), append a field.Invalid error on dhPath (use host
as the bad value) with a message like "machine pool with dedicated hosts must
specify explicit zones" so we fail fast instead of accepting any zone and later
producing MachineSets that lose dedicated-host placement.
In `@pkg/asset/machines/aws/machines.go`:
- Around line 371-382: The DedicatedHost function currently returns an empty
string when a placement is provided but no host matches the zone, which silently
omits Placement.Host in provider(); change DedicatedHost to return (string,
error), return a non-nil error when placement != nil and no host.ID matches the
given zone (include context like requested host IDs and zone), and update the
caller provider() to propagate or handle that error so the invalid host/zone
mapping is surfaced (e.g., fail validation or emit a clear error) rather than
falling back to shared placement.
In `@pkg/asset/machines/worker.go`:
- Around line 538-543: The code currently calls
installConfig.AWS.DedicatedHosts(ctx) whenever mpool.HostPlacement != nil, which
also runs for valid AnyAvailable placements; change the guard to only call
DedicatedHosts when the placement explicitly indicates DedicatedHost affinity
(i.e., check mpool.HostPlacement == DedicatedHost or the equivalent
enum/constant used in your codebase), so only the DedicatedHost case triggers
installConfig.AWS.DedicatedHosts(ctx) and other placements (e.g., AnyAvailable)
skip the host lookup.
In `@pkg/types/aws/validation/machinepool_test.go`:
- Around line 299-321: Test cases assert short 8-character host IDs as valid but
the validator in pkg/types/aws/machinepool.go expects DedicatedHost.ID to match
^h-[0-9a-f]{17}$; update the failing test entries in
pkg/types/aws/validation/machinepool_test.go that construct aws.MachinePool ->
aws.HostPlacement -> DedicatedHost (IDs "h-1a2b3c4d" and "h-9876abcd") to use
valid 17-hex-character IDs (e.g., "h-0123456789abcdef0") or mark them as invalid
so they align with the DedicatedHost.ID validation. Ensure you adjust the test
name/expectation (valid vs invalid) for those cases accordingly.
In `@pkg/types/aws/validation/machinepool.go`:
- Around line 85-99: The error path is being double-nested because fldPath is
already hostPlacement; remove the extra fldPath.Child("hostPlacement") calls and
use fldPath directly when building the Invalid field errors. Update both checks
that append to allErrs (the block comparing poolName to
types.MachinePoolComputeRoleName and the block for control plane pools) so they
pass fldPath (not fldPath.Child(...)) to field.Invalid along with
p.HostPlacement and the existing errMsg; keep the rest of the logic and messages
unchanged.
---
Outside diff comments:
In `@pkg/asset/machines/aws/machinesets.go`:
- Around line 91-109: The code currently continues to build and emit a
machineset even when DedicatedHost(in.Hosts, mpool.HostPlacement, az) returns an
empty dedicatedHost, which silently drops host placement and creates a regular
machineset; update the logic around calling provider(&machineProviderInput{...})
to check the pool's affinity (mpool.HostPlacement or the field that indicates
Affinity=DedicatedHost) and if affinity is DedicatedHost and dedicatedHost == ""
then skip creating/emitting the machineset (return or continue) so we don't
produce a non-dedicated machineset for that AZ; reference DedicatedHost(), the
dedicatedHost variable, machineProviderInput and mpool.HostPlacement when making
the change.
---
Duplicate comments:
In `@pkg/asset/installconfig/aws/metadata.go`:
- Around line 379-391: The cache check uses len(m.Hosts) == 0 which treats “not
loaded” and “loaded-but-empty” the same and causes repeated DescribeHosts calls;
change the guard to check for nil (if m.Hosts == nil) so an empty slice returned
by dedicatedHosts is cached, and ensure dedicatedHosts or the assignment leaves
m.Hosts as a non-nil empty slice when no hosts are found (refer to m.Hosts,
EC2Client, and dedicatedHosts).
In `@pkg/destroy/aws/ec2helpers.go`:
- Around line 973-985: The DescribeHosts error handling in the block using
HandleErrorCode(err) incorrectly logs a warning for "UnauthorizedOperation" and
"AccessDenied" but still returns an error, causing destroy to fail when
ec2:DescribeHosts is missing; update the switch in the same function so that for
cases "UnauthorizedOperation" and "AccessDenied" you log the warning via
logger.Warn("User does not have permission to describe dedicated hosts,
skipping") and then return nil instead of falling through to the final
fmt.Errorf return; keep the existing behavior for "InvalidHostID.NotFound"
(return nil) and for other errors continue to return fmt.Errorf("failed to
describe dedicated host %s: %w", id, err).
---
Nitpick comments:
In `@pkg/types/aws/validation/machinepool.go`:
- Around line 111-121: The DedicatedHost branch in Validate (case
aws.HostAffinityDedicatedHost) lacks a check for duplicate host IDs in
p.HostPlacement.DedicatedHost; add uniqueness validation by iterating the slice
(the same loop that checks host.ID) and track seen IDs (e.g., with a
map[string]bool) to append a field.Duplicate or field.Invalid error to
fldPath.Child("dedicatedHost").Index(index).Child("id") when an ID repeats,
ensuring behavior matches the DedicatedHost() helper which deduplicates IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ae35e0e-d65b-46e5-883d-66408e6032fb
⛔ Files ignored due to path filters (1)
pkg/types/aws/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (20)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/cluster/aws/aws.gopkg/asset/installconfig/aws/dedicatedhosts.gopkg/asset/installconfig/aws/metadata.gopkg/asset/installconfig/aws/permissions.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/aws/validation_test.gopkg/asset/machines/aws/machines.gopkg/asset/machines/aws/machinesets.gopkg/asset/machines/worker.gopkg/asset/machines/worker_test.gopkg/destroy/aws/ec2helpers.gopkg/types/aws/machinepool.gopkg/types/aws/validation/featuregates.gopkg/types/aws/validation/featuregates_test.gopkg/types/aws/validation/machinepool.gopkg/types/aws/validation/machinepool_test.gopkg/types/aws/validation/platform.gopkg/types/machinepools.gopkg/types/validation/machinepools.go
| hostPlacement: | ||
| description: |- | ||
| hostPlacement configures placement on AWS Dedicated Hosts. This allows admins to assign instances to specific host | ||
| for a variety of needs including for regulatory compliance, to leverage existing per-socket or per-core software licenses (BYOL), | ||
| and to gain visibility and control over instance placement on a physical server. | ||
| When omitted, the instance is not constrained to a dedicated host. | ||
| properties: | ||
| affinity: | ||
| description: |- | ||
| affinity specifies the affinity setting for the instance. | ||
| Allowed values are AnyAvailable and DedicatedHost. | ||
| When Affinity is set to DedicatedHost, an instance started onto a specific host always restarts on the same host if stopped. In this scenario, the `dedicatedHost` field must be set. | ||
| When Affinity is set to AnyAvailable, and you stop and restart the instance, it can be restarted on any available host. | ||
| enum: | ||
| - DedicatedHost | ||
| - AnyAvailable | ||
| type: string | ||
| dedicatedHost: | ||
| description: |- | ||
| dedicatedHost specifies the exact host that an instance should be restarted on if stopped. | ||
| dedicatedHost is required when 'affinity' is set to DedicatedHost, and forbidden otherwise. | ||
| items: | ||
| description: DedicatedHost represents the configuration | ||
| for the usage of dedicated host. | ||
| properties: | ||
| id: | ||
| description: |- | ||
| id identifies the AWS Dedicated Host on which the instance must run. | ||
| The value must start with "h-" followed by 17 lowercase hexadecimal characters (0-9 and a-f). | ||
| Must be exactly 19 characters in length. | ||
| maxLength: 19 | ||
| minLength: 19 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: hostID must start with 'h-' followed | ||
| by 17 lowercase hexadecimal characters (0-9 | ||
| and a-f) | ||
| rule: self.matches('^h-[0-9a-f]{17}$') | ||
| required: | ||
| - id | ||
| type: object | ||
| type: array | ||
| required: | ||
| - affinity | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: dedicatedHost is required when affinity is DedicatedHost, | ||
| and forbidden otherwise | ||
| rule: 'has(self.affinity) && self.affinity == ''DedicatedHost'' | ||
| ? has(self.dedicatedHost) : !has(self.dedicatedHost)' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Kubernetes CRD x-kubernetes-validations, does has(self.someList) evaluate to true when the list field is present but empty ([]), and is minItems: 1 the recommended schema-level way to require at least one list element?
💡 Result:
-
Yes. In CRD CEL,
has(self.someList)is a presence test (for JSON-like objects: “is this field/key defined?”), not a “non-empty” test—so ifsomeListis present in the object as[],has(self.someList)evaluates to true. This aligns with CEL’s intent of unifying presence checks across protobuf and JSON-like types. [1][2]- If you need “present and non-empty” in CEL, use something like
has(self.someList) && self.someList.size() > 0.
- If you need “present and non-empty” in CEL, use something like
-
Yes. To require at least one list element at the schema level,
minItems: 1is the standard / recommended OpenAPI-schema validation for arrays (preferred over CEL when an OpenAPI validation exists). [3][4]
Sources: Kubernetes CEL docs [2], cel-go README (has macro semantics) [1], Kubernetes CRD validation best practices (prefer OpenAPI validations) [3], controller-tools / OpenAPI marker for MinItems=1 [4].
Enforce non-empty dedicatedHost array when affinity: DedicatedHost.
The has(self.dedicatedHost) check only validates field presence, so a config with affinity: DedicatedHost and dedicatedHost: [] passes validation despite being unusable. Add minItems: 1 to the array definition to reject empty lists at the schema boundary.
Suggested fix
dedicatedHost:
+ minItems: 1
description: |-Apply the same change at lines 1812–1861, 3362–3411, and 5112–5161.
📝 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.
| hostPlacement: | |
| description: |- | |
| hostPlacement configures placement on AWS Dedicated Hosts. This allows admins to assign instances to specific host | |
| for a variety of needs including for regulatory compliance, to leverage existing per-socket or per-core software licenses (BYOL), | |
| and to gain visibility and control over instance placement on a physical server. | |
| When omitted, the instance is not constrained to a dedicated host. | |
| properties: | |
| affinity: | |
| description: |- | |
| affinity specifies the affinity setting for the instance. | |
| Allowed values are AnyAvailable and DedicatedHost. | |
| When Affinity is set to DedicatedHost, an instance started onto a specific host always restarts on the same host if stopped. In this scenario, the `dedicatedHost` field must be set. | |
| When Affinity is set to AnyAvailable, and you stop and restart the instance, it can be restarted on any available host. | |
| enum: | |
| - DedicatedHost | |
| - AnyAvailable | |
| type: string | |
| dedicatedHost: | |
| description: |- | |
| dedicatedHost specifies the exact host that an instance should be restarted on if stopped. | |
| dedicatedHost is required when 'affinity' is set to DedicatedHost, and forbidden otherwise. | |
| items: | |
| description: DedicatedHost represents the configuration | |
| for the usage of dedicated host. | |
| properties: | |
| id: | |
| description: |- | |
| id identifies the AWS Dedicated Host on which the instance must run. | |
| The value must start with "h-" followed by 17 lowercase hexadecimal characters (0-9 and a-f). | |
| Must be exactly 19 characters in length. | |
| maxLength: 19 | |
| minLength: 19 | |
| type: string | |
| x-kubernetes-validations: | |
| - message: hostID must start with 'h-' followed | |
| by 17 lowercase hexadecimal characters (0-9 | |
| and a-f) | |
| rule: self.matches('^h-[0-9a-f]{17}$') | |
| required: | |
| - id | |
| type: object | |
| type: array | |
| required: | |
| - affinity | |
| type: object | |
| x-kubernetes-validations: | |
| - message: dedicatedHost is required when affinity is DedicatedHost, | |
| and forbidden otherwise | |
| rule: 'has(self.affinity) && self.affinity == ''DedicatedHost'' | |
| ? has(self.dedicatedHost) : !has(self.dedicatedHost)' | |
| hostPlacement: | |
| description: |- | |
| hostPlacement configures placement on AWS Dedicated Hosts. This allows admins to assign instances to specific host | |
| for a variety of needs including for regulatory compliance, to leverage existing per-socket or per-core software licenses (BYOL), | |
| and to gain visibility and control over instance placement on a physical server. | |
| When omitted, the instance is not constrained to a dedicated host. | |
| properties: | |
| affinity: | |
| description: |- | |
| affinity specifies the affinity setting for the instance. | |
| Allowed values are AnyAvailable and DedicatedHost. | |
| When Affinity is set to DedicatedHost, an instance started onto a specific host always restarts on the same host if stopped. In this scenario, the `dedicatedHost` field must be set. | |
| When Affinity is set to AnyAvailable, and you stop and restart the instance, it can be restarted on any available host. | |
| enum: | |
| - DedicatedHost | |
| - AnyAvailable | |
| type: string | |
| dedicatedHost: | |
| minItems: 1 | |
| description: |- | |
| dedicatedHost specifies the exact host that an instance should be restarted on if stopped. | |
| dedicatedHost is required when 'affinity' is set to DedicatedHost, and forbidden otherwise. | |
| items: | |
| description: DedicatedHost represents the configuration | |
| for the usage of dedicated host. | |
| properties: | |
| id: | |
| description: |- | |
| id identifies the AWS Dedicated Host on which the instance must run. | |
| The value must start with "h-" followed by 17 lowercase hexadecimal characters (0-9 and a-f). | |
| Must be exactly 19 characters in length. | |
| maxLength: 19 | |
| minLength: 19 | |
| type: string | |
| x-kubernetes-validations: | |
| - message: hostID must start with 'h-' followed | |
| by 17 lowercase hexadecimal characters (0-9 | |
| and a-f) | |
| rule: self.matches('^h-[0-9a-f]{17}$') | |
| required: | |
| - id | |
| type: object | |
| type: array | |
| required: | |
| - affinity | |
| type: object | |
| x-kubernetes-validations: | |
| - message: dedicatedHost is required when affinity is DedicatedHost, | |
| and forbidden otherwise | |
| rule: 'has(self.affinity) && self.affinity == ''DedicatedHost'' | |
| ? has(self.dedicatedHost) : !has(self.dedicatedHost)' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/data/install.openshift.io_installconfigs.yaml` around lines 202 - 251,
The schema allows affinity: DedicatedHost with an empty dedicatedHost array
because has(self.dedicatedHost) only checks presence; update the dedicatedHost
array definition (the dedicatedHost property under hostPlacement with type:
array and item schema containing id) to include minItems: 1 so empty lists are
rejected, and ensure the existing x-kubernetes-validations rule that references
self.dedicatedHost remains unchanged; apply the same minItems: 1 addition to the
other duplicated dedicatedHost array blocks referenced in the comment (the other
two occurrences).
| PermissionDynamicHostAllocation: { | ||
| // This is only used during cluster destroy if during cluster destroy we detect a dedicated host with appropriate tags on it. | ||
| "ec2:ReleaseHosts", | ||
| }, |
There was a problem hiding this comment.
Also append the destroy-time host release permission.
RequiredPermissionGroups opts into PermissionDedicatedHosts, but never PermissionDynamicHostAllocation. Since this function already assembles the delete-time permission set too, creds validation will miss ec2:ReleaseHosts, so dedicated-host clusters can validate successfully and still fail to clean up dynamically allocated hosts on destroy.
Suggested fix
if includesDedicatdHosts(ic) {
permissionGroups = append(permissionGroups, PermissionDedicatedHosts)
+ if !isSecretRegion {
+ permissionGroups = append(permissionGroups, PermissionDynamicHostAllocation)
+ }
}Also applies to: 581-583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/installconfig/aws/permissions.go` around lines 442 - 445,
RequiredPermissionGroups currently includes PermissionDedicatedHosts but not
PermissionDynamicHostAllocation, so delete-time permission assembly omits the
ec2:ReleaseHosts action; update the code that builds the destroy/delete
permission set (the logic that assembles RequiredPermissionGroups for destroy)
to also include PermissionDynamicHostAllocation alongside
PermissionDedicatedHosts so ec2:ReleaseHosts is present at destroy time; make
the same change where the delete-time groups are assembled later in the file
(the second occurrence around the 581-583 area). Ensure you reference
PermissionDynamicHostAllocation, PermissionDedicatedHosts and
RequiredPermissionGroups when making the change.
| func DedicatedHost(hosts map[string]aws.Host, placement *awstypes.HostPlacement, zone string) string { | ||
| // If install-config has HostPlacements configured, lets check the DedicatedHosts to see if one matches our region & zone. | ||
| if placement != nil { | ||
| // We only support one host ID currently for an instance. Need to also get host that matches the zone the machines will be put into. | ||
| for _, host := range placement.DedicatedHost { | ||
| hostDetails, found := hosts[host.ID] | ||
| if found && hostDetails.Zone == zone { | ||
| return hostDetails.ID | ||
| } | ||
| } | ||
| } | ||
| return "" |
There was a problem hiding this comment.
Don’t silently fall back to shared placement when no host matches the zone.
Returning "" here causes provider() on Lines 309-318 to omit Placement.Host completely. For a pool with Affinity == DedicatedHost, that emits a normal instance config instead of surfacing an invalid host/zone mapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/machines/aws/machines.go` around lines 371 - 382, The DedicatedHost
function currently returns an empty string when a placement is provided but no
host matches the zone, which silently omits Placement.Host in provider(); change
DedicatedHost to return (string, error), return a non-nil error when placement
!= nil and no host.ID matches the given zone (include context like requested
host IDs and zone), and update the caller provider() to propagate or handle that
error so the invalid host/zone mapping is surfaced (e.g., fail validation or
emit a clear error) rather than falling back to shared placement.
|
/unhold |
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-merge-robot
removed
the
needs-rebase
|
@vr4manta: This pull request references SPLAT-2172 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. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
pkg/types/aws/validation/machinepool_test.go (1)
300-323:⚠️ Potential issue | 🟠 MajorShort host IDs contradict the documented validation pattern.
These test cases expect 8-character host IDs (
h-1a2b3c4d,h-9876abcd) to be valid, but theDedicatedHost.IDvalidation is documented as^h-[0-9a-f]{17}$(17 hex characters). Either the tests or the validation regex needs updating for consistency.#!/bin/bash # Check the actual validation regex for DedicatedHost.ID rg -n "h-\[0-9a-f\]" --type go # Also check the DedicatedHost type definition ast-grep --pattern 'type DedicatedHost struct { $$$ }'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/validation/machinepool_test.go` around lines 300 - 323, The tests in machinepool_test.go are using 8-character host IDs (e.g., in the MachinePool -> HostPlacement -> DedicatedHost.ID values "h-1a2b3c4d" and "h-9876abcd") which contradict the documented/implemented validation pattern for DedicatedHost.ID (^h-[0-9a-f]{17}$); update the test cases to use valid 17-hex-character IDs (prefix with "h-" followed by 17 lowercase hex characters) so they match the DedicatedHost.ID validation, HostPlacement, and MachinePool expectations, or alternatively change the validation regex where DedicatedHost.ID is validated to accept 8-character IDs if you intend to support the shorter format—pick one approach and make the corresponding change consistently.data/data/install.openshift.io_installconfigs.yaml (1)
219-243:⚠️ Potential issue | 🟠 MajorRequire at least one
dedicatedHostentry whenaffinity: DedicatedHost.
has(self.dedicatedHost)only checks field presence, sodedicatedHost: []still passes this validation and leaves an unusable config. AddminItems: 1to this array here, and mirror the same change in the other identical AWShostPlacementblocks in this file.Suggested fix
dedicatedHost: + minItems: 1 description: |- dedicatedHost specifies the exact host that an instance should be restarted on if stopped.Also applies to: 247-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/data/install.openshift.io_installconfigs.yaml` around lines 219 - 243, The dedicatedHost array definition is missing a minimum item constraint so configurations like dedicatedHost: [] pass has(...) checks; update the array schema for the dedicatedHost property (the object with properties.id and x-kubernetes-validations) to include "minItems: 1" to require at least one entry, and apply the same "minItems: 1" change to the identical AWS hostPlacement/dedicatedHost blocks elsewhere in the file (the other hostPlacement/dedicatedHost definitions referenced around lines 247-251).pkg/asset/installconfig/aws/permissions.go (2)
794-804:⚠️ Potential issue | 🟠 MajorScan every non-edge compute pool for dedicated hosts.
This helper only returns true for pools named
worker. Additional compute pools withplatform.aws.hostPlacementwill skipDescribeHosts/ReleaseHostspermission validation even though the rest of the dedicated-host flow is per-pool.Suggested fix
-// includesDedicatedHosts checks if any dedicated hosts are specified for worker machine pools. +// includesDedicatedHosts checks if any dedicated hosts are specified for non-edge compute pools. func includesDedicatedHosts(installConfig *types.InstallConfig) bool { for _, mpool := range installConfig.Compute { - if mpool.Name != types.MachinePoolComputeRoleName { + if mpool.Name == types.MachinePoolEdgeRoleName { continue } if mpool.Platform.AWS != nil && mpool.Platform.AWS.HostPlacement != nil { return true } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/permissions.go` around lines 794 - 804, The includesDedicatedHosts function only checks for dedicated hosts on the compute pool named types.MachinePoolComputeRoleName ("worker"), so other non-edge compute pools using platform.aws.hostPlacement are ignored; update includesDedicatedHosts to scan every non-edge compute pool (skip those with mpool.Name == types.MachinePoolEdgeRoleName) and return true if any mpool.Platform.AWS != nil && mpool.Platform.AWS.HostPlacement != nil, referencing the includesDedicatedHosts function and the mpool.Name, mpool.Platform.AWS, and mpool.Platform.AWS.HostPlacement fields to locate where to adjust the check.
581-583:⚠️ Potential issue | 🟠 MajorAlso require
ec2:ReleaseHostsfor dedicated-host installs.
pkg/destroy/aws/ec2helpers.gonow releases installer-owned dedicated hosts during destroy, but this path only appendsPermissionDedicatedHosts. That lets credentials pass validation withoutPermissionDynamicHostAllocation, and cleanup then fails once a dynamic host has to be released.Suggested fix
if includesDedicatedHosts(ic) { permissionGroups = append(permissionGroups, PermissionDedicatedHosts) + if !isSecretRegion { + permissionGroups = append(permissionGroups, PermissionDynamicHostAllocation) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/permissions.go` around lines 581 - 583, When includesDedicatedHosts(ic) is true, also require the dynamic-host-release permission: in the block that currently appends PermissionDedicatedHosts to permissionGroups, append PermissionDynamicHostAllocation (which corresponds to ec2:ReleaseHosts) as well so validation requires both PermissionDedicatedHosts and PermissionDynamicHostAllocation before proceeding.
🧹 Nitpick comments (3)
pkg/types/aws/validation/machinepool_test.go (1)
382-391: Simplify the poolName defaulting logic.The current logic infers
poolNamefromtc.expectedstring contents, which is fragile and hard to follow. Consider explicitly settingpoolNamein all test cases that need specific behavior, using a constant for clarity.♻️ Suggested approach
for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - poolName := tc.poolName - // If poolName is not specified, default to worker pool unless the test - // is explicitly testing control plane or edge pool behavior - if poolName == "" && tc.expected != "" && - !strings.Contains(tc.expected, "control plane") && - !strings.Contains(tc.expected, "edge pool") { - poolName = types.MachinePoolComputeRoleName - } else if poolName == "" && tc.expected == "" { - poolName = types.MachinePoolComputeRoleName - } + poolName := tc.poolName + if poolName == "" { + poolName = types.MachinePoolComputeRoleName + } err := ValidateMachinePool(platform, tc.pool, poolName, field.NewPath("test-path")).ToAggregate()Then explicitly set
poolName: types.MachinePoolControlPlaneRoleName(or the appropriate constant) in the control-plane test case at line 363 instead of relying on the empty string convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/validation/machinepool_test.go` around lines 382 - 391, The defaulting logic for poolName in the test is fragile because it infers role from tc.expected string contents; replace this by explicitly setting poolName in each test case that requires a non-default role and simplify the test setup to always use tc.poolName (falling back only to types.MachinePoolComputeRoleName if tc.poolName is empty). Update the control-plane test case to set poolName: types.MachinePoolControlPlaneRoleName (and edge tests to types.MachinePoolEdgeRoleName if present) instead of relying on empty tc.poolName and checking tc.expected text.pkg/asset/installconfig/aws/dedicatedhosts.go (1)
23-55: Consider returning an error when requested hosts are not found.When specific host IDs are passed via the
hostsparameter, the function silently returns a partial result if some IDs don't exist or aren't accessible. Callers may expect all requested hosts to be present and could silently proceed with incomplete data.Consider validating that all requested hosts were returned:
♻️ Proposed validation
+ // Validate that all requested hosts were found + if len(hosts) > 0 { + for _, id := range hosts { + if _, found := hostsByID[id]; !found { + return nil, fmt.Errorf("dedicated host %q not found or not accessible", id) + } + } + } + return hostsByID, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/dedicatedhosts.go` around lines 23 - 55, The function dedicatedHosts should validate that when the hosts parameter is non-empty all requested IDs were returned; after populating hostsByID (in dedicatedHosts, using hostsByID and input.HostIds/hosts), compute the set of requested IDs missing from hostsByID and if any are missing return a descriptive error (e.g., "requested hosts not found: <ids>") instead of silently returning a partial map; keep returning the map and nil only when all requested IDs are present or when hosts was empty.pkg/asset/installconfig/aws/validation_test.go (1)
2431-2443: Make the zone-specific helper actually encode zone-specific state.
withComputeHostPlacementAndZoneignoreszone, so it currently builds the same install-config aswithComputeHostPlacement. That makes the test names more specific than the fixture they are actually exercising.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/validation_test.go` around lines 2431 - 2443, The helper withComputeHostPlacementAndZone currently ignores the zone parameter; update it to set the zone on each DedicatedHost so the resulting InstallConfig is zone-specific. In the function withComputeHostPlacementAndZone, when building dhs (aws.DedicatedHost), populate the appropriate zone field on aws.DedicatedHost (e.g., Zone or AvailabilityZone) with the provided zone string before assigning to ic.Compute[index].Platform.AWS.HostPlacement so the fixture differs from withComputeHostPlacement by zone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/cluster/aws/aws.go`:
- Around line 255-266: The code currently only inspects the "worker" compute
entry; update the loop over installConfig.Config.Compute to handle every compute
pool by creating the awstypes.MachinePool (mpool) for each compute (using
mpool.Set(installConfig.Config.AWS.DefaultMachinePlatform) and
mpool.Set(compute.Platform.AWS)), then if mpool.HostPlacement != nil &&
mpool.HostPlacement.DedicatedHost != nil iterate
mpool.HostPlacement.DedicatedHost and append each name.ID to dhNames so BYO
dedicated hosts from non-"worker" pools also get the shared tag; keep references
to compute.Name, awstypes.MachinePool, mpool.Set,
mpool.HostPlacement.DedicatedHost and dhNames to locate the change.
In `@pkg/asset/installconfig/aws/dedicatedhosts.go`:
- Around line 18-19: Update the struct field comment for Tags in the Host
definition to correctly state it refers to the Host's tags (not the VPC); locate
the Host struct and the Tags field in dedicatedhosts.go and change the comment
"Tags is the map of the VPC's tags." to a brief description such as "Tags is the
map of the Host's tags." so the documentation matches the Host struct.
In `@pkg/asset/installconfig/aws/validation.go`:
- Around line 592-597: The ownership validation is reporting the dedicated-host
error against the pool-level fldPath instead of the specific dedicated-host
dhPath; update the field.Forbidden call that appends to allErrs (the one using
foundHost.ID and clusterIDs, TagValueOwned) to use dhPath instead of fldPath so
the error is associated with the dedicated host entry (referencing foundHost,
clusterIDs, dhPath, fldPath, field.Forbidden, and allErrs).
- Around line 569-581: The local variable zones is computed via
meta.AvailabilityZones(ctx) but never used; update the validation in the
function containing the pool/Zones logic to either 1) replace the existing check
that uses pool.Zones with a check against the resolved zones variable (i.e., use
zones when pool.Zones is empty), so the condition becomes based on zones and
still reports the error with host.ID and foundHost.Zone (references: zones,
pool.Zones, meta.AvailabilityZones(ctx), foundHost.Zone, dhPath), or 2) remove
the dead fetch entirely if you only intend to validate when pool.Zones is
explicitly set; choose one approach and delete the unused assignment and error
handling for meta.AvailabilityZones(ctx) if removing the dead code.
In `@pkg/types/aws/machinepool.go`:
- Around line 238-239: Update the public comment for HostAffinityDedicatedHost
to reference the correct YAML path for the schema: replace the stale
`dedicatedHost.host.hostID` with the actual field `dedicatedHost[].id` in the
comment that documents HostAffinityDedicatedHost (symbol:
HostAffinityDedicatedHost HostAffinity) so generated docs point to the correct
config path.
In `@pkg/types/aws/validation/featuregates.go`:
- Around line 28-37: The current loop over c.Compute only adds the
AWSDedicatedHosts gate when compute[*].platform.aws.hostPlacement is set, but
hostPlacement can also appear on controlPlane.platform.aws.hostPlacement and
platform.aws.defaultMachinePlatform (and on aws.MachinePool types), so add
checks that mirror the existing logic for those paths: inspect
c.ControlPlane.Platform.AWS.HostPlacement and
c.Platform.AWS.DefaultMachinePlatform.HostPlacement (and any
MachinePool/DefaultMachinePlatform structures) and, when non-nil, append a
featuregates.GatedInstallConfigFeature using
features.FeatureGateAWSDedicatedHosts with an appropriate field.NewPath pointing
to "controlPlane","platform","aws","hostPlacement" or
"platform","aws","defaultMachinePlatform","hostPlacement" (and for any machine
pool paths use "machinePool" or the same index scheme as compute), ensuring you
do not duplicate the gate (break/guard if already added).
---
Duplicate comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 219-243: The dedicatedHost array definition is missing a minimum
item constraint so configurations like dedicatedHost: [] pass has(...) checks;
update the array schema for the dedicatedHost property (the object with
properties.id and x-kubernetes-validations) to include "minItems: 1" to require
at least one entry, and apply the same "minItems: 1" change to the identical AWS
hostPlacement/dedicatedHost blocks elsewhere in the file (the other
hostPlacement/dedicatedHost definitions referenced around lines 247-251).
In `@pkg/asset/installconfig/aws/permissions.go`:
- Around line 794-804: The includesDedicatedHosts function only checks for
dedicated hosts on the compute pool named types.MachinePoolComputeRoleName
("worker"), so other non-edge compute pools using platform.aws.hostPlacement are
ignored; update includesDedicatedHosts to scan every non-edge compute pool (skip
those with mpool.Name == types.MachinePoolEdgeRoleName) and return true if any
mpool.Platform.AWS != nil && mpool.Platform.AWS.HostPlacement != nil,
referencing the includesDedicatedHosts function and the mpool.Name,
mpool.Platform.AWS, and mpool.Platform.AWS.HostPlacement fields to locate where
to adjust the check.
- Around line 581-583: When includesDedicatedHosts(ic) is true, also require the
dynamic-host-release permission: in the block that currently appends
PermissionDedicatedHosts to permissionGroups, append
PermissionDynamicHostAllocation (which corresponds to ec2:ReleaseHosts) as well
so validation requires both PermissionDedicatedHosts and
PermissionDynamicHostAllocation before proceeding.
In `@pkg/types/aws/validation/machinepool_test.go`:
- Around line 300-323: The tests in machinepool_test.go are using 8-character
host IDs (e.g., in the MachinePool -> HostPlacement -> DedicatedHost.ID values
"h-1a2b3c4d" and "h-9876abcd") which contradict the documented/implemented
validation pattern for DedicatedHost.ID (^h-[0-9a-f]{17}$); update the test
cases to use valid 17-hex-character IDs (prefix with "h-" followed by 17
lowercase hex characters) so they match the DedicatedHost.ID validation,
HostPlacement, and MachinePool expectations, or alternatively change the
validation regex where DedicatedHost.ID is validated to accept 8-character IDs
if you intend to support the shorter format—pick one approach and make the
corresponding change consistently.
---
Nitpick comments:
In `@pkg/asset/installconfig/aws/dedicatedhosts.go`:
- Around line 23-55: The function dedicatedHosts should validate that when the
hosts parameter is non-empty all requested IDs were returned; after populating
hostsByID (in dedicatedHosts, using hostsByID and input.HostIds/hosts), compute
the set of requested IDs missing from hostsByID and if any are missing return a
descriptive error (e.g., "requested hosts not found: <ids>") instead of silently
returning a partial map; keep returning the map and nil only when all requested
IDs are present or when hosts was empty.
In `@pkg/asset/installconfig/aws/validation_test.go`:
- Around line 2431-2443: The helper withComputeHostPlacementAndZone currently
ignores the zone parameter; update it to set the zone on each DedicatedHost so
the resulting InstallConfig is zone-specific. In the function
withComputeHostPlacementAndZone, when building dhs (aws.DedicatedHost), populate
the appropriate zone field on aws.DedicatedHost (e.g., Zone or AvailabilityZone)
with the provided zone string before assigning to
ic.Compute[index].Platform.AWS.HostPlacement so the fixture differs from
withComputeHostPlacement by zone.
In `@pkg/types/aws/validation/machinepool_test.go`:
- Around line 382-391: The defaulting logic for poolName in the test is fragile
because it infers role from tc.expected string contents; replace this by
explicitly setting poolName in each test case that requires a non-default role
and simplify the test setup to always use tc.poolName (falling back only to
types.MachinePoolComputeRoleName if tc.poolName is empty). Update the
control-plane test case to set poolName: types.MachinePoolControlPlaneRoleName
(and edge tests to types.MachinePoolEdgeRoleName if present) instead of relying
on empty tc.poolName and checking tc.expected text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42207298-d563-47ca-9939-72bc72563c98
⛔ Files ignored due to path filters (1)
pkg/types/aws/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (20)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/cluster/aws/aws.gopkg/asset/installconfig/aws/dedicatedhosts.gopkg/asset/installconfig/aws/metadata.gopkg/asset/installconfig/aws/permissions.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/aws/validation_test.gopkg/asset/machines/aws/machines.gopkg/asset/machines/aws/machinesets.gopkg/asset/machines/worker.gopkg/asset/machines/worker_test.gopkg/destroy/aws/ec2helpers.gopkg/types/aws/machinepool.gopkg/types/aws/validation/featuregates.gopkg/types/aws/validation/featuregates_test.gopkg/types/aws/validation/machinepool.gopkg/types/aws/validation/machinepool_test.gopkg/types/aws/validation/platform.gopkg/types/machinepools.gopkg/types/validation/machinepools.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/types/aws/validation/featuregates_test.go
- pkg/asset/machines/aws/machinesets.go
- pkg/asset/machines/aws/machines.go
- pkg/asset/installconfig/aws/metadata.go
- pkg/types/machinepools.go
- pkg/asset/machines/worker.go
| // DH is only allowed to be created in "worker" pool at this time. No need to check default platform. | ||
| for _, compute := range installConfig.Config.Compute { | ||
| if compute.Name == "worker" { | ||
| mpool := awstypes.MachinePool{} | ||
| mpool.Set(installConfig.Config.AWS.DefaultMachinePlatform) | ||
| mpool.Set(compute.Platform.AWS) | ||
| if mpool.HostPlacement != nil && mpool.HostPlacement.DedicatedHost != nil { | ||
| for _, name := range mpool.HostPlacement.DedicatedHost { | ||
| dhNames = append(dhNames, name.ID) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Tag shared dedicated hosts on every supported compute pool.
This collector still hard-codes worker. If a non-edge compute pool carries HostPlacement, its BYO dedicated hosts never get the shared tag here, so destroy can later treat them as installer-owned.
Suggested fix
- // DH is only allowed to be created in "worker" pool at this time. No need to check default platform.
for _, compute := range installConfig.Config.Compute {
- if compute.Name == "worker" {
- mpool := awstypes.MachinePool{}
- mpool.Set(installConfig.Config.AWS.DefaultMachinePlatform)
- mpool.Set(compute.Platform.AWS)
- if mpool.HostPlacement != nil && mpool.HostPlacement.DedicatedHost != nil {
- for _, name := range mpool.HostPlacement.DedicatedHost {
- dhNames = append(dhNames, name.ID)
- }
- }
- }
+ if compute.Name == types.MachinePoolEdgeRoleName {
+ continue
+ }
+ mpool := awstypes.MachinePool{}
+ mpool.Set(installConfig.Config.AWS.DefaultMachinePlatform)
+ mpool.Set(compute.Platform.AWS)
+ if mpool.HostPlacement != nil {
+ for _, host := range mpool.HostPlacement.DedicatedHost {
+ dhNames = append(dhNames, host.ID)
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/cluster/aws/aws.go` around lines 255 - 266, The code currently only
inspects the "worker" compute entry; update the loop over
installConfig.Config.Compute to handle every compute pool by creating the
awstypes.MachinePool (mpool) for each compute (using
mpool.Set(installConfig.Config.AWS.DefaultMachinePlatform) and
mpool.Set(compute.Platform.AWS)), then if mpool.HostPlacement != nil &&
mpool.HostPlacement.DedicatedHost != nil iterate
mpool.HostPlacement.DedicatedHost and append each name.ID to dhNames so BYO
dedicated hosts from non-"worker" pools also get the shared tag; keep references
to compute.Name, awstypes.MachinePool, mpool.Set,
mpool.HostPlacement.DedicatedHost and dhNames to locate the change.
| // Check if any compute pool has dedicated hosts configured | ||
| for idx, compute := range c.Compute { | ||
| if compute.Platform.AWS != nil && compute.Platform.AWS.HostPlacement != nil { | ||
| gatedFeatures = append(gatedFeatures, featuregates.GatedInstallConfigFeature{ | ||
| FeatureGateName: features.FeatureGateAWSDedicatedHosts, | ||
| Condition: true, | ||
| Field: field.NewPath("compute").Index(idx).Child("platform", "aws", "hostPlacement"), | ||
| }) | ||
| break // Only need to add the feature gate once | ||
| } |
There was a problem hiding this comment.
Gate hostPlacement on every path that exposes it.
HostPlacement lives on aws.MachinePool, not just compute pools. This loop only emits AWSDedicatedHosts for compute[*].platform.aws.hostPlacement, so platform.aws.defaultMachinePlatform.hostPlacement and controlPlane.platform.aws.hostPlacement bypass feature-gate validation entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/types/aws/validation/featuregates.go` around lines 28 - 37, The current
loop over c.Compute only adds the AWSDedicatedHosts gate when
compute[*].platform.aws.hostPlacement is set, but hostPlacement can also appear
on controlPlane.platform.aws.hostPlacement and
platform.aws.defaultMachinePlatform (and on aws.MachinePool types), so add
checks that mirror the existing logic for those paths: inspect
c.ControlPlane.Platform.AWS.HostPlacement and
c.Platform.AWS.DefaultMachinePlatform.HostPlacement (and any
MachinePool/DefaultMachinePlatform structures) and, when non-nil, append a
featuregates.GatedInstallConfigFeature using
features.FeatureGateAWSDedicatedHosts with an appropriate field.NewPath pointing
to "controlPlane","platform","aws","hostPlacement" or
"platform","aws","defaultMachinePlatform","hostPlacement" (and for any machine
pool paths use "machinePool" or the same index scheme as compute), ensuring you
do not duplicate the gate (break/guard if already added).
|
@vr4manta: This pull request references SPLAT-2172 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
pkg/asset/machines/aws/machines.go (1)
370-383:⚠️ Potential issue | 🟠 MajorSilent fallback when no host matches the zone.
When
placement != nilbut no host inplacement.DedicatedHostmatches the requested zone, this function returns"", which causesprovider()to omitPlacement.Hostentirely. For a pool withAffinity == DedicatedHost, this silently creates instances without dedicated host placement instead of surfacing the configuration error.Consider returning
(string, error)and returning an error when placement is configured but no host matches the zone, or ensure upstream validation catches this mismatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/aws/machines.go` around lines 370 - 383, The DedicatedHost function currently returns an empty string when placement != nil but no DedicatedHost matches the requested zone, causing silent omission of Placement.Host; change DedicatedHost to return (string, error) and, in the case placement != nil and no host in placement.DedicatedHost matches the given zone, return a non-nil error that clearly states "no dedicated host found for zone <zone>" so callers (e.g., provider() or the machine pool handling code) can fail fast or surface the configuration error; update all call sites of DedicatedHost to handle the new (hostID, err) signature and propagate or surface the error appropriately.pkg/asset/installconfig/aws/dedicatedhosts.go (1)
12-20:⚠️ Potential issue | 🟡 MinorFix documentation: Tags belong to Host, not VPC.
The comment on line 18 incorrectly references "VPC's tags" but this is the
Hoststruct for dedicated hosts.📝 Suggested fix
- // Tags is the map of the VPC's tags. + // Tags is the map of the dedicated host's tags. Tags Tags🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/dedicatedhosts.go` around lines 12 - 20, The struct comment for Host incorrectly says "Tags is the map of the VPC's tags"; update the comment for the Tags field in the Host struct to reflect that these are the host's tags (e.g., "Tags is the map of the host's tags" or "Tags is the map of tags for the host") so the documentation matches the Host struct and its Tags field.data/data/install.openshift.io_installconfigs.yaml (1)
219-251:⚠️ Potential issue | 🟠 MajorReject empty
dedicatedHostlists whenaffinity: DedicatedHost.
has(self.dedicatedHost)only checks field presence, sodedicatedHost: []still passes validation here. That lets an unusable dedicated-host config through the schema and pushes the failure downstream. AddminItems: 1to the array definition in each duplicated block.Suggested fix
dedicatedHost: + minItems: 1 description: |-Apply the same change at Line 1829, Line 3379, and Line 5129.
In Kubernetes CRD x-kubernetes-validations CEL, does `has(self.someList)` return true when the list field is present but empty (`[]`), and is `minItems: 1` the standard schema-level way to require at least one list item?Also applies to: 1829-1861, 3379-3411, 5129-5161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/data/install.openshift.io_installconfigs.yaml` around lines 219 - 251, The validation currently uses has(self.dedicatedHost) which returns true for an empty array, so update each array schema for dedicatedHost (the property named dedicatedHost under the affinity object) to require at least one element by adding minItems: 1 to the array definition; ensure you change the duplicated blocks that define dedicatedHost (the items/object with id and its x-kubernetes-validations) so the x-kubernetes-validations rule 'has(self.affinity) && self.affinity == ''DedicatedHost'' ? has(self.dedicatedHost) : !has(self.dedicatedHost)' will fail on empty lists.
🧹 Nitpick comments (3)
pkg/asset/installconfig/aws/metadata.go (1)
410-435: Consider moving hostIDs construction inside the cache check.The
hostIDsslice is built on every call, even when the result is already cached. Moving the construction inside thelen(m.Hosts) == 0block would be more efficient.♻️ Proposed optimization
func (m *Metadata) DedicatedHosts(ctx context.Context, hosts []typesaws.DedicatedHost) (map[string]Host, error) { m.mutex.Lock() defer m.mutex.Unlock() - // Create array of host IDs for client - hostIDs := make([]string, len(hosts)) - for idx, host := range hosts { - hostIDs[idx] = host.ID - } - if len(m.Hosts) == 0 { + // Create array of host IDs for client + hostIDs := make([]string, len(hosts)) + for idx, host := range hosts { + hostIDs[idx] = host.ID + } + client, err := m.EC2Client(ctx) if err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/metadata.go` around lines 410 - 435, The DedicatedHosts method currently constructs the hostIDs slice on every call even when m.Hosts is already populated; move the hostIDs creation into the cache-miss branch (inside the if len(m.Hosts) == 0 block) so you only build hostIDs when you actually call m.EC2Client and dedicatedHosts; update the code references in DedicatedHosts (hostIDs, m.Hosts, EC2Client(ctx), dedicatedHosts(ctx, client, hostIDs)) accordingly so behavior is unchanged but unnecessary slice allocation is avoided.pkg/asset/installconfig/aws/validation_test.go (1)
2431-2443: Drop the deadzoneparameter from this helper.
withComputeHostPlacementAndZonenever readszone, so these "with zone" cases currently build the sameHostPlacementpayload aswithComputeHostPlacement. That makes the zone-specific tests easy to misread later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/validation_test.go` around lines 2431 - 2443, The helper function withComputeHostPlacementAndZone has an unused zone parameter; remove it from the signature and update all call sites to pass only (hostIDs []string, index int). Specifically, change func (icBuild icBuildForAWS) withComputeHostPlacementAndZone(hostIDs []string, zone string, index int) icOption to func (icBuild icBuildForAWS) withComputeHostPlacementAndZone(hostIDs []string, index int) icOption and adjust any tests that call withComputeHostPlacementAndZone(...) to drop the zone argument so the produced HostPlacement differs only when intended (compare with withComputeHostPlacement if present).pkg/types/aws/validation/machinepool_test.go (1)
382-391: Don't infer the pool kind from the expected error text.A wording-only change to
tc.expectedcan silently switch a case back to worker-pool validation. It would be safer to make the intendedpoolNameexplicit in the table, using a sentinel for control-plane cases instead ofstrings.Contains(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/validation/machinepool_test.go` around lines 382 - 391, The test is inferring pool kind from the error text (tc.expected) which is brittle; instead add an explicit test field (e.g., tc.poolKind or tc.isControlPlane/isEdge) to indicate intended pool type and use that to decide poolName, replacing the strings.Contains checks around tc.expected. Concretely, modify the logic that sets poolName (currently referencing poolName, tc.expected and types.MachinePoolComputeRoleName) to: if tc.poolName != "" use it, else if tc.poolKind == "control-plane" set poolName to the control-plane sentinel, else if tc.poolKind == "edge" set edge sentinel, otherwise default to types.MachinePoolComputeRoleName; update the test table entries to set tc.poolKind for control-plane/edge cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/installconfig/aws/validation.go`:
- Around line 119-125: The code flags
platform.DefaultMachinePlatform.HostPlacement as invalid but then passes
platform.DefaultMachinePlatform into validateMachinePool, which causes the
forbidden HostPlacement to be copied into pools; instead, after appending the
field.Invalid error, make a shallow copy of platform.DefaultMachinePlatform
(e.g. defaultMP := platform.DefaultMachinePlatform), clear its HostPlacement
(set to nil/zero) and pass that sanitized defaultMP into
validateMachinePool(fldPath.Child("defaultMachinePlatform"), ...) so the
forbidden setting is not propagated; keep the original error append intact
(referencing DefaultMachinePlatform, HostPlacement, validateMachinePool,
fldPath.Child).
- Around line 569-580: The code compares meta.AvailabilityZones(ctx) entries to
foundHost.Zone directly which can mismatch full AZ names (us-east-1a) vs short
names (a); update the comparison in the block around pool.Zones /
meta.AvailabilityZones by normalizing both sides to the same form before calling
slices.Contains: extract the short-suffix (text after the last '-') from
foundHost.Zone (e.g., 'a') into a hostShort variable, map the zones slice
entries to their short suffixes (treat entries without '-' as already short),
then check slices.Contains(normalizedZones, hostShort) and use the same
dhPath/field.Invalid error path if it fails; reference pool.Zones,
meta.AvailabilityZones(ctx), foundHost.Zone, and slices.Contains when making the
change.
---
Duplicate comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 219-251: The validation currently uses has(self.dedicatedHost)
which returns true for an empty array, so update each array schema for
dedicatedHost (the property named dedicatedHost under the affinity object) to
require at least one element by adding minItems: 1 to the array definition;
ensure you change the duplicated blocks that define dedicatedHost (the
items/object with id and its x-kubernetes-validations) so the
x-kubernetes-validations rule 'has(self.affinity) && self.affinity ==
''DedicatedHost'' ? has(self.dedicatedHost) : !has(self.dedicatedHost)' will
fail on empty lists.
In `@pkg/asset/installconfig/aws/dedicatedhosts.go`:
- Around line 12-20: The struct comment for Host incorrectly says "Tags is the
map of the VPC's tags"; update the comment for the Tags field in the Host struct
to reflect that these are the host's tags (e.g., "Tags is the map of the host's
tags" or "Tags is the map of tags for the host") so the documentation matches
the Host struct and its Tags field.
In `@pkg/asset/machines/aws/machines.go`:
- Around line 370-383: The DedicatedHost function currently returns an empty
string when placement != nil but no DedicatedHost matches the requested zone,
causing silent omission of Placement.Host; change DedicatedHost to return
(string, error) and, in the case placement != nil and no host in
placement.DedicatedHost matches the given zone, return a non-nil error that
clearly states "no dedicated host found for zone <zone>" so callers (e.g.,
provider() or the machine pool handling code) can fail fast or surface the
configuration error; update all call sites of DedicatedHost to handle the new
(hostID, err) signature and propagate or surface the error appropriately.
---
Nitpick comments:
In `@pkg/asset/installconfig/aws/metadata.go`:
- Around line 410-435: The DedicatedHosts method currently constructs the
hostIDs slice on every call even when m.Hosts is already populated; move the
hostIDs creation into the cache-miss branch (inside the if len(m.Hosts) == 0
block) so you only build hostIDs when you actually call m.EC2Client and
dedicatedHosts; update the code references in DedicatedHosts (hostIDs, m.Hosts,
EC2Client(ctx), dedicatedHosts(ctx, client, hostIDs)) accordingly so behavior is
unchanged but unnecessary slice allocation is avoided.
In `@pkg/asset/installconfig/aws/validation_test.go`:
- Around line 2431-2443: The helper function withComputeHostPlacementAndZone has
an unused zone parameter; remove it from the signature and update all call sites
to pass only (hostIDs []string, index int). Specifically, change func (icBuild
icBuildForAWS) withComputeHostPlacementAndZone(hostIDs []string, zone string,
index int) icOption to func (icBuild icBuildForAWS)
withComputeHostPlacementAndZone(hostIDs []string, index int) icOption and adjust
any tests that call withComputeHostPlacementAndZone(...) to drop the zone
argument so the produced HostPlacement differs only when intended (compare with
withComputeHostPlacement if present).
In `@pkg/types/aws/validation/machinepool_test.go`:
- Around line 382-391: The test is inferring pool kind from the error text
(tc.expected) which is brittle; instead add an explicit test field (e.g.,
tc.poolKind or tc.isControlPlane/isEdge) to indicate intended pool type and use
that to decide poolName, replacing the strings.Contains checks around
tc.expected. Concretely, modify the logic that sets poolName (currently
referencing poolName, tc.expected and types.MachinePoolComputeRoleName) to: if
tc.poolName != "" use it, else if tc.poolKind == "control-plane" set poolName to
the control-plane sentinel, else if tc.poolKind == "edge" set edge sentinel,
otherwise default to types.MachinePoolComputeRoleName; update the test table
entries to set tc.poolKind for control-plane/edge cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77f902f5-c765-43dd-999d-bba9b565455e
⛔ Files ignored due to path filters (1)
pkg/types/aws/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (20)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/cluster/aws/aws.gopkg/asset/installconfig/aws/dedicatedhosts.gopkg/asset/installconfig/aws/metadata.gopkg/asset/installconfig/aws/permissions.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/aws/validation_test.gopkg/asset/machines/aws/machines.gopkg/asset/machines/aws/machinesets.gopkg/asset/machines/worker.gopkg/asset/machines/worker_test.gopkg/destroy/aws/ec2helpers.gopkg/types/aws/machinepool.gopkg/types/aws/validation/featuregates.gopkg/types/aws/validation/featuregates_test.gopkg/types/aws/validation/machinepool.gopkg/types/aws/validation/machinepool_test.gopkg/types/aws/validation/platform.gopkg/types/machinepools.gopkg/types/validation/machinepools.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/asset/machines/aws/machinesets.go
- pkg/types/aws/validation/featuregates_test.go
- pkg/types/machinepools.go
| // Dedicated hosts cannot be configured in defaultMachinePlatform | ||
| if platform.DefaultMachinePlatform.HostPlacement != nil { | ||
| defaultPath := fldPath.Child("defaultMachinePlatform").Child("hostPlacement") | ||
| errMsg := "dedicated hosts cannot be configured in defaultMachinePlatform, they must be specified per machine pool" | ||
| allErrs = append(allErrs, field.Invalid(defaultPath, platform.DefaultMachinePlatform.HostPlacement, errMsg)) | ||
| } | ||
| allErrs = append(allErrs, validateMachinePool(ctx, meta, fldPath.Child("defaultMachinePlatform"), platform, platform.DefaultMachinePlatform, controlPlaneReq, "", "")...) |
There was a problem hiding this comment.
Stop propagating a forbidden default host placement into later pool validation.
This branch records defaultMachinePlatform.hostPlacement as invalid, but Validate later merges DefaultMachinePlatform into the synthesized control-plane and compute pools, and MachinePool.Set now copies HostPlacement too (pkg/types/aws/machinepool.go Lines 123-125). That turns one root-cause error into extra host lookups and secondary hostPlacement errors on other paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/installconfig/aws/validation.go` around lines 119 - 125, The code
flags platform.DefaultMachinePlatform.HostPlacement as invalid but then passes
platform.DefaultMachinePlatform into validateMachinePool, which causes the
forbidden HostPlacement to be copied into pools; instead, after appending the
field.Invalid error, make a shallow copy of platform.DefaultMachinePlatform
(e.g. defaultMP := platform.DefaultMachinePlatform), clear its HostPlacement
(set to nil/zero) and pass that sanitized defaultMP into
validateMachinePool(fldPath.Child("defaultMachinePlatform"), ...) so the
forbidden setting is not propagated; keep the original error append intact
(referencing DefaultMachinePlatform, HostPlacement, validateMachinePool,
fldPath.Child).
| // Is host valid for pools region and zone config? | ||
| // Only check if zones are explicitly configured; if pool.Zones is empty, all zones are allowed | ||
| zones := pool.Zones | ||
| if len(zones) == 0 { | ||
| zones, err = meta.AvailabilityZones(ctx) | ||
| if err != nil { | ||
| allErrs = append(allErrs, field.InternalError(fldPath, fmt.Errorf("unable to retrieve availability zones: %w", err))) | ||
| } | ||
| } | ||
| if len(zones) > 0 && !slices.Contains(zones, foundHost.Zone) { | ||
| errMsg := fmt.Sprintf("machine pool specifies zones %v but dedicated host %s is in zone %s", pool.Zones, host.ID, foundHost.Zone) | ||
| allErrs = append(allErrs, field.Invalid(dhPath, host, errMsg)) |
There was a problem hiding this comment.
The implicit-zone fallback compares different AZ formats.
When pool.Zones is empty, this falls back to meta.AvailabilityZones(ctx) and compares those entries directly with foundHost.Zone. In this package the fallback list is often the short form (a, b, c), while dedicated hosts are returned as full names like us-east-1a, so a valid host can be rejected on the no-explicit-zones path.
Suggested fix
- // Is host valid for pools region and zone config?
- // Only check if zones are explicitly configured; if pool.Zones is empty, all zones are allowed
- zones := pool.Zones
- if len(zones) == 0 {
- zones, err = meta.AvailabilityZones(ctx)
- if err != nil {
- allErrs = append(allErrs, field.InternalError(fldPath, fmt.Errorf("unable to retrieve availability zones: %w", err)))
- }
- }
- if len(zones) > 0 && !slices.Contains(zones, foundHost.Zone) {
+ // Is host valid for the pool's explicit zone config?
+ if len(pool.Zones) > 0 && !slices.Contains(pool.Zones, foundHost.Zone) {
errMsg := fmt.Sprintf("machine pool specifies zones %v but dedicated host %s is in zone %s", pool.Zones, host.ID, foundHost.Zone)
allErrs = append(allErrs, field.Invalid(dhPath, host, errMsg))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/installconfig/aws/validation.go` around lines 569 - 580, The code
compares meta.AvailabilityZones(ctx) entries to foundHost.Zone directly which
can mismatch full AZ names (us-east-1a) vs short names (a); update the
comparison in the block around pool.Zones / meta.AvailabilityZones by
normalizing both sides to the same form before calling slices.Contains: extract
the short-suffix (text after the last '-') from foundHost.Zone (e.g., 'a') into
a hostShort variable, map the zones slice entries to their short suffixes (treat
entries without '-' as already short), then check
slices.Contains(normalizedZones, hostShort) and use the same
dhPath/field.Invalid error path if it fails; reference pool.Zones,
meta.AvailabilityZones(ctx), foundHost.Zone, and slices.Contains when making the
change.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/asset/machines/worker.go (1)
537-555:⚠️ Potential issue | 🟠 MajorFail fast when a resolved AZ has no matching dedicated host.
mpool.Zonesmay already have been defaulted to every worker subnet/AZ before this block runs. If the user only configured dedicated hosts for a subset of those zones,aws.MachineSetswill still generate a MachineSet for the extra AZs, andaws.DedicatedHost(...)resolves to""for them. That leaves aDedicatedHostpool emitting invalid provider specs instead of surfacing a clear installer error.Proposed guard
dHosts := map[string]icaws.Host{} if mpool.HostPlacement != nil && mpool.HostPlacement.Affinity != nil && *mpool.HostPlacement.Affinity == awstypes.HostAffinityDedicatedHost { dHosts, err = installConfig.AWS.DedicatedHosts(ctx, mpool.HostPlacement.DedicatedHost) if err != nil { return fmt.Errorf("failed to retrieve dedicated hosts for compute pool: %w", err) } + for _, zone := range mpool.Zones { + if aws.DedicatedHost(dHosts, mpool.HostPlacement, zone) == "" { + return fmt.Errorf("no dedicated host matches zone %s for compute pool %s", zone, pool.Name) + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/worker.go` around lines 537 - 555, If dedicated hosts were requested (mpool.HostPlacement != nil && mpool.HostPlacement.Affinity != nil && *mpool.HostPlacement.Affinity == awstypes.HostAffinityDedicatedHost), after calling installConfig.AWS.DedicatedHosts(...) and before calling aws.MachineSets(...), validate that every AZ in mpool.Zones has a corresponding entry in the returned dHosts map (or that aws.DedicatedHost(...) would not return an empty string for that AZ); if any AZ is missing a dedicated host, return an error (fmt.Errorf) describing the specific AZ(s) with no matching dedicated host so we fail fast instead of producing MachineSets with empty DedicatedHost values.
♻️ Duplicate comments (7)
pkg/asset/machines/aws/machines.go (1)
370-383:⚠️ Potential issue | 🟠 MajorSilent fallback to shared placement when no host matches the zone.
When
placement != nil(i.e., DedicatedHost affinity is configured) but no host ID matches the given zone, returning""causes the caller to omitPlacement.Hostentirely. This silently produces a non-dedicated instance for a pool that explicitly requested dedicated host placement.Consider returning an error when placement is configured but no matching host is found:
🔧 Suggested approach
-func DedicatedHost(hosts map[string]aws.Host, placement *awstypes.HostPlacement, zone string) string { +func DedicatedHost(hosts map[string]aws.Host, placement *awstypes.HostPlacement, zone string) (string, error) { // If install-config has HostPlacements configured, lets check the DedicatedHosts to see if one matches our region & zone. if placement != nil { + if *placement.Affinity == awstypes.HostAffinityDedicatedHost && len(placement.DedicatedHost) > 0 { + // Expect to find a matching host + } // We only support one host ID currently for an instance. Need to also get host that matches the zone the machines will be put into. for _, host := range placement.DedicatedHost { hostDetails, found := hosts[host.ID] if found && hostDetails.Zone == zone { - return hostDetails.ID + return hostDetails.ID, nil } } + if *placement.Affinity == awstypes.HostAffinityDedicatedHost { + return "", fmt.Errorf("no dedicated host found for zone %s", zone) + } } - return "" + return "", nil },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/aws/machines.go` around lines 370 - 383, The DedicatedHost function currently returns an empty string when placement is non-nil but no host in placement.DedicatedHost matches the given zone, which causes callers to omit Placement.Host; change DedicatedHost to propagate an error in that case (e.g., return an error alongside the host ID or convert the function to (string, error)) so callers can fail fast when DedicatedHost affinity is configured but no matching host is found; update callers to handle the new error return and surface a clear error message referencing DedicatedHost, placement.DedicatedHost and the zone.pkg/types/aws/machinepool.go (1)
238-239:⚠️ Potential issue | 🟡 MinorFix the stale YAML path in the public comment.
The comment references
dedicatedHost.host.idbut the actual schema path isdedicatedHost[].id(since DedicatedHost is a slice).📝 Suggested fix
- // HostAffinityDedicatedHost requires specifying a particular host via dedicatedHost.host.id. + // HostAffinityDedicatedHost requires specifying at least one host via dedicatedHost[].id. HostAffinityDedicatedHost HostAffinity = "DedicatedHost",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/machinepool.go` around lines 238 - 239, Update the public comment for HostAffinityDedicatedHost to correct the stale YAML path: change the referenced schema path from dedicatedHost.host.id to dedicatedHost[].id (reflecting that DedicatedHost is a slice) so the comment accurately points to dedicatedHost[].id.pkg/types/aws/validation/featuregates.go (1)
28-38:⚠️ Potential issue | 🟠 MajorGate
hostPlacementondefaultMachinePlatformas well.This loop only gates
compute[*].platform.aws.hostPlacement. If a user setsplatform.aws.defaultMachinePlatform.hostPlacement, it will be merged into compute pools viaSet()but won't trigger the feature gate check.Consider adding a check before the loop:
+ // Check if defaultMachinePlatform has dedicated hosts configured + if c.Platform.AWS != nil && c.Platform.AWS.DefaultMachinePlatform != nil && c.Platform.AWS.DefaultMachinePlatform.HostPlacement != nil { + gatedFeatures = append(gatedFeatures, featuregates.GatedInstallConfigFeature{ + FeatureGateName: features.FeatureGateAWSDedicatedHosts, + Condition: true, + Field: field.NewPath("platform", "aws", "defaultMachinePlatform", "hostPlacement"), + }) + } // Check if any compute pool has dedicated hosts configured for idx, compute := range c.Compute {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/validation/featuregates.go` around lines 28 - 38, The loop currently only checks compute pools (c.Compute) for AWS HostPlacement; add a pre-check for c.Platform.AWS.DefaultMachinePlatform.HostPlacement and, if non-nil, append the same feature gate (featuregates.GatedInstallConfigFeature with FeatureGateName features.FeatureGateAWSDedicatedHosts, Condition true) using field.NewPath("platform","aws","defaultMachinePlatform","hostPlacement") before iterating compute so the defaultMachinePlatform case is gated (avoid duplicating the gate by performing this check first and only falling back to the existing compute loop if the defaultMachinePlatform check is not set).pkg/types/aws/validation/machinepool_test.go (1)
300-323:⚠️ Potential issue | 🟡 MinorTest cases use host IDs that would fail CRD validation.
These tests are named "valid dedicated host - 8 character format" but use IDs like
h-1a2b3c4d(8 hex chars) which don't match the kubebuilder validation pattern^h-[0-9a-f]{17}$(17 hex chars) defined inpkg/types/aws/machinepool.go.While these tests pass because the Go validation only checks for empty IDs (the regex is enforced by kubebuilder), the test names are misleading. Consider either:
- Using valid 17-character IDs:
h-1a2b3c4d5e6f78901- Renaming tests to clarify they only test Go-level validation
🔧 Suggested fix using valid IDs
{ - name: "valid dedicated host - 8 character format", + name: "valid dedicated host", pool: &aws.MachinePool{ HostPlacement: &aws.HostPlacement{ Affinity: ptr.To(aws.HostAffinityDedicatedHost), DedicatedHost: []aws.DedicatedHost{ - {ID: "h-1a2b3c4d"}, + {ID: "h-1a2b3c4d5e6f78901"}, }, }, }, }, { - name: "valid dedicated host - 8 character format with zone", + name: "valid dedicated host - another example", pool: &aws.MachinePool{ HostPlacement: &aws.HostPlacement{ Affinity: ptr.To(aws.HostAffinityDedicatedHost), DedicatedHost: []aws.DedicatedHost{ { - ID: "h-9876abcd", + ID: "h-9876abcdef0123456", }, }, }, }, },,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/validation/machinepool_test.go` around lines 300 - 323, The test cases named "valid dedicated host - 8 character format" and "valid dedicated host - 8 character format with zone" use DedicatedHost.ID values like "h-1a2b3c4d" which do not match the kubebuilder CRD regex ^h-[0-9a-f]{17}$; update the DedicatedHost.ID fields in those aws.MachinePool test entries (the structs under HostPlacement -> DedicatedHost -> ID) to use valid 17-hex-character host IDs (e.g., "h-1a2b3c4d5e6f78901") or, if you prefer to keep short IDs, rename the test names to indicate they only exercise Go-level (non-CRD) validation rather than CRD-valid IDs.pkg/types/aws/validation/machinepool.go (1)
105-108:⚠️ Potential issue | 🟡 MinorUse
field.Forbiddeninstead offield.Requiredfor AnyAvailable case.When affinity is
AnyAvailableandDedicatedHostis set, the field should be marked as forbidden, not required. The error message says "forbidden otherwise" but usesfield.Required.🐛 Proposed fix
case aws.HostAffinityAnyAvailable: if len(p.HostPlacement.DedicatedHost) > 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("dedicatedHost"), "dedicatedHost is required when 'affinity' is set to DedicatedHost, and forbidden otherwise")) + allErrs = append(allErrs, field.Forbidden(fldPath.Child("dedicatedHost"), "dedicatedHost must not be set when affinity is AnyAvailable")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/validation/machinepool.go` around lines 105 - 108, The validation for the aws.HostAffinityAnyAvailable case is using field.Required but should use field.Forbidden; update the code in the switch case handling aws.HostAffinityAnyAvailable to append field.Forbidden(fldPath.Child("dedicatedHost"), "<message>") instead of field.Required, keeping or clarifying the existing message (e.g., "dedicatedHost is forbidden when 'affinity' is set to AnyAvailable") so the error matches the logic applied to allErrs and fldPath.Child("dedicatedHost").pkg/asset/installconfig/aws/validation.go (1)
592-597:⚠️ Potential issue | 🟡 MinorInconsistent field path in ownership validation error.
The ownership validation error at line 595 uses
fldPath(the pool-level path) instead ofdhPath(the dedicated host entry path). Other errors in this loop consistently usedhPathfor better error localization.Suggested fix
// Check to make sure dedicated host is not owned by another cluster clusterIDs := foundHost.Tags.GetClusterIDs(TagValueOwned) if len(clusterIDs) > 0 { - allErrs = append(allErrs, field.Forbidden(fldPath, + allErrs = append(allErrs, field.Forbidden(dhPath, fmt.Sprintf("Dedicated host %s is owned by other cluster %v and cannot be used for new installations, another Dedicated Host must be created separately", foundHost.ID, clusterIDs))) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/validation.go` around lines 592 - 597, The ownership validation appends an error with the wrong field path (uses the pool-level path variable instead of the dedicated-host entry path), so update the field.Forbidden call in the dedicated host ownership check to use dhPath (the dedicated-host specific path) rather than fldPath; locate the block using variables clusterIDs, foundHost, TagValueOwned, dhPath, allErrs and replace the path argument passed to field.Forbidden so the error is associated with the dedicated host entry.data/data/install.openshift.io_installconfigs.yaml (1)
219-251:⚠️ Potential issue | 🟠 MajorReject empty
dedicatedHostlists whenaffinity: DedicatedHost.Lines 250, 1860, 3410, and 5160 only test field presence, so
dedicatedHost: []still satisfies the CEL rule and an unusable config gets accepted. AddminItems: 1to the array schema in each copy of this block. If this YAML is generated, the fix should be made in the source schema/marker and then regenerated.Suggested fix
dedicatedHost: + minItems: 1 description: |-Also applies to: 1829-1861, 3379-3411, 5129-5161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/data/install.openshift.io_installconfigs.yaml` around lines 219 - 251, The CEL validation only checks for presence of dedicatedHost, so an empty array passes; update the dedicatedHost array schema (the dedicatedHost property definition that contains items -> properties -> id and the surrounding type: array) to include minItems: 1 so empty lists are rejected when affinity == "DedicatedHost"; apply the same change to each duplicated block (the dedicatedHost array definitions that accompany the affinity/x-kubernetes-validations rule referencing affinity == 'DedicatedHost') or, if generated, fix the source schema/marker and regenerate the YAML.
🧹 Nitpick comments (2)
pkg/types/aws/validation/machinepool_test.go (1)
380-392: The poolName defaulting logic is fragile.The logic relies on string matching in
tc.expectedto determine if a test is for control plane or edge pools. This is brittle and could break if error messages change.Consider making this explicit by always setting
poolNamein the test cases:♻️ Suggested approach
for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { poolName := tc.poolName - // If poolName is not specified, default to worker pool unless the test - // is explicitly testing control plane or edge pool behavior - if poolName == "" && tc.expected != "" && - !strings.Contains(tc.expected, "control plane") && - !strings.Contains(tc.expected, "edge pool") { - poolName = types.MachinePoolComputeRoleName - } else if poolName == "" && tc.expected == "" { + if poolName == "" { poolName = types.MachinePoolComputeRoleName }Then update the control plane test case to explicitly set
poolName: types.MachinePoolControlPlaneRoleNameinstead of relying on empty string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/validation/machinepool_test.go` around lines 380 - 392, The current defaulting logic for poolName in the test loop is brittle because it inspects tc.expected text; instead make poolName explicit in each test case and simplify the test setup: remove the string-matching branches around tc.expected and just set poolName := tc.poolName; if poolName == "" set it to types.MachinePoolComputeRoleName (default), and update the control plane test case to set poolName: types.MachinePoolControlPlaneRoleName (and any edge-pool tests to their explicit names) so ValidateMachinePool calls use explicit pool names rather than parsing tc.expected.pkg/asset/machines/worker_test.go (1)
130-155: Add one AWS dedicated-host regression case.These tests now initialize AWS metadata, but they still never exercise
HostPlacement. A case withhostPlacement.affinity=DedicatedHostwould cover the new branch inWorker.Generateand catch zone-to-host mapping regressions much earlier.Also applies to: 189-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/worker_test.go` around lines 130 - 155, Add a new test case in pkg/asset/machines/worker_test.go that exercises AWS HostPlacement by setting the MachinePool AWS HostPlacement affinity to DedicatedHost (e.g. set cfg.Compute[0].Platform.AWS.HostPlacement.Affinity = awstypes.HostAffinityDedicatedHost) before creating the installconfig asset (installconfig.MakeAsset) and initializing icAsset.AWS; this will force the Worker.Generate branch that handles HostPlacement and validate zone-to-host mapping behavior alongside the existing cases (reuse cfg, icAsset.AWS = icaws.NewMetadata(...), and existing tc variants).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/installconfig/aws/dedicatedhosts.go`:
- Around line 18-19: Update the grammatical typo in the field comment for the
Tags field: change "Hosts's" to "Host's" in the comment above the Tags
declaration (the Tags field in dedicatedhosts.go / the Tags struct field) so it
reads "Tags is the map of the Host's tags." Ensure only the comment text is
modified and no code logic is changed.
In `@pkg/asset/installconfig/aws/validation_test.go`:
- Around line 2431-2443: The function withComputeHostPlacementAndZone accepts a
zone but never uses it; update the function to populate the compute pool's AWS
Zones so the zone parameter is applied: inside withComputeHostPlacementAndZone
(which currently sets HostPlacement on ic.Compute[index].Platform.AWS), also set
ic.Compute[index].Platform.AWS.Zones = []string{zone} (or append if multiple
zones are intended); alternatively, if you don’t need zone semantics, remove
withComputeHostPlacementAndZone and use withComputeHostPlacement to avoid
duplication.
---
Outside diff comments:
In `@pkg/asset/machines/worker.go`:
- Around line 537-555: If dedicated hosts were requested (mpool.HostPlacement !=
nil && mpool.HostPlacement.Affinity != nil && *mpool.HostPlacement.Affinity ==
awstypes.HostAffinityDedicatedHost), after calling
installConfig.AWS.DedicatedHosts(...) and before calling aws.MachineSets(...),
validate that every AZ in mpool.Zones has a corresponding entry in the returned
dHosts map (or that aws.DedicatedHost(...) would not return an empty string for
that AZ); if any AZ is missing a dedicated host, return an error (fmt.Errorf)
describing the specific AZ(s) with no matching dedicated host so we fail fast
instead of producing MachineSets with empty DedicatedHost values.
---
Duplicate comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 219-251: The CEL validation only checks for presence of
dedicatedHost, so an empty array passes; update the dedicatedHost array schema
(the dedicatedHost property definition that contains items -> properties -> id
and the surrounding type: array) to include minItems: 1 so empty lists are
rejected when affinity == "DedicatedHost"; apply the same change to each
duplicated block (the dedicatedHost array definitions that accompany the
affinity/x-kubernetes-validations rule referencing affinity == 'DedicatedHost')
or, if generated, fix the source schema/marker and regenerate the YAML.
In `@pkg/asset/installconfig/aws/validation.go`:
- Around line 592-597: The ownership validation appends an error with the wrong
field path (uses the pool-level path variable instead of the dedicated-host
entry path), so update the field.Forbidden call in the dedicated host ownership
check to use dhPath (the dedicated-host specific path) rather than fldPath;
locate the block using variables clusterIDs, foundHost, TagValueOwned, dhPath,
allErrs and replace the path argument passed to field.Forbidden so the error is
associated with the dedicated host entry.
In `@pkg/asset/machines/aws/machines.go`:
- Around line 370-383: The DedicatedHost function currently returns an empty
string when placement is non-nil but no host in placement.DedicatedHost matches
the given zone, which causes callers to omit Placement.Host; change
DedicatedHost to propagate an error in that case (e.g., return an error
alongside the host ID or convert the function to (string, error)) so callers can
fail fast when DedicatedHost affinity is configured but no matching host is
found; update callers to handle the new error return and surface a clear error
message referencing DedicatedHost, placement.DedicatedHost and the zone.
In `@pkg/types/aws/machinepool.go`:
- Around line 238-239: Update the public comment for HostAffinityDedicatedHost
to correct the stale YAML path: change the referenced schema path from
dedicatedHost.host.id to dedicatedHost[].id (reflecting that DedicatedHost is a
slice) so the comment accurately points to dedicatedHost[].id.
In `@pkg/types/aws/validation/featuregates.go`:
- Around line 28-38: The loop currently only checks compute pools (c.Compute)
for AWS HostPlacement; add a pre-check for
c.Platform.AWS.DefaultMachinePlatform.HostPlacement and, if non-nil, append the
same feature gate (featuregates.GatedInstallConfigFeature with FeatureGateName
features.FeatureGateAWSDedicatedHosts, Condition true) using
field.NewPath("platform","aws","defaultMachinePlatform","hostPlacement") before
iterating compute so the defaultMachinePlatform case is gated (avoid duplicating
the gate by performing this check first and only falling back to the existing
compute loop if the defaultMachinePlatform check is not set).
In `@pkg/types/aws/validation/machinepool_test.go`:
- Around line 300-323: The test cases named "valid dedicated host - 8 character
format" and "valid dedicated host - 8 character format with zone" use
DedicatedHost.ID values like "h-1a2b3c4d" which do not match the kubebuilder CRD
regex ^h-[0-9a-f]{17}$; update the DedicatedHost.ID fields in those
aws.MachinePool test entries (the structs under HostPlacement -> DedicatedHost
-> ID) to use valid 17-hex-character host IDs (e.g., "h-1a2b3c4d5e6f78901") or,
if you prefer to keep short IDs, rename the test names to indicate they only
exercise Go-level (non-CRD) validation rather than CRD-valid IDs.
In `@pkg/types/aws/validation/machinepool.go`:
- Around line 105-108: The validation for the aws.HostAffinityAnyAvailable case
is using field.Required but should use field.Forbidden; update the code in the
switch case handling aws.HostAffinityAnyAvailable to append
field.Forbidden(fldPath.Child("dedicatedHost"), "<message>") instead of
field.Required, keeping or clarifying the existing message (e.g., "dedicatedHost
is forbidden when 'affinity' is set to AnyAvailable") so the error matches the
logic applied to allErrs and fldPath.Child("dedicatedHost").
---
Nitpick comments:
In `@pkg/asset/machines/worker_test.go`:
- Around line 130-155: Add a new test case in pkg/asset/machines/worker_test.go
that exercises AWS HostPlacement by setting the MachinePool AWS HostPlacement
affinity to DedicatedHost (e.g. set
cfg.Compute[0].Platform.AWS.HostPlacement.Affinity =
awstypes.HostAffinityDedicatedHost) before creating the installconfig asset
(installconfig.MakeAsset) and initializing icAsset.AWS; this will force the
Worker.Generate branch that handles HostPlacement and validate zone-to-host
mapping behavior alongside the existing cases (reuse cfg, icAsset.AWS =
icaws.NewMetadata(...), and existing tc variants).
In `@pkg/types/aws/validation/machinepool_test.go`:
- Around line 380-392: The current defaulting logic for poolName in the test
loop is brittle because it inspects tc.expected text; instead make poolName
explicit in each test case and simplify the test setup: remove the
string-matching branches around tc.expected and just set poolName :=
tc.poolName; if poolName == "" set it to types.MachinePoolComputeRoleName
(default), and update the control plane test case to set poolName:
types.MachinePoolControlPlaneRoleName (and any edge-pool tests to their explicit
names) so ValidateMachinePool calls use explicit pool names rather than parsing
tc.expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4f9a69f-8259-46bd-9425-e15a45a67d9b
⛔ Files ignored due to path filters (1)
pkg/types/aws/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (20)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/cluster/aws/aws.gopkg/asset/installconfig/aws/dedicatedhosts.gopkg/asset/installconfig/aws/metadata.gopkg/asset/installconfig/aws/permissions.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/aws/validation_test.gopkg/asset/machines/aws/machines.gopkg/asset/machines/aws/machinesets.gopkg/asset/machines/worker.gopkg/asset/machines/worker_test.gopkg/destroy/aws/ec2helpers.gopkg/types/aws/machinepool.gopkg/types/aws/validation/featuregates.gopkg/types/aws/validation/featuregates_test.gopkg/types/aws/validation/machinepool.gopkg/types/aws/validation/machinepool_test.gopkg/types/aws/validation/platform.gopkg/types/machinepools.gopkg/types/validation/machinepools.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/asset/installconfig/aws/metadata.go
- pkg/types/aws/validation/featuregates_test.go
- pkg/types/validation/machinepools.go
- pkg/types/aws/validation/platform.go
- pkg/asset/cluster/aws/aws.go
| func (icBuild icBuildForAWS) withComputeHostPlacementAndZone(hostIDs []string, zone string, index int) icOption { | ||
| return func(ic *types.InstallConfig) { | ||
| aff := aws.HostAffinityDedicatedHost | ||
| dhs := make([]aws.DedicatedHost, 0, len(hostIDs)) | ||
| for _, id := range hostIDs { | ||
| dhs = append(dhs, aws.DedicatedHost{ID: id}) | ||
| } | ||
| ic.Compute[index].Platform.AWS.HostPlacement = &aws.HostPlacement{ | ||
| Affinity: &aff, | ||
| DedicatedHost: dhs, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused zone parameter in withComputeHostPlacementAndZone.
The zone parameter is accepted but never used in the function body. The function is identical to withComputeHostPlacement. Either the zone should be used (e.g., to set ic.Compute[index].Platform.AWS.Zones) or this method should be removed in favor of using withComputeHostPlacement directly.
Suggested fix (if zone should set compute pool zones)
func (icBuild icBuildForAWS) withComputeHostPlacementAndZone(hostIDs []string, zone string, index int) icOption {
return func(ic *types.InstallConfig) {
aff := aws.HostAffinityDedicatedHost
dhs := make([]aws.DedicatedHost, 0, len(hostIDs))
for _, id := range hostIDs {
dhs = append(dhs, aws.DedicatedHost{ID: id})
}
ic.Compute[index].Platform.AWS.HostPlacement = &aws.HostPlacement{
Affinity: &aff,
DedicatedHost: dhs,
}
+ // Set the zone for the compute pool if specified
+ if zone != "" {
+ ic.Compute[index].Platform.AWS.Zones = []string{zone}
+ }
}
}📝 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 (icBuild icBuildForAWS) withComputeHostPlacementAndZone(hostIDs []string, zone string, index int) icOption { | |
| return func(ic *types.InstallConfig) { | |
| aff := aws.HostAffinityDedicatedHost | |
| dhs := make([]aws.DedicatedHost, 0, len(hostIDs)) | |
| for _, id := range hostIDs { | |
| dhs = append(dhs, aws.DedicatedHost{ID: id}) | |
| } | |
| ic.Compute[index].Platform.AWS.HostPlacement = &aws.HostPlacement{ | |
| Affinity: &aff, | |
| DedicatedHost: dhs, | |
| } | |
| } | |
| } | |
| func (icBuild icBuildForAWS) withComputeHostPlacementAndZone(hostIDs []string, zone string, index int) icOption { | |
| return func(ic *types.InstallConfig) { | |
| aff := aws.HostAffinityDedicatedHost | |
| dhs := make([]aws.DedicatedHost, 0, len(hostIDs)) | |
| for _, id := range hostIDs { | |
| dhs = append(dhs, aws.DedicatedHost{ID: id}) | |
| } | |
| ic.Compute[index].Platform.AWS.HostPlacement = &aws.HostPlacement{ | |
| Affinity: &aff, | |
| DedicatedHost: dhs, | |
| } | |
| // Set the zone for the compute pool if specified | |
| if zone != "" { | |
| ic.Compute[index].Platform.AWS.Zones = []string{zone} | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/installconfig/aws/validation_test.go` around lines 2431 - 2443, The
function withComputeHostPlacementAndZone accepts a zone but never uses it;
update the function to populate the compute pool's AWS Zones so the zone
parameter is applied: inside withComputeHostPlacementAndZone (which currently
sets HostPlacement on ic.Compute[index].Platform.AWS), also set
ic.Compute[index].Platform.AWS.Zones = []string{zone} (or append if multiple
zones are intended); alternatively, if you don’t need zone semantics, remove
withComputeHostPlacementAndZone and use withComputeHostPlacement to avoid
duplication.
|
@vr4manta: This pull request references SPLAT-2172 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
pkg/asset/installconfig/aws/validation_test.go (1)
2431-2443:⚠️ Potential issue | 🟡 MinorUnused
zoneparameter inwithComputeHostPlacementAndZone.The
zoneparameter is accepted but never used—this function is identical towithComputeHostPlacement. Either use the zone (e.g., to setic.Compute[index].Platform.AWS.Zones) or remove this method and usewithComputeHostPlacementdirectly.Looking at the test case at line 1593 that uses this method, the intent seems to be testing a mismatch between pool zones and host zones, but the zone parameter doesn't actually set the compute pool's zones.
Suggested fix (if zone should set compute pool zones)
func (icBuild icBuildForAWS) withComputeHostPlacementAndZone(hostIDs []string, zone string, index int) icOption { return func(ic *types.InstallConfig) { aff := aws.HostAffinityDedicatedHost dhs := make([]aws.DedicatedHost, 0, len(hostIDs)) for _, id := range hostIDs { dhs = append(dhs, aws.DedicatedHost{ID: id}) } ic.Compute[index].Platform.AWS.HostPlacement = &aws.HostPlacement{ Affinity: &aff, DedicatedHost: dhs, } + // Ensure the zone is associated with the host placement + if zone != "" && !slices.Contains(ic.Compute[index].Platform.AWS.Zones, zone) { + ic.Compute[index].Platform.AWS.Zones = append(ic.Compute[index].Platform.AWS.Zones, zone) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/validation_test.go` around lines 2431 - 2443, The helper withComputeHostPlacementAndZone currently ignores its zone parameter; update it so that after setting HostPlacement it also assigns the provided zone to the compute pool's AWS Zones (e.g., set ic.Compute[index].Platform.AWS.Zones = []string{zone}) so tests that expect a mismatch between pool zones and host zones are valid; alternatively, if the zone is not needed, remove withComputeHostPlacementAndZone and use the existing withComputeHostPlacement helper instead.pkg/asset/installconfig/aws/permissions.go (1)
793-805:⚠️ Potential issue | 🟡 MinorHelper only checks pools named
worker, missing other compute pools.
includesDedicatedHostsskips pools wherempool.Name != types.MachinePoolComputeRoleName("worker"). This means additional compute pools with host placement (which are valid per tests infeaturegates_test.go) won't trigger the dedicated host permissions.The condition should skip only edge pools, not non-worker compute pools:
Suggested fix
func includesDedicatedHosts(installConfig *types.InstallConfig) bool { for _, mpool := range installConfig.Compute { - if mpool.Name != types.MachinePoolComputeRoleName { + if mpool.Name == types.MachinePoolEdgeRoleName { continue } if mpool.Platform.AWS != nil && mpool.Platform.AWS.HostPlacement != nil { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/permissions.go` around lines 793 - 805, includesDedicatedHosts only inspects pools named types.MachinePoolComputeRoleName ("worker"), so other compute pools with AWS.HostPlacement are ignored; change the logic in includesDedicatedHosts to skip only edge pools (check for types.MachinePoolEdgeRoleName) instead of skipping non-worker names, i.e., iterate all installConfig.Compute entries, continue when mpool.Name == types.MachinePoolEdgeRoleName, and keep the existing AWS/HostPlacement nil checks to return true when any non-edge pool specifies dedicated hosts.pkg/asset/installconfig/aws/validation.go (1)
569-581:⚠️ Potential issue | 🟠 MajorPotential AZ format mismatch when pool.Zones is empty.
When
pool.Zonesis empty,meta.AvailabilityZones(ctx)is called as a fallback. In this codebase, availability zones from metadata can be short form (a,b,c) whilefoundHost.Zonefrom AWS is full form (us-east-1a). Theslices.Containscheck at line 578 would fail even for valid hosts.Also, the error message at line 579 shows
pool.Zoneswhich may be empty when the fallback was used, making the error confusing.Suggested fix: Only validate when zones are explicitly configured
// Is host valid for pools region and zone config? // Only check if zones are explicitly configured; if pool.Zones is empty, all zones are allowed - zones := pool.Zones - if len(zones) == 0 { - zones, err = meta.AvailabilityZones(ctx) - if err != nil { - allErrs = append(allErrs, field.InternalError(fldPath, fmt.Errorf("unable to retrieve availability zones: %w", err))) - } - } - if !slices.Contains(zones, foundHost.Zone) { - errMsg := fmt.Sprintf("machine pool specifies zones %v but dedicated host %s is in zone %s", pool.Zones, host.ID, foundHost.Zone) + if len(pool.Zones) > 0 && !slices.Contains(pool.Zones, foundHost.Zone) { + errMsg := fmt.Sprintf("machine pool specifies zones %v but dedicated host %s is in zone %s", pool.Zones, host.ID, foundHost.Zone) allErrs = append(allErrs, field.Invalid(dhPath, host, errMsg)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/validation.go` around lines 569 - 581, The validation currently uses meta.AvailabilityZones(ctx) when pool.Zones is empty, causing mismatches between short AZs (a,b) and full zones (us-east-1a) and producing a confusing error that prints an empty pool.Zones; change the logic to only perform the dedicated-host zone membership check when pool.Zones is explicitly configured (i.e. len(pool.Zones) > 0) so that slices.Contains(pool.Zones, foundHost.Zone) is used only for user-provided zones, and update the field.Invalid call (dhPath, host, ...) to include the actual configured zones (pool.Zones) in the message rather than any metadata fallback.pkg/types/aws/machinepool.go (1)
238-239:⚠️ Potential issue | 🟡 MinorStale YAML path in doc comment.
The comment references
dedicatedHost.host.idbut the actual schema field isdedicatedHost[].id. This will propagate to generated docs.Suggested fix
- // HostAffinityDedicatedHost requires specifying a particular host via dedicatedHost.host.id. + // HostAffinityDedicatedHost requires specifying at least one host via dedicatedHost[].id. HostAffinityDedicatedHost HostAffinity = "DedicatedHost"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/machinepool.go` around lines 238 - 239, The doc comment for the HostAffinityDedicatedHost constant is using a stale YAML path; update the comment text that currently references "dedicatedHost.host.id" to the correct schema path "dedicatedHost[].id" so generated docs reflect the real field; locate the constant HostAffinityDedicatedHost (type HostAffinity, value "DedicatedHost") and edit its preceding comment accordingly.pkg/asset/machines/aws/machines.go (1)
370-382:⚠️ Potential issue | 🟠 MajorSilent fallback when no host matches the zone may mask configuration errors.
When
placement != nilbut no host inplacement.DedicatedHostmatches the requested zone, this function returns an empty string. This causes the provider to generate an instance without dedicated host placement, silently falling back to shared tenancy.For a pool explicitly configured with
Affinity == DedicatedHost, this could launch instances without the intended dedicated host isolation. While upstream validation should catch zone mismatches, consider returning an error (or at least a secondary boolean indicating whether a match was found) to surface configuration issues more explicitly.🔧 Suggested approach
Consider changing the signature to return
(string, bool)or(string, error)to indicate whether a valid host was found:-func DedicatedHost(hosts map[string]aws.Host, placement *awstypes.HostPlacement, zone string) string { +func DedicatedHost(hosts map[string]aws.Host, placement *awstypes.HostPlacement, zone string) (string, error) { if placement != nil { for _, host := range placement.DedicatedHost { hostDetails, found := hosts[host.ID] if found && hostDetails.Zone == zone { - return hostDetails.ID + return hostDetails.ID, nil } } + // Placement configured but no host matches zone + return "", fmt.Errorf("no dedicated host found for zone %s", zone) } - return "" + return "", nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/aws/machines.go` around lines 370 - 382, The DedicatedHost function currently returns an empty string on no-match which silently falls back to shared tenancy; change DedicatedHost(hosts map[string]aws.Host, placement *awstypes.HostPlacement, zone string) to return a result indicator (either (string, bool) or (string, error)) so callers can detect no-match. Update the function to return the matched host ID and true (or id, nil) when a host.Zone == zone is found, and to return ("", false) or ("", descriptiveError) when placement != nil but no host matches; then update all callers of DedicatedHost to handle the new boolean/error return and fail fast or surface a clear configuration error when a dedicated-affinity pool did not find a host.pkg/types/aws/validation/machinepool.go (1)
104-108:⚠️ Potential issue | 🟡 MinorMisleading error message for forbidden
dedicatedHostfield.When
AffinityisAnyAvailableandDedicatedHostentries are present, the error message says:"dedicatedHost is required when 'affinity' is set to DedicatedHost, and forbidden otherwise"
This message is more appropriate for the
DedicatedHostaffinity case (line 111). ForAnyAvailable, usefield.Forbiddenwith a clearer message:🐛 Proposed fix
case aws.HostAffinityAnyAvailable: if len(p.HostPlacement.DedicatedHost) > 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("dedicatedHost"), "dedicatedHost is required when 'affinity' is set to DedicatedHost, and forbidden otherwise")) + allErrs = append(allErrs, field.Forbidden(fldPath.Child("dedicatedHost"), "dedicatedHost must not be set when affinity is AnyAvailable")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/validation/machinepool.go` around lines 104 - 108, The current validation branch for aws.HostAffinityAnyAvailable incorrectly appends a field.Required error with a misleading message; update the branch handling p.HostPlacement.Affinity == aws.HostAffinityAnyAvailable to append field.Forbidden(fldPath.Child("dedicatedHost"), "<clear message>") instead, e.g., "dedicatedHost must not be set when affinity is AnyAvailable", so replace the field.Required call that references p.HostPlacement.DedicatedHost with field.Forbidden(...) and ensure the error targets fldPath.Child("dedicatedHost").
🧹 Nitpick comments (2)
pkg/types/aws/validation/machinepool_test.go (2)
382-391: Complex test defaulting logic could be simplified.The logic to determine
poolNamebased on error message content is fragile and harder to maintain. Consider explicitly settingpoolNamein each test case that needs a specific value, rather than inferring from the expected error string.♻️ Simpler approach
Set
poolName: types.MachinePoolComputeRoleNameexplicitly in test cases that require it, and remove the inference logic:for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { poolName := tc.poolName - // If poolName is not specified, default to worker pool unless the test - // is explicitly testing control plane or edge pool behavior - if poolName == "" && tc.expected != "" && - !strings.Contains(tc.expected, "control plane") && - !strings.Contains(tc.expected, "edge pool") { - poolName = types.MachinePoolComputeRoleName - } else if poolName == "" && tc.expected == "" { + if poolName == "" { poolName = types.MachinePoolComputeRoleName }Then explicitly set
poolName: ""for the control plane test case (already done on line 363).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/aws/validation/machinepool_test.go` around lines 382 - 391, The test's fragile inference for poolName should be removed and tests should explicitly set the poolName in their test cases; replace the conditional block that mutates poolName based on tc.expected and strings.Contains with no-op, and update each test case struct to include poolName: types.MachinePoolComputeRoleName where the compute/worker default is required and poolName: "" for control-plane/edge tests; this makes the setup deterministic and you can delete the logic that inspects tc.expected and strings.Contains.
300-323: Misleading test names suggest format validation that doesn't exist.These test cases are named "valid dedicated host - 8 character format" but use IDs like
h-1a2b3c4d(10 chars total) which don't match the AWS format requirement documented inpkg/types/aws/machinepool.go:
- Pattern:
^h-[0-9a-f]{17}$- Length: exactly 19 characters
The tests pass because
validateHostPlacementonly checkslen(host.ID) == 0and doesn't validate the format. The kubebuilder annotations provide CRD-level validation, but the Go validation is lenient.Consider either:
- Rename tests to clarify they test "non-empty ID" rather than "valid format"
- Add format validation in
validateHostPlacementto match the documented constraints♻️ Option 1: Rename tests to reflect actual validation
- { - name: "valid dedicated host - 8 character format", + { + name: "valid dedicated host - non-empty ID", pool: &aws.MachinePool{ HostPlacement: &aws.HostPlacement{ Affinity: ptr.To(aws.HostAffinityDedicatedHost), DedicatedHost: []aws.DedicatedHost{ - {ID: "h-1a2b3c4d"}, + {ID: "h-09dcf61cb388b0149"}, }, }, }, },
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/installconfig/aws/permissions.go`:
- Around line 581-583: RequiredPermissionGroups currently appends
PermissionDedicatedHosts for any HostPlacement via includesDedicatedHosts(ic)
but must also append PermissionDynamicHostAllocation only when the
HostPlacement's affinity is HostAffinityAnyAvailable (dynamically allocated
hosts that require ec2:ReleaseHosts). Update the logic in
RequiredPermissionGroups to inspect ic.Platform.AWS.HostPlacement (or the
function that reads HostPlacement used by includesDedicatedHosts) and: keep
appending PermissionDedicatedHosts when HostPlacement is present, and
additionally append PermissionDynamicHostAllocation only when
HostPlacement.Affinity == HostAffinityAnyAvailable; do not add
PermissionDynamicHostAllocation for HostAffinityDedicatedHost.
In `@pkg/types/aws/machinepool.go`:
- Around line 250-251: The ID field in the machine pool struct is marked
+required but its JSON tag uses omitempty; update the struct's ID field (the
symbol "ID" in the machine pool struct in machinepool.go) to remove omitempty so
the tag reads `json:"id"` (keep the `+required` comment intact) to match other
required fields like the one in platform.go.
---
Duplicate comments:
In `@pkg/asset/installconfig/aws/permissions.go`:
- Around line 793-805: includesDedicatedHosts only inspects pools named
types.MachinePoolComputeRoleName ("worker"), so other compute pools with
AWS.HostPlacement are ignored; change the logic in includesDedicatedHosts to
skip only edge pools (check for types.MachinePoolEdgeRoleName) instead of
skipping non-worker names, i.e., iterate all installConfig.Compute entries,
continue when mpool.Name == types.MachinePoolEdgeRoleName, and keep the existing
AWS/HostPlacement nil checks to return true when any non-edge pool specifies
dedicated hosts.
In `@pkg/asset/installconfig/aws/validation_test.go`:
- Around line 2431-2443: The helper withComputeHostPlacementAndZone currently
ignores its zone parameter; update it so that after setting HostPlacement it
also assigns the provided zone to the compute pool's AWS Zones (e.g., set
ic.Compute[index].Platform.AWS.Zones = []string{zone}) so tests that expect a
mismatch between pool zones and host zones are valid; alternatively, if the zone
is not needed, remove withComputeHostPlacementAndZone and use the existing
withComputeHostPlacement helper instead.
In `@pkg/asset/installconfig/aws/validation.go`:
- Around line 569-581: The validation currently uses meta.AvailabilityZones(ctx)
when pool.Zones is empty, causing mismatches between short AZs (a,b) and full
zones (us-east-1a) and producing a confusing error that prints an empty
pool.Zones; change the logic to only perform the dedicated-host zone membership
check when pool.Zones is explicitly configured (i.e. len(pool.Zones) > 0) so
that slices.Contains(pool.Zones, foundHost.Zone) is used only for user-provided
zones, and update the field.Invalid call (dhPath, host, ...) to include the
actual configured zones (pool.Zones) in the message rather than any metadata
fallback.
In `@pkg/asset/machines/aws/machines.go`:
- Around line 370-382: The DedicatedHost function currently returns an empty
string on no-match which silently falls back to shared tenancy; change
DedicatedHost(hosts map[string]aws.Host, placement *awstypes.HostPlacement, zone
string) to return a result indicator (either (string, bool) or (string, error))
so callers can detect no-match. Update the function to return the matched host
ID and true (or id, nil) when a host.Zone == zone is found, and to return ("",
false) or ("", descriptiveError) when placement != nil but no host matches; then
update all callers of DedicatedHost to handle the new boolean/error return and
fail fast or surface a clear configuration error when a dedicated-affinity pool
did not find a host.
In `@pkg/types/aws/machinepool.go`:
- Around line 238-239: The doc comment for the HostAffinityDedicatedHost
constant is using a stale YAML path; update the comment text that currently
references "dedicatedHost.host.id" to the correct schema path
"dedicatedHost[].id" so generated docs reflect the real field; locate the
constant HostAffinityDedicatedHost (type HostAffinity, value "DedicatedHost")
and edit its preceding comment accordingly.
In `@pkg/types/aws/validation/machinepool.go`:
- Around line 104-108: The current validation branch for
aws.HostAffinityAnyAvailable incorrectly appends a field.Required error with a
misleading message; update the branch handling p.HostPlacement.Affinity ==
aws.HostAffinityAnyAvailable to append
field.Forbidden(fldPath.Child("dedicatedHost"), "<clear message>") instead,
e.g., "dedicatedHost must not be set when affinity is AnyAvailable", so replace
the field.Required call that references p.HostPlacement.DedicatedHost with
field.Forbidden(...) and ensure the error targets
fldPath.Child("dedicatedHost").
---
Nitpick comments:
In `@pkg/types/aws/validation/machinepool_test.go`:
- Around line 382-391: The test's fragile inference for poolName should be
removed and tests should explicitly set the poolName in their test cases;
replace the conditional block that mutates poolName based on tc.expected and
strings.Contains with no-op, and update each test case struct to include
poolName: types.MachinePoolComputeRoleName where the compute/worker default is
required and poolName: "" for control-plane/edge tests; this makes the setup
deterministic and you can delete the logic that inspects tc.expected and
strings.Contains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7471aa8-4691-4293-aa40-f9992fca7013
⛔ Files ignored due to path filters (1)
pkg/types/aws/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (20)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/cluster/aws/aws.gopkg/asset/installconfig/aws/dedicatedhosts.gopkg/asset/installconfig/aws/metadata.gopkg/asset/installconfig/aws/permissions.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/aws/validation_test.gopkg/asset/machines/aws/machines.gopkg/asset/machines/aws/machinesets.gopkg/asset/machines/worker.gopkg/asset/machines/worker_test.gopkg/destroy/aws/ec2helpers.gopkg/types/aws/machinepool.gopkg/types/aws/validation/featuregates.gopkg/types/aws/validation/featuregates_test.gopkg/types/aws/validation/machinepool.gopkg/types/aws/validation/machinepool_test.gopkg/types/aws/validation/platform.gopkg/types/machinepools.gopkg/types/validation/machinepools.go
✅ Files skipped from review due to trivial changes (1)
- pkg/asset/installconfig/aws/metadata.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/asset/installconfig/aws/dedicatedhosts.go
- pkg/types/aws/validation/featuregates_test.go
- pkg/destroy/aws/ec2helpers.go
- pkg/types/aws/validation/featuregates.go
| if includesDedicatedHosts(ic) { | ||
| permissionGroups = append(permissionGroups, PermissionDedicatedHosts) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if PermissionDynamicHostAllocation is used elsewhere
rg -n "PermissionDynamicHostAllocation" --type goRepository: openshift/installer
Length of output: 473
🏁 Script executed:
#!/bin/bash
# Get context around line 442 where PermissionDynamicHostAllocation is used
sed -n '435,450p' pkg/asset/installconfig/aws/permissions.go
# Also check the PermissionDedicatedHosts definition
rg -n "PermissionDedicatedHosts" --type go -A 2 -B 2Repository: openshift/installer
Length of output: 2301
🏁 Script executed:
#!/bin/bash
# Check the includesDedicatedHosts function to understand what it checks
rg -n "func includesDedicatedHosts" --type go -A 10
# Check how RequiredPermissionGroups is used
rg -n "RequiredPermissionGroups" --type go -B 2 -A 2Repository: openshift/installer
Length of output: 31117
🏁 Script executed:
#!/bin/bash
# Search for dedicated hosts tests
rg -n "DedicatedHosts" pkg/asset/installconfig/aws/permissions_test.go -B 3 -A 10
# Also check if there's any logic about dynamic host allocation
rg -n "ReleaseHosts\|DynamicHostAllocation" pkg/asset/installconfig/aws/ -B 2 -A 2Repository: openshift/installer
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search more broadly for tests and usage of dedicated hosts
rg -n "includesDedicatedHosts" pkg/asset/installconfig/aws/ -B 2 -A 2
# Check the RequiredPermissionGroups function more carefully
sed -n '500,650p' pkg/asset/installconfig/aws/permissions.goRepository: openshift/installer
Length of output: 5696
🏁 Script executed:
#!/bin/bash
# Look for any logic about dynamic host allocation in the codebase
rg -n "dynamic.*host\|allocat.*host" --type go -i pkg/asset/installconfig/aws/
# Also search for any comments or code that explains the relationship
grep -rn "ReleaseHosts\|dynamic" pkg/asset/installconfig/aws/permissions.goRepository: openshift/installer
Length of output: 490
🏁 Script executed:
#!/bin/bash
# Search for any test files that might test dedicated hosts scenarios
find . -name "*test.go" -type f | xargs grep -l "DedicatedHosts\|dedicat" | head -5
# Check git history or comments around the PermissionDynamicHostAllocation to understand when it was added
rg -n "PermissionDynamicHostAllocation" pkg/asset/installconfig/aws/permissions.go -B 5 -A 5Repository: openshift/installer
Length of output: 1372
🏁 Script executed:
#!/bin/bash
# Check where RequiredPermissionGroups is used to confirm it's for both install and delete
rg -n "RequiredPermissionGroups" pkg/asset/ -B 1 -A 1 | head -40
# Also check if there's any code that allocates dynamic hosts
rg -n "allocat\|AllocateHosts" pkg/ --type go -i | head -20Repository: openshift/installer
Length of output: 3170
🏁 Script executed:
#!/bin/bash
# Search for any code that uses HostPlacement or allocates hosts
rg -n "HostPlacement\|allocat" pkg/asset/installconfig/aws/ --type go -i -B 2 -A 2
# Check if there are any tests that specifically test the dedicated hosts permission
rg -n "PermissionDedicatedHosts\|PermissionDynamicHostAllocation" pkg/asset/installconfig/aws/permissions_test.go -B 3 -A 3Repository: openshift/installer
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for dedicated hosts in validation tests
rg -n "HostPlacement" pkg/ --type go -B 2 -A 2 | head -60
# Also check if there's any documentation or comments about when dynamic hosts are allocated
rg -n "HostPlacement\|DedicatedHost" pkg/types/aws/ --type go -B 2 -A 2Repository: openshift/installer
Length of output: 4394
🏁 Script executed:
#!/bin/bash
# Check the HostPlacement struct definition to understand dedicated vs dynamic hosts
rg -n "type HostPlacement\|DedicatedHost\|Affinity" pkg/types/aws/machinepool.go -B 1 -A 5 | head -80
# Also check if there's any code that creates/releases dedicated hosts
rg -n "AllocateHosts\|ReleaseHosts" pkg/ --type go -B 2 -A 2Repository: openshift/installer
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Find the HostPlacement struct definition
find . -name "*.go" -type f | xargs grep -l "type HostPlacement struct" | head -3
# Then get its definition
rg -n "type HostPlacement struct" --type go -A 20
# Also search for what determines dynamic host allocation
rg -n "Affinity\|DedicatedHost" pkg/types/aws/ --type go | head -30Repository: openshift/installer
Length of output: 4548
🏁 Script executed:
#!/bin/bash
# Check what HostAffinity constants are defined
rg -n "HostAffinity\|AnyAvailable\|DedicatedHost" pkg/types/aws/machinepool.go | head -30
# Look for any code that handles destroy/cleanup of hosts
rg -n "destroy\|cleanup\|release" pkg/asset/installconfig/aws/permissions.go -i -B 2 -A 2Repository: openshift/installer
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check the HostAffinity constants
rg -n "const (" pkg/types/aws/machinepool.go -A 20 | grep -E "Affinity|Host|const"
# Also check the comment about when dynamic hosts are allocated in permissions.go
sed -n '438,445p' pkg/asset/installconfig/aws/permissions.go
# Let me check if there's any PR or documentation about this
git log --oneline -n 20 -- pkg/asset/installconfig/aws/permissions.go 2>/dev/null | head -10Repository: openshift/installer
Length of output: 591
🏁 Script executed:
#!/bin/bash
# Check the HostAffinity constants more carefully
sed -n '230,245p' pkg/types/aws/machinepool.go
# Also check the git log more carefully
git log --oneline -n 30 -- pkg/asset/installconfig/aws/permissions.go 2>/dev/nullRepository: openshift/installer
Length of output: 857
🏁 Script executed:
#!/bin/bash
# Now I understand the distinction - let me verify the logic
# Check if includesDedicatedHosts distinguishes between AnyAvailable and DedicatedHost
sed -n '794,806p' pkg/asset/installconfig/aws/permissions.go
# Also search for any other code that adds PermissionDynamicHostAllocation
rg -n "PermissionDynamicHostAllocation" pkg/asset/installconfig/aws/permissions.goRepository: openshift/installer
Length of output: 742
🏁 Script executed:
#!/bin/bash
# Final verification - check if there's a way to distinguish between affinity types in RequiredPermissionGroups
# or if the permissions are intentionally grouped together
rg -n "AnyAvailable\|HostAffinityAnyAvailable" pkg/asset/installconfig/aws/ --type go
# Also check if there are any recent changes or issues related to this
git log --all --grep="DynamicHostAllocation\|ReleaseHosts" --oneline 2>/dev/null | head -10Repository: openshift/installer
Length of output: 45
Missing PermissionDynamicHostAllocation for dynamic host cleanup at destroy time, but the suggested fix needs refinement.
RequiredPermissionGroups is used for both install and delete credential validation. When dedicated hosts are configured with AnyAvailable affinity, the system dynamically allocates hosts that must be released at destroy time via ec2:ReleaseHosts. However, the current code only appends PermissionDedicatedHosts.
The suggested fix requires refinement: includesDedicatedHosts() returns true for any HostPlacement configuration but doesn't distinguish between HostAffinityAnyAvailable (dynamically allocated, requiring release) and HostAffinityDedicatedHost (user-provided, no release needed). Simply appending PermissionDynamicHostAllocation whenever HostPlacement != nil would add unnecessary permissions for the DedicatedHost affinity case.
The fix should conditionally add PermissionDynamicHostAllocation only when affinity is set to AnyAvailable, not for all dedicated host placements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/installconfig/aws/permissions.go` around lines 581 - 583,
RequiredPermissionGroups currently appends PermissionDedicatedHosts for any
HostPlacement via includesDedicatedHosts(ic) but must also append
PermissionDynamicHostAllocation only when the HostPlacement's affinity is
HostAffinityAnyAvailable (dynamically allocated hosts that require
ec2:ReleaseHosts). Update the logic in RequiredPermissionGroups to inspect
ic.Platform.AWS.HostPlacement (or the function that reads HostPlacement used by
includesDedicatedHosts) and: keep appending PermissionDedicatedHosts when
HostPlacement is present, and additionally append
PermissionDynamicHostAllocation only when HostPlacement.Affinity ==
HostAffinityAnyAvailable; do not add PermissionDynamicHostAllocation for
HostAffinityDedicatedHost.
| // +required | ||
| ID string `json:"id,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other required fields in the codebase use omitempty
rg -n '\+required' -A1 --type go pkg/types/aws/ | grep -E 'omitempty|json:'Repository: openshift/installer
Length of output: 251
🏁 Script executed:
sed -n '248,253p' pkg/types/aws/machinepool.goRepository: openshift/installer
Length of output: 193
Remove omitempty from the required ID field's JSON tag.
The ID field is annotated with +required but includes omitempty in the JSON tag, which is contradictory—a required field should not be omitted when empty. This also inconsistent with other required fields in the same package (e.g., in platform.go), which use json:"id" without omitempty.
Suggested fix
// +required
- ID string `json:"id,omitempty"`
+ ID string `json:"id"`📝 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.
| // +required | |
| ID string `json:"id,omitempty"` | |
| // +required | |
| ID string `json:"id"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/types/aws/machinepool.go` around lines 250 - 251, The ID field in the
machine pool struct is marked +required but its JSON tag uses omitempty; update
the struct's ID field (the symbol "ID" in the machine pool struct in
machinepool.go) to remove omitempty so the tag reads `json:"id"` (keep the
`+required` comment intact) to match other required fields like the one in
platform.go.
|
@vr4manta: 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. |
6 participants
SPLAT-2172
Changes
Dependencies
Notes
MAO and CAO changes are needed for it to fully work. For now, this PR is adding the ability to generate the needed outputs for bootstrapping.
Summary by CodeRabbit
New Features
Improvements
Tests