Add deletion validation for IPPool webhook #137

haijianyang · 2022-09-15T16:34:15Z

The webhook should reject IPPool deletion if any IP has been allocated

Fixes #135

metal3-io-bot · 2022-09-15T16:34:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign furkatgofurov7 after the PR has been reviewed.
You can assign the PR to them by writing /assign @furkatgofurov7 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

metal3-io-bot · 2022-09-15T16:34:24Z

Hi @haijianyang. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

haijianyang · 2022-09-16T01:51:15Z

@furkatgofurov7

mboukhalfa · 2022-09-16T09:26:50Z

/ok-to-test

haijianyang · 2022-09-21T09:17:55Z

@furkatgofurov7
@mboukhalfa

furkatgofurov7 · 2022-09-23T07:28:14Z

/test-ubuntu-integration-main

jessehu · 2022-09-23T10:00:39Z

test case needs to be updated as well.
admission webhook \\\"validation.ippool.ipam.metal3.io\\\" denied the request: IPPool.ipam.metal3.io \\\"provisioning-pool\\\" is forbidden: IPPool cannot be deleted because it is in use\"", "Deleting IPPool=\"provisioning-pool\" Namespace=\"metal3\"", "Deleting Metal3MachineTemplate=\"test1-controlplane\" Namespace=\"metal3\"", "Deleting Metal3Cluster=\"test1\" Namespace=\"metal3\"", "Deleting Metal3DataTemplate=\"test1-controlplane-template\" Namespace=\"metal3\"", "Deleting Metal3MachineTemplate=\"test1-workers\" Namespace=\"metal3\"", "Deleting KubeadmControlPlane=\"test1\" Namespace=\"metal3\"", "Deleting Metal3DataTemplate=\"test1-workers-template\" Namespace=\"metal3\"", "Error: [action failed after 10 attempts: error deleting \"ipam.metal3.io/v1alpha1, Kind=IPPool\" metal3/baremetalv4-pool: admission webhook \"validation.ippool.ipam.metal3.io\" denied the request: IPPool.ipam.metal3.io \"baremetalv4-pool\" is forbidden: IPPool cannot be deleted because it is in use, action failed after 10 attempts: error deleting \"ipam.metal3.io/v1alpha1, Kind=IPPool\" metal3/provisioning-pool: admission webhook \"validation.ippool.ipam.metal3.io\" denied the request: IPPool.ipam.metal3.io \"provisioning-pool\" is forbidden: IPPool cannot be deleted because it is in use]"], "stdout": "", "stdout_lines": []}

haijianyang · 2022-09-27T02:19:03Z

E2E tests of cluster-api-provider-metal3 use clusterName to bind IPPool, IPPoolManager will set CAPI cluster as the owner of IPPool. So when CAPI cluster was deleted, kubernetes will try to delete the bound IPPool (provisioning-pool and baremetalv4-pool). However, the resources associated with the IPPool have not been released, so the ValidateDelete
webhook is blocked.

The use of clusterName may cause the CAPI cluster and IPPool to depend on each other. Do we have any solutions.

# IPPools of cluster-api-provider-metal3 e2e test
---
apiVersion: ipam.metal3.io/v1alpha1
kind: IPPool
metadata:
  name: provisioning-pool
  namespace: ${NAMESPACE}
spec:
  clusterName: ${CLUSTER_NAME}
  namePrefix: ${CLUSTER_NAME}-prov
  pools:
    - start: ${PROVISIONING_POOL_RANGE_START}
      end: ${PROVISIONING_POOL_RANGE_END}
  prefix: ${PROVISIONING_CIDR}
---
apiVersion: ipam.metal3.io/v1alpha1
kind: IPPool
metadata:
  name: baremetalv4-pool
  namespace: ${NAMESPACE}
spec:
  clusterName: ${CLUSTER_NAME}
  namePrefix: ${CLUSTER_NAME}-bmv4
  pools:
    - start: ${BAREMETALV4_POOL_RANGE_START}
      end: ${BAREMETALV4_POOL_RANGE_END}
  prefix: ${EXTERNAL_SUBNET_V4_PREFIX}
  gateway: ${EXTERNAL_SUBNET_V4_HOST}

// ip-address-manager
func (m *IPPoolManager) SetClusterOwnerRef(cluster *clusterv1.Cluster) error {
	if cluster == nil {
		return errors.New("Missing cluster")
	}
	// Verify that the owner reference is there, if not add it and update object,
	// if error requeue.
	_, err := findOwnerRefFromList(m.IPPool.OwnerReferences,
		cluster.TypeMeta, cluster.ObjectMeta)
	if err != nil {
		if ok := errors.As(err, &notFoundErr); !ok {
			return err
		}
		m.IPPool.OwnerReferences, err = setOwnerRefInList(
			m.IPPool.OwnerReferences, false, cluster.TypeMeta,
			cluster.ObjectMeta,
		)
		if err != nil {
			return err
		}
	}
	return nil
}

metal3-io-bot · 2022-12-26T02:31:06Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

jessehu · 2023-01-19T11:22:36Z

@furkatgofurov7 https://jenkins.nordix.org/job/metal3_ipam_main_integration_test_ubuntu/32/ returns 404. Could you help check the cause please?

furkatgofurov7 · 2023-01-19T11:27:45Z

/test-ubuntu-integration-main

furkatgofurov7 · 2023-01-19T11:34:42Z

@furkatgofurov7 https://jenkins.nordix.org/job/metal3_ipam_main_integration_test_ubuntu/32/ returns 404. Could you help check the cause please?

hey, looks like we have some problems reaching Jenkins in the CI, let me see if I can find out the cause

Rozzii · 2023-01-19T13:35:01Z

/test-ubuntu-integration-main

Rozzii · 2023-01-19T13:36:39Z

/retest

Rozzii · 2023-01-19T13:47:41Z

@jessehu build 32 doesn't exists anymore so that is why you get the 404 error, oldest build log is build 65 at the moment.
@furkatgofurov7 the bigger issue is that I can't retrigger the /test-ubuntu-integration-main for this PR.

furkatgofurov7 · 2023-01-19T14:32:11Z

@jessehu build 32 doesn't exists anymore so that is why you get the 404 error, oldest build log is build 65 at the moment. @furkatgofurov7 the bigger issue is that I can't retrigger the /test-ubuntu-integration-main for this PR.

I believe Jenkins workers back up now.

/test-ubuntu-integration-main

andrew-est · 2023-01-19T15:43:12Z

/test-ubuntu-integration-main

andrew-est · 2023-01-19T15:52:28Z

/test-ubuntu-integration-main

andrew-est · 2023-01-19T16:45:07Z

/test-ubuntu-integration-main

furkatgofurov7 · 2023-01-20T08:18:19Z

@haijianyang can you pleases rebase the PR?

furkatgofurov7 · 2023-01-20T08:58:31Z

/test-ubuntu-integration-main

furkatgofurov7 · 2023-01-23T19:39:54Z

/test-ubuntu-integration-main

mboukhalfa · 2023-02-01T06:45:11Z

/test-ubuntu-integration-main

haijianyang · 2023-02-01T07:57:41Z

logs-jenkins-metal3_ipam_main_integration_test_ubuntu-79/k8s_management_cluster/kube-system/kube-apiserver-kind-control-plane/kube-apiserver/stdout.log

W0201 07:16:11.110250       1 dispatcher.go:195] rejected by webhook "validation.ippool.ipam.metal3.io": &errors.StatusError{ErrStatus:v1.Status{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ListMeta:v1.ListMeta{SelfLink:"", ResourceVersion:"", Continue:"", RemainingItemCount:(*int64)(nil)}, Status:"Failure", Message:"admission webhook \"validation.ippool.ipam.metal3.io\" denied the request: IPPool.ipam.metal3.io \"provisioning-pool\" is forbidden: IPPool cannot be deleted because it is in use", Reason:"Forbidden", Details:(*v1.StatusDetails)(0xc0165419e0), Code:403}}
W0201 07:16:11.798602       1 dispatcher.go:195] rejected by webhook "validation.ippool.ipam.metal3.io": &errors.StatusError{ErrStatus:v1.Status{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ListMeta:v1.ListMeta{SelfLink:"", ResourceVersion:"", Continue:"", RemainingItemCount:(*int64)(nil)}, Status:"Failure", Message:"admission webhook \"validation.ippool.ipam.metal3.io\" denied the request: IPPool.ipam.metal3.io \"provisioning-pool\" is forbidden: IPPool cannot be deleted because it is in use", Reason:"Forbidden", Details:(*v1.StatusDetails)(0xc016c382a0), Code:403}}
W0201 07:16:12.725203       1 dispatcher.go:195] rejected by webhook "validation.ippool.ipam.metal3.io": &errors.StatusError{ErrStatus:v1.Status{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ListMeta:v1.ListMeta{SelfLink:"", ResourceVersion:"", Continue:"", RemainingItemCount:(*int64)(nil)}, Status:"Failure", Message:"admission webhook \"validation.ippool.ipam.metal3.io\" denied the request: IPPool.ipam.metal3.io \"provisioning-pool\" is forbidden: IPPool cannot be deleted because it is in use", Reason:"Forbidden", Details:(*v1.StatusDetails)(0xc0163e7680), Code:403}}

https://jenkins.nordix.org/job/metal3_ipam_main_integration_test_ubuntu/79/consoleFull

Kind=IPPool\\\" metal3/provisioning-pool: admission webhook \\\"validation.ippool.ipam.metal3.io\\\" denied the request: IPPool.ipam.metal3.io \\\"provisioning-pool\\\" is forbidden: IPPool cannot be deleted because it is in use\"\nDeleting IPPool=\"provisioning-pool\" Namespace=\"metal3\"\nRetrying with backoff Cause=\"error deleting \\\"ipam.metal3.io/v1alpha1

mboukhalfa · 2023-02-01T08:02:41Z

logs-jenkins-metal3_ipam_main_integration_test_ubuntu-79/k8s_management_cluster/kube-system/kube-apiserver-kind-control-plane/kube-apiserver/stdout.log

W0201 07:16:11.110250       1 dispatcher.go:195] rejected by webhook "validation.ippool.ipam.metal3.io": &errors.StatusError{ErrStatus:v1.Status{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ListMeta:v1.ListMeta{SelfLink:"", ResourceVersion:"", Continue:"", RemainingItemCount:(*int64)(nil)}, Status:"Failure", Message:"admission webhook \"validation.ippool.ipam.metal3.io\" denied the request: IPPool.ipam.metal3.io \"provisioning-pool\" is forbidden: IPPool cannot be deleted because it is in use", Reason:"Forbidden", Details:(*v1.StatusDetails)(0xc0165419e0), Code:403}}
W0201 07:16:11.798602       1 dispatcher.go:195] rejected by webhook "validation.ippool.ipam.metal3.io": &errors.StatusError{ErrStatus:v1.Status{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ListMeta:v1.ListMeta{SelfLink:"", ResourceVersion:"", Continue:"", RemainingItemCount:(*int64)(nil)}, Status:"Failure", Message:"admission webhook \"validation.ippool.ipam.metal3.io\" denied the request: IPPool.ipam.metal3.io \"provisioning-pool\" is forbidden: IPPool cannot be deleted because it is in use", Reason:"Forbidden", Details:(*v1.StatusDetails)(0xc016c382a0), Code:403}}
W0201 07:16:12.725203       1 dispatcher.go:195] rejected by webhook "validation.ippool.ipam.metal3.io": &errors.StatusError{ErrStatus:v1.Status{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ListMeta:v1.ListMeta{SelfLink:"", ResourceVersion:"", Continue:"", RemainingItemCount:(*int64)(nil)}, Status:"Failure", Message:"admission webhook \"validation.ippool.ipam.metal3.io\" denied the request: IPPool.ipam.metal3.io \"provisioning-pool\" is forbidden: IPPool cannot be deleted because it is in use", Reason:"Forbidden", Details:(*v1.StatusDetails)(0xc0163e7680), Code:403}}

it seems failing the move step https://github.com/metal3-io/metal3-dev-env/blob/df300e37653cc29e94c20dd06debd014b6a1854c/tests/roles/run_tests/tasks/move.yml#L171

haijianyang · 2023-02-01T09:17:41Z

clusterctl move try to delete the IPPool that have not released IP resources.
Can we release the IP resources before clusterctl move.

furkatgofurov7 · 2023-02-01T14:47:58Z

clusterctl move try to delete the IPPool that have not released IP resources. Can we release the IP resources before clusterctl move.

cc @kashifest

@haijianyang move operation is a special case, and we always check if pivoting is succesful in every CI run. Technically, when move is performed, all provider specific objects are deleted from the source cluster and recreated in the target cluster, meaning we were always able to delete the object (no webhook validation check). But now since the validation is in place, it won't be able to do a move properly, hence fails.

Two options I could think of could be:

using just finalizers instead of webhook
adding a custom logic in the validation webhook, checking "is move operation performed" action and allow the deletion if that is the case.

mboukhalfa · 2023-02-01T15:14:22Z

Before deletion in clusterctl move resources will get paused using the cluster.x-k8s.io/paused annotation if you can check the paused annotation existence from the webhook and just skip the restriction in that case

haijianyang · 2023-02-03T02:47:59Z

clusterctl move will set Cluster.Spec.Paused = true, the cause of the cluster pause is not necessarily by clusterctl move.
If the user manually pauses the cluster, should the IPPool be deleted without releasing IP resources at this time?

Whether it is possible to add a field（spec、annotation、label） to IPPool to describe whether to allow IPPool to be deleted without releasing IP resources?

For example:
By default, it is not allowed to delete IPPools that do not release IP resources.
When adding allow-force-delete: true, it means that IPPools that have not released IP resources can be deleted.

apiVersion: ipam.metal3.io/v1alpha1
kind: IPPool
metadata:
  name: ip-pool-cluster
  namespace: default
  annotations:
    ipclaim.ipam.metal3.io/allow-force-delete: 'true'
spec:
  pools:
    - start: 10.255.160.1
      end: 10.255.160.1
  prefix: 16
  gateway: 10.255.0.1
  namePrefix: "ip-pool-cluster"
---

furkatgofurov7 · 2023-03-07T14:43:34Z

/test-centos-e2e-integration-main

metal3-io-bot · 2023-06-05T15:24:13Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

metal3-io-bot · 2023-07-05T15:40:38Z

Stale issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle stale.

/close

metal3-io-bot · 2023-07-05T15:40:42Z

@metal3-io-bot: Closed this PR.

In response to this:

Stale issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle stale.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

metal3-io-bot · 2023-07-05T15:40:46Z

Can one of the admins verify this patch?

metal3-io-bot · 2023-07-05T15:40:46Z

Can one of the admins verify this patch?

metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 15, 2022

metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 15, 2022

metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 16, 2022

haijianyang force-pushed the feature-add-validatedelete branch from dcf499d to 7ce225a Compare September 20, 2022 02:31

furkatgofurov7 mentioned this pull request Sep 22, 2022

The webhook should reject IPPool deletion if any IP has been allocated #135

Open

metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 26, 2022

furkatgofurov7 removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2023

Add deletion validation for IPPool webhook

765a4a2

haijianyang force-pushed the feature-add-validatedelete branch from 7ce225a to 765a4a2 Compare February 1, 2023 02:23

metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 5, 2023

metal3-io-bot closed this Jul 5, 2023

Add deletion validation for IPPool webhook #137

Add deletion validation for IPPool webhook #137

Conversation

haijianyang commented Sep 15, 2022

metal3-io-bot commented Sep 15, 2022

metal3-io-bot commented Sep 15, 2022

haijianyang commented Sep 16, 2022

mboukhalfa commented Sep 16, 2022

haijianyang commented Sep 21, 2022 • edited Loading

furkatgofurov7 commented Sep 23, 2022

jessehu commented Sep 23, 2022

haijianyang commented Sep 27, 2022

metal3-io-bot commented Dec 26, 2022

jessehu commented Jan 19, 2023

furkatgofurov7 commented Jan 19, 2023

furkatgofurov7 commented Jan 19, 2023

Rozzii commented Jan 19, 2023

Rozzii commented Jan 19, 2023

Rozzii commented Jan 19, 2023

furkatgofurov7 commented Jan 19, 2023

andrew-est commented Jan 19, 2023

andrew-est commented Jan 19, 2023

andrew-est commented Jan 19, 2023

furkatgofurov7 commented Jan 20, 2023

furkatgofurov7 commented Jan 20, 2023

furkatgofurov7 commented Jan 23, 2023

mboukhalfa commented Feb 1, 2023

haijianyang commented Feb 1, 2023 • edited Loading

mboukhalfa commented Feb 1, 2023

haijianyang commented Feb 1, 2023 • edited Loading

furkatgofurov7 commented Feb 1, 2023 • edited Loading

mboukhalfa commented Feb 1, 2023 • edited Loading

haijianyang commented Feb 3, 2023

furkatgofurov7 commented Mar 7, 2023

metal3-io-bot commented Jun 5, 2023

metal3-io-bot commented Jul 5, 2023

metal3-io-bot commented Jul 5, 2023

metal3-io-bot commented Jul 5, 2023

metal3-io-bot commented Jul 5, 2023

haijianyang commented Sep 21, 2022 •

edited

Loading

haijianyang commented Feb 1, 2023 •

edited

Loading

haijianyang commented Feb 1, 2023 •

edited

Loading

furkatgofurov7 commented Feb 1, 2023 •

edited

Loading

mboukhalfa commented Feb 1, 2023 •

edited

Loading