Skip to content

NO-ISSUE: Change BareMetalHostRef's type to LocalObjectReference #827

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
giladravid16 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

NO-ISSUE: Change BareMetalHostRef's type to LocalObjectReference #827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions api/v1alpha1/imageclusterinstall_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ type ImageClusterInstallSpec struct {

// BareMetalHostRef identifies a BareMetalHost object to be used to attach the configuration to the host.
// +optional
BareMetalHostRef *BareMetalHostReference `json:"bareMetalHostRef,omitempty"`
BareMetalHostRef *corev1.LocalObjectReference `json:"bareMetalHostRef,omitempty"`
Comment on lines 90 to +92
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

This silently changes the meaning of existing CRs that used a different BMH namespace.

Before this change, persisted spec.bareMetalHostRef.namespace values were honored; after this change the controller resolves the BMH from ici.Namespace instead. Any existing ImageClusterInstall that referenced a host in another namespace will now either stop finding it or, worse, resolve a same-named BMH in the ICI namespace. This needs an explicit migration/validation story before rollout.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@api/v1alpha1/imageclusterinstall_types.go` around lines 90 - 92, The change
removed namespace information from the BareMetalHostRef causing CRs that
referenced BareMetalHosts in other namespaces to be misresolved; revert to an
explicit namespaced reference and add validation: change the BareMetalHostRef
field type from corev1.LocalObjectReference to corev1.ObjectReference (so it
includes Name and Namespace) in the ImageClusterInstall spec, update the field
comment to require Namespace when referencing cross-namespace BMHs, and
add/adjust the ImageClusterInstall admission validation
(ValidateCreate/ValidateUpdate) to reject empty or ambiguous namespaces (or
explicitly require a migration step) so existing persisted CRs keep their
intended target rather than silently resolving to ici.Namespace.


// MachineNetwork is the subnet provided by user for the ocp cluster.
// This will be used to create the node network and choose ip address for the node.
Expand Down Expand Up @@ -125,19 +125,12 @@ type ImageClusterInstallStatus struct {
// InstallRestarts is the total count of container restarts on the clusters install job.
InstallRestarts int `json:"installRestarts,omitempty"`

BareMetalHostRef *BareMetalHostReference `json:"bareMetalHostRef,omitempty"`
BareMetalHostRef *corev1.LocalObjectReference `json:"bareMetalHostRef,omitempty"`

// BootTime indicates the time at which the host was requested to boot. Used to determine install timeouts.
BootTime metav1.Time `json:"bootTime,omitempty"`
}

type BareMetalHostReference struct {
// Name identifies the BareMetalHost within a namespace
Name string `json:"name"`
// Namespace identifies the namespace containing the referenced BareMetalHost
Namespace string `json:"namespace"`
}

//+kubebuilder:object:root=true
//+kubebuilder:subresource:status
// +kubebuilder:resource:path=imageclusterinstalls,shortName=ici
Expand Down
45 changes: 18 additions & 27 deletions api/v1alpha1/imageclusterinstall_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,8 @@ var _ = Describe("ValidateUpdate", func() {
Expect(err.Error()).To(ContainSubstring("invalid hostname"))
})
It("update fail when installation started", func() {
bareMetalHostRef := &BareMetalHostReference{
Name: "test-bmh",
Namespace: "test-bmh-namespace",
bareMetalHostRef := &corev1.LocalObjectReference{
Name: "test-bmh",
}

oldClusterInstall := &ImageClusterInstall{
Expand Down Expand Up @@ -285,9 +284,8 @@ var _ = Describe("ValidateUpdate", func() {
},
}
newClusterInstall := oldClusterInstall.DeepCopy()
newClusterInstall.Spec.BareMetalHostRef = &BareMetalHostReference{
Name: "test-bmh",
Namespace: "test-bmh-namespace",
newClusterInstall.Spec.BareMetalHostRef = &corev1.LocalObjectReference{
Name: "test-bmh",
}

warns, err := newClusterInstall.ValidateUpdate(oldClusterInstall)
Expand All @@ -296,9 +294,8 @@ var _ = Describe("ValidateUpdate", func() {
})

It("update fails BMH ref update when installation started", func() {
bareMetalHostRef := &BareMetalHostReference{
Name: "test-bmh",
Namespace: "test-bmh-namespace",
bareMetalHostRef := &corev1.LocalObjectReference{
Name: "test-bmh",
}
oldClusterInstall := &ImageClusterInstall{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -329,16 +326,14 @@ var _ = Describe("ValidateUpdate", func() {
},
Spec: ImageClusterInstallSpec{
Hostname: "test",
BareMetalHostRef: &BareMetalHostReference{
Name: "test-bmh",
Namespace: "test-bmh-namespace",
BareMetalHostRef: &corev1.LocalObjectReference{
Name: "test-bmh",
},
},
}
newClusterInstall := oldClusterInstall.DeepCopy()
newClusterInstall.Spec.BareMetalHostRef = &BareMetalHostReference{
Name: "other-bmh",
Namespace: "test-bmh-namespace",
newClusterInstall.Spec.BareMetalHostRef = &corev1.LocalObjectReference{
Name: "other-bmh",
}

warns, err := newClusterInstall.ValidateUpdate(oldClusterInstall)
Expand All @@ -354,9 +349,8 @@ var _ = Describe("ValidateUpdate", func() {
},
Spec: ImageClusterInstallSpec{
Hostname: "test",
BareMetalHostRef: &BareMetalHostReference{
Name: "test-bmh",
Namespace: "test-bmh-namespace",
BareMetalHostRef: &corev1.LocalObjectReference{
Name: "test-bmh",
},
},
Status: ImageClusterInstallStatus{
Expand Down Expand Up @@ -384,9 +378,8 @@ var _ = Describe("ValidateUpdate", func() {
},
Spec: ImageClusterInstallSpec{
Hostname: "test",
BareMetalHostRef: &BareMetalHostReference{
Name: "test-bmh",
Namespace: "test-bmh-namespace",
BareMetalHostRef: &corev1.LocalObjectReference{
Name: "test-bmh",
},
},
Status: ImageClusterInstallStatus{
Expand All @@ -402,9 +395,8 @@ var _ = Describe("ValidateUpdate", func() {
})

It("fail status and spec update when installation started", func() {
bareMetalHostRef := &BareMetalHostReference{
Name: "test-bmh",
Namespace: "test-bmh-namespace",
bareMetalHostRef := &corev1.LocalObjectReference{
Name: "test-bmh",
}

oldClusterInstall := &ImageClusterInstall{
Expand Down Expand Up @@ -442,9 +434,8 @@ var _ = Describe("ValidateUpdate", func() {
},
Spec: ImageClusterInstallSpec{
Hostname: "test",
BareMetalHostRef: &BareMetalHostReference{
Name: "test-bmh",
Namespace: "test-bmh-namespace",
BareMetalHostRef: &corev1.LocalObjectReference{
Name: "test-bmh",
},
},
Status: ImageClusterInstallStatus{
Expand Down
19 changes: 2 additions & 17 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 19 additions & 16 deletions bundle/manifests/extensions.hive.openshift.io_imageclusterinstalls.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,16 @@ spec:
be used to attach the configuration to the host.
properties:
name:
description: Name identifies the BareMetalHost within a namespace
type: string
namespace:
description: Namespace identifies the namespace containing the
referenced BareMetalHost
default: ""
description: |-
Name of the referent.
This field is effectively required, but due to backwards compatibility is
allowed to be empty. Instances of this type with an empty value here are
almost certainly wrong.
More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
type: string
required:
- name
- namespace
type: object
x-kubernetes-map-type: atomic
caBundleRef:
description: |-
CABundle is a reference to a config map containing the new bundle of trusted certificates for the host.
Expand Down Expand Up @@ -365,18 +365,21 @@ spec:
description: ImageClusterInstallStatus defines the observed state of ImageClusterInstall
properties:
bareMetalHostRef:
description: |-
LocalObjectReference contains enough information to let you locate the
referenced object inside the same namespace.
properties:
name:
description: Name identifies the BareMetalHost within a namespace
type: string
namespace:
description: Namespace identifies the namespace containing the
referenced BareMetalHost
default: ""
description: |-
Name of the referent.
This field is effectively required, but due to backwards compatibility is
allowed to be empty. Instances of this type with an empty value here are
almost certainly wrong.
More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
type: string
required:
- name
- namespace
type: object
x-kubernetes-map-type: atomic
bootTime:
description: BootTime indicates the time at which the host was requested
to boot. Used to determine install timeouts.
Expand Down
5 changes: 2 additions & 3 deletions bundle/manifests/image-based-install-operator.clusterserviceversion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ metadata:
},
"spec": {
"bareMetalHostRef": {
"name": "host-0",
"namespace": "test-sno"
"name": "host-0"
},
"clusterDeploymentRef": {
"name": "test-sno"
Expand All @@ -36,7 +35,7 @@ metadata:
}
]
capabilities: Basic Install
createdAt: "2026-04-26T07:25:00Z"
createdAt: "2026-04-30T14:01:51Z"
operators.operatorframework.io/builder: operator-sdk-v1.30.0
operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
name: image-based-install-operator.v0.0.1
Expand Down
35 changes: 19 additions & 16 deletions config/crd/bases/extensions.hive.openshift.io_imageclusterinstalls.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,16 @@ spec:
be used to attach the configuration to the host.
properties:
name:
description: Name identifies the BareMetalHost within a namespace
type: string
namespace:
description: Namespace identifies the namespace containing the
referenced BareMetalHost
default: ""
description: |-
Name of the referent.
This field is effectively required, but due to backwards compatibility is
allowed to be empty. Instances of this type with an empty value here are
almost certainly wrong.
More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
type: string
required:
- name
- namespace
type: object
x-kubernetes-map-type: atomic
caBundleRef:
description: |-
CABundle is a reference to a config map containing the new bundle of trusted certificates for the host.
Expand Down Expand Up @@ -363,18 +363,21 @@ spec:
description: ImageClusterInstallStatus defines the observed state of ImageClusterInstall
properties:
bareMetalHostRef:
description: |-
LocalObjectReference contains enough information to let you locate the
referenced object inside the same namespace.
properties:
name:
description: Name identifies the BareMetalHost within a namespace
type: string
namespace:
description: Namespace identifies the namespace containing the
referenced BareMetalHost
default: ""
description: |-
Name of the referent.
This field is effectively required, but due to backwards compatibility is
allowed to be empty. Instances of this type with an empty value here are
almost certainly wrong.
More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
type: string
required:
- name
- namespace
type: object
x-kubernetes-map-type: atomic
bootTime:
description: BootTime indicates the time at which the host was requested
to boot. Used to determine install timeouts.
Expand Down
1 change: 0 additions & 1 deletion config/samples/extensions_v1alpha1_imageclusterinstall.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ spec:
name: 4.14.10
bareMetalHostRef:
name: host-0
namespace: test-sno
machineNetworks:
- cidr: "192.0.2.0/24"
- cidr: "2001:db8::/64"
Loading