Skip to content

Commit c30411a

Browse files
linoyaslantsorya
linoyaslan
authored and
tsorya
committed
NVIDIA-477: Add ownership validation before deleting resources
1 parent 9abb138 commit c30411a

6 files changed

Lines changed: 108 additions & 59 deletions

File tree

‎internal/common/ownership.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/*
2+
Copyright 2025.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package common
18+
19+
// IsOwnedByProvisioner checks if a resource's labels indicate ownership by the given DPFHCPProvisioner.
20+
// Used for cross-namespace resources where OwnerReferences cannot be used.
21+
func IsOwnedByProvisioner(labels map[string]string, provisionerName, provisionerNamespace string) bool {
22+
if len(labels) == 0 {
23+
return false
24+
}
25+
return labels[LabelDPFHCPProvisionerName] == provisionerName &&
26+
labels[LabelDPFHCPProvisionerNamespace] == provisionerNamespace
27+
}

‎internal/controller/hostedcluster/cleanup_handler.go‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
corev1 "k8s.io/api/core/v1"
2626
apierrors "k8s.io/apimachinery/pkg/api/errors"
2727
"k8s.io/apimachinery/pkg/api/meta"
28+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2829
"k8s.io/apimachinery/pkg/types"
2930
"k8s.io/client-go/tools/record"
3031
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -163,7 +164,15 @@ func (h *CleanupHandler) deleteResource(
163164
return false, fmt.Errorf("failed to get %s: %w", resourceKind, err)
164165
}
165166

166-
// Resource still exists
167+
// Verify ownership before deleting
168+
if !metav1.IsControlledBy(obj, cr) {
169+
log.Info(fmt.Sprintf("Skipping %s deletion - not owned by this DPFHCPProvisioner", resourceKind),
170+
resourceKind, key.Name,
171+
"namespace", key.Namespace)
172+
return true, nil
173+
}
174+
175+
// Resource still exists and is owned by us
167176
deletionTimestamp := obj.GetDeletionTimestamp()
168177
if deletionTimestamp == nil {
169178
// Resource not yet marked for deletion, delete it now
@@ -205,7 +214,7 @@ func (h *CleanupHandler) deleteSecrets(ctx context.Context, cr *provisioningv1al
205214
}
206215

207216
for _, secretName := range secretNames {
208-
if err := h.deleteSecret(ctx, cr.Namespace, secretName); err != nil {
217+
if err := h.deleteSecret(ctx, cr, secretName); err != nil {
209218
log.Error(err, "Failed to delete secret",
210219
"secret", secretName,
211220
"namespace", cr.Namespace)
@@ -220,14 +229,14 @@ func (h *CleanupHandler) deleteSecrets(ctx context.Context, cr *provisioningv1al
220229
return nil
221230
}
222231

223-
// deleteSecret deletes a single secret
224-
func (h *CleanupHandler) deleteSecret(ctx context.Context, namespace, secretName string) error {
232+
// deleteSecret deletes a single secret after verifying ownership
233+
func (h *CleanupHandler) deleteSecret(ctx context.Context, cr *provisioningv1alpha1.DPFHCPProvisioner, secretName string) error {
225234
log := logf.FromContext(ctx)
226235

227236
secret := &corev1.Secret{}
228237
secretKey := types.NamespacedName{
229238
Name: secretName,
230-
Namespace: namespace,
239+
Namespace: cr.Namespace,
231240
}
232241

233242
err := h.client.Get(ctx, secretKey, secret)
@@ -236,16 +245,24 @@ func (h *CleanupHandler) deleteSecret(ctx context.Context, namespace, secretName
236245
// Secret already deleted
237246
log.V(1).Info("Secret already deleted",
238247
"secret", secretName,
239-
"namespace", namespace)
248+
"namespace", cr.Namespace)
240249
return nil
241250
}
242251
return fmt.Errorf("failed to get secret %s: %w", secretName, err)
243252
}
244253

254+
// Verify ownership before deleting
255+
if !metav1.IsControlledBy(secret, cr) {
256+
log.Info("Skipping secret deletion - not owned by this DPFHCPProvisioner",
257+
"secret", secretName,
258+
"namespace", cr.Namespace)
259+
return nil
260+
}
261+
245262
// Delete secret
246263
log.V(1).Info("Deleting secret",
247264
"secret", secretName,
248-
"namespace", namespace)
265+
"namespace", cr.Namespace)
249266

250267
if err := h.client.Delete(ctx, secret); err != nil {
251268
if apierrors.IsNotFound(err) {
@@ -257,7 +274,7 @@ func (h *CleanupHandler) deleteSecret(ctx context.Context, namespace, secretName
257274

258275
log.Info("Secret deleted successfully",
259276
"secret", secretName,
260-
"namespace", namespace)
277+
"namespace", cr.Namespace)
261278

262279
return nil
263280
}

‎internal/controller/kubeconfiginjection/cleanup_handler.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ func (h *CleanupHandler) Cleanup(ctx context.Context, cr *provisioningv1alpha1.D
9393
return nil
9494
}
9595

96-
// Delete found secrets
96+
// Delete found secrets (ownership verified via label selector above)
9797
deletedCount := 0
9898
for i := range secretList.Items {
9999
secret := &secretList.Items[i]

‎internal/controller/metallb/cleanup_handler.go‎

Lines changed: 37 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -88,23 +88,27 @@ func (h *CleanupHandler) Cleanup(ctx context.Context, cr *provisioningv1alpha1.D
8888
return fmt.Errorf("getting IPAddressPool: %w", err)
8989
}
9090
} else {
91-
// Resource still exists, initiate deletion
92-
log.Info("Deleting IPAddressPool",
93-
"name", pool.Name,
94-
"namespace", pool.Namespace)
95-
96-
if err := h.client.Delete(ctx, pool); err != nil {
97-
if !apierrors.IsNotFound(err) {
98-
// Deletion failed with non-NotFound error
99-
log.Error(err, "Failed to delete IPAddressPool")
100-
return fmt.Errorf("deleting IPAddressPool: %w", err)
91+
// Verify ownership via labels before deleting
92+
if !common.IsOwnedByProvisioner(pool.Labels, cr.Name, cr.Namespace) {
93+
log.Info("Skipping IPAddressPool deletion - not owned by this DPFHCPProvisioner",
94+
"name", pool.Name,
95+
"namespace", pool.Namespace)
96+
} else {
97+
// Resource still exists and is owned by us, initiate deletion
98+
log.Info("Deleting IPAddressPool",
99+
"name", pool.Name,
100+
"namespace", pool.Namespace)
101+
102+
if err := h.client.Delete(ctx, pool); err != nil {
103+
if !apierrors.IsNotFound(err) {
104+
log.Error(err, "Failed to delete IPAddressPool")
105+
return fmt.Errorf("deleting IPAddressPool: %w", err)
106+
}
101107
}
102-
// Resource was deleted between GET and DELETE (race condition - this is OK)
103-
}
104108

105-
// Resource deletion initiated, wait for it to complete
106-
log.V(1).Info("Waiting for IPAddressPool deletion to complete")
107-
return fmt.Errorf("waiting for IPAddressPool deletion")
109+
log.V(1).Info("Waiting for IPAddressPool deletion to complete")
110+
return fmt.Errorf("waiting for IPAddressPool deletion")
111+
}
108112
}
109113

110114
// Step 2: Delete L2Advertisement
@@ -115,32 +119,33 @@ func (h *CleanupHandler) Cleanup(ctx context.Context, cr *provisioningv1alpha1.D
115119
}, advert)
116120

117121
if err != nil {
118-
// Check if resource was already deleted (NotFound is success)
119122
if apierrors.IsNotFound(err) {
120123
log.V(1).Info("L2Advertisement already deleted or never existed")
121124
} else {
122-
// Unexpected error (permission denied, network issue, etc.)
123125
log.Error(err, "Failed to get L2Advertisement")
124126
return fmt.Errorf("getting L2Advertisement: %w", err)
125127
}
126128
} else {
127-
// Resource still exists, initiate deletion
128-
log.Info("Deleting L2Advertisement",
129-
"name", advert.Name,
130-
"namespace", advert.Namespace)
131-
132-
if err := h.client.Delete(ctx, advert); err != nil {
133-
if !apierrors.IsNotFound(err) {
134-
// Deletion failed with non-NotFound error
135-
log.Error(err, "Failed to delete L2Advertisement")
136-
return fmt.Errorf("deleting L2Advertisement: %w", err)
129+
// Verify ownership via labels before deleting
130+
if !common.IsOwnedByProvisioner(advert.Labels, cr.Name, cr.Namespace) {
131+
log.Info("Skipping L2Advertisement deletion - not owned by this DPFHCPProvisioner",
132+
"name", advert.Name,
133+
"namespace", advert.Namespace)
134+
} else {
135+
log.Info("Deleting L2Advertisement",
136+
"name", advert.Name,
137+
"namespace", advert.Namespace)
138+
139+
if err := h.client.Delete(ctx, advert); err != nil {
140+
if !apierrors.IsNotFound(err) {
141+
log.Error(err, "Failed to delete L2Advertisement")
142+
return fmt.Errorf("deleting L2Advertisement: %w", err)
143+
}
137144
}
138-
// Resource was deleted between GET and DELETE (race condition - this is OK)
139-
}
140145

141-
// Resource deletion initiated, wait for it to complete
142-
log.V(1).Info("Waiting for L2Advertisement deletion to complete")
143-
return fmt.Errorf("waiting for L2Advertisement deletion")
146+
log.V(1).Info("Waiting for L2Advertisement deletion to complete")
147+
return fmt.Errorf("waiting for L2Advertisement deletion")
148+
}
144149
}
145150

146151
// All resources cleaned up successfully

‎internal/controller/metallb/metallb.go‎

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ func (m *MetalLBManager) ensureIPAddressPool(ctx context.Context, provisioner *p
151151

152152
op, err := controllerutil.CreateOrUpdate(ctx, m.client, pool, func() error {
153153
// Verify ownership if resource already exists (ResourceVersion is set by API server on existing resources)
154-
if pool.ResourceVersion != "" && !m.isOwnedByProvisioner(pool, provisioner) {
154+
if pool.ResourceVersion != "" && !common.IsOwnedByProvisioner(pool.Labels, provisioner.Name, provisioner.Namespace) {
155155
return fmt.Errorf("IPAddressPool %s/%s exists but is not owned by DPFHCPProvisioner %s/%s (missing ownership labels)",
156156
pool.Namespace, pool.Name, provisioner.Namespace, provisioner.Name)
157157
}
@@ -211,7 +211,7 @@ func (m *MetalLBManager) ensureL2Advertisement(ctx context.Context, provisioner
211211

212212
op, err := controllerutil.CreateOrUpdate(ctx, m.client, advert, func() error {
213213
// Verify ownership if resource already exists (ResourceVersion is set by API server on existing resources)
214-
if advert.ResourceVersion != "" && !m.isOwnedByProvisioner(advert, provisioner) {
214+
if advert.ResourceVersion != "" && !common.IsOwnedByProvisioner(advert.Labels, provisioner.Name, provisioner.Namespace) {
215215
return fmt.Errorf("L2Advertisement %s/%s exists but is not owned by DPFHCPProvisioner %s/%s (missing ownership labels)",
216216
advert.Namespace, advert.Name, provisioner.Namespace, provisioner.Name)
217217
}
@@ -249,19 +249,3 @@ func (m *MetalLBManager) ensureL2Advertisement(ctx context.Context, provisioner
249249

250250
return nil
251251
}
252-
253-
// isOwnedByProvisioner checks if a resource has the correct ownership labels for the given DPFHCPProvisioner.
254-
// This prevents taking ownership of resources created by other operators or users.
255-
func (m *MetalLBManager) isOwnedByProvisioner(obj client.Object, provisioner *provisioningv1alpha1.DPFHCPProvisioner) bool {
256-
labels := obj.GetLabels()
257-
if labels == nil {
258-
return false
259-
}
260-
261-
provisionerName, hasProvisionerName := labels[common.LabelDPFHCPProvisionerName]
262-
provisionerNamespace, hasProvisionerNamespace := labels[common.LabelDPFHCPProvisionerNamespace]
263-
264-
return hasProvisionerName && hasProvisionerNamespace &&
265-
provisionerName == provisioner.Name &&
266-
provisionerNamespace == provisioner.Namespace
267-
}

‎internal/controller/metallb/metallb_test.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,10 @@ var _ = Describe("MetalLB Manager", func() {
390390
ObjectMeta: metav1.ObjectMeta{
391391
Name: "test-provisioner",
392392
Namespace: common.OpenshiftOperatorsNamespace,
393+
Labels: map[string]string{
394+
common.LabelDPFHCPProvisionerName: "test-provisioner",
395+
common.LabelDPFHCPProvisionerNamespace: "test-ns",
396+
},
393397
},
394398
Spec: metallbv1beta1.IPAddressPoolSpec{
395399
Addresses: []string{"192.168.1.100/32"},
@@ -400,6 +404,10 @@ var _ = Describe("MetalLB Manager", func() {
400404
ObjectMeta: metav1.ObjectMeta{
401405
Name: "advertise-test-provisioner",
402406
Namespace: common.OpenshiftOperatorsNamespace,
407+
Labels: map[string]string{
408+
common.LabelDPFHCPProvisionerName: "test-provisioner",
409+
common.LabelDPFHCPProvisionerNamespace: "test-ns",
410+
},
403411
},
404412
Spec: metallbv1beta1.L2AdvertisementSpec{
405413
IPAddressPools: []string{"test-provisioner"},
@@ -477,6 +485,10 @@ var _ = Describe("MetalLB Manager", func() {
477485
ObjectMeta: metav1.ObjectMeta{
478486
Name: "test-provisioner",
479487
Namespace: common.OpenshiftOperatorsNamespace,
488+
Labels: map[string]string{
489+
common.LabelDPFHCPProvisionerName: "test-provisioner",
490+
common.LabelDPFHCPProvisionerNamespace: "test-ns",
491+
},
480492
},
481493
}
482494

@@ -516,6 +528,10 @@ var _ = Describe("MetalLB Manager", func() {
516528
ObjectMeta: metav1.ObjectMeta{
517529
Name: "advertise-test-provisioner",
518530
Namespace: common.OpenshiftOperatorsNamespace,
531+
Labels: map[string]string{
532+
common.LabelDPFHCPProvisionerName: "test-provisioner",
533+
common.LabelDPFHCPProvisionerNamespace: "test-ns",
534+
},
519535
},
520536
}
521537

0 commit comments

Comments
 (0)