Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
wip: try to delete the CR as a side-effect of namespace deletion
Browse files Browse the repository at this point in the history
Signed-off-by: Simone Tiraboschi <[email protected]>
tiraboschi committed Dec 3, 2020

Verified

This commit was signed with the committer’s verified signature.
1 parent dc50e7f commit be258c1
Showing 8 changed files with 88 additions and 60 deletions.
Original file line number Diff line number Diff line change
@@ -2454,7 +2454,7 @@ spec:
resources:
- namespaces
sideEffects: NoneOnDryRun
timeoutSeconds: 30
timeoutSeconds: 900
type: MutatingAdmissionWebhook
webhookPath: /mutate-ns-hco-kubevirt-io
- admissionReviewVersions:
Original file line number Diff line number Diff line change
@@ -2454,7 +2454,7 @@ spec:
resources:
- namespaces
sideEffects: NoneOnDryRun
timeoutSeconds: 30
timeoutSeconds: 900
type: MutatingAdmissionWebhook
webhookPath: /mutate-ns-hco-kubevirt-io
- admissionReviewVersions:
28 changes: 20 additions & 8 deletions hack/test_delete_ns.sh
Original file line number Diff line number Diff line change
@@ -20,22 +20,34 @@
set -ex

function test_delete_ns(){
OLM=0
if [ "${CMD}" == "oc" ]; then
echo "Trying to delete kubevirt-hyperconverged namespace when the hyperconverged CR is still there"
# this should fail with a clear error message
${CMD} delete namespace kubevirt-hyperconverged 2>&1 | grep "denied the request: HyperConverged CR is still present, please remove it before deleting the containing namespace"
CSV=$(oc get csv -n kubevirt-hyperconverged -o name | wc -l)
if [ "$CSV" -gt 0 ]; then
OLM=1
fi
fi

if [ "$OLM" -gt 0 ]; then
# TODO: remove this once we are able to run the webhook also on k8s
echo "HCO has been deployed by the OLM, so its webhook is supposed to work: let's test it"

echo "kubevirt-hyperconverged namespace should be still there"
${CMD} get namespace kubevirt-hyperconverged -o yaml

echo "Trying to delete kubevirt-hyperconverged namespace when the hyperconverged CR is still there"
time timeout 60m ${CMD} delete namespace kubevirt-hyperconverged

else
# TODO: remove this once we are able to run the webhook also on k8s
echo "Ignoring webhook on k8s where we don't have OLM based validating webhooks"
fi

echo "Delete the hyperconverged CR to remove the product"
timeout 10m ${CMD} delete hyperconverged -n kubevirt-hyperconverged kubevirt-hyperconverged
echo "Delete the hyperconverged CR to remove the product"
time timeout 30m ${CMD} delete hyperconverged -n kubevirt-hyperconverged kubevirt-hyperconverged

echo "Finally delete kubevirt-hyperconverged namespace"
time timeout 30m ${CMD} delete namespace kubevirt-hyperconverged
fi

echo "Finally delete kubevirt-hyperconverged namespace"
timeout 10m ${CMD} delete namespace kubevirt-hyperconverged
}

24 changes: 14 additions & 10 deletions pkg/apis/hco/v1beta1/hyperconverged_webhook.go
Original file line number Diff line number Diff line change
@@ -35,7 +35,7 @@ type WebhookHandlerIfs interface {
ValidateCreate(hc *HyperConverged) error
ValidateUpdate(requested *HyperConverged, exists *HyperConverged) error
ValidateDelete(hc *HyperConverged) error
HandleMutatingNsDelete(ns *corev1.Namespace, dryRun bool) (bool, error)
HandleMutatingNsDelete(ns *corev1.Namespace, dryRun bool) error
}

var whHandler WebhookHandlerIfs
@@ -128,8 +128,12 @@ type nsMutator struct {
decoder *admission.Decoder
}

// TODO: nsMutator should try to delete HyperConverged CR before deleting the namespace
// currently it simply blocks namespace deletion if HyperConverged CR is there
// nsMutator is trying to to delete HyperConverged CR before accepting the deletion request for the namespace
// if it accepts the deletion request, the namespace should be ready to be delete
// without getting stuck or without the risk of any leftovers.
// Please notice that the serviceAccounts used by other operator are also part of this namespace
// so once we accept the deletion of the namespace, all the components operator are going to quickly
// lose any power
func (a *nsMutator) Handle(ctx context.Context, req admission.Request) admission.Response {
hcolog.Info("reaching nsMutator.Handle")
ns := &corev1.Namespace{}
@@ -144,18 +148,18 @@ func (a *nsMutator) Handle(ctx context.Context, req admission.Request) admission
return admission.Errored(http.StatusBadRequest, err)
}

admitted, herr := whHandler.HandleMutatingNsDelete(ns, *req.DryRun)
if err != nil {
herr := whHandler.HandleMutatingNsDelete(ns, *req.DryRun)
if herr != nil {
hcolog.Error(herr, hcoutil.HCONSWebhookPath+" refused the request")
return admission.Errored(http.StatusInternalServerError, herr)
}
if admitted {
return admission.Allowed("the namespace doesn't contain HyperConverged CR, admitting its deletion")
}
return admission.Denied("HyperConverged CR is still present, please remove it before deleting the containing namespace")

hcolog.Info(hcoutil.HCONSWebhookPath + " admitted the delete request")
return admission.Allowed(hcoutil.HCONSWebhookPath + " admitted the delete request")
}

// ignoring other operations
return admission.Allowed("ignoring other operations")
return admission.Allowed(hcoutil.HCONSWebhookPath + " admitted the request")

}

4 changes: 3 additions & 1 deletion pkg/components/components.go
Original file line number Diff line number Diff line change
@@ -971,6 +971,8 @@ func GetCSVBase(name, namespace, displayName, description, image, replaces strin

sideEffectMutating := admissionregistrationv1.SideEffectClassNoneOnDryRun
webhookPathMutating := util.HCONSWebhookPath
// give it enough time to uninstall the whole product
var webhookNSTimeout int32 = 900

ns_mutating_webhook := csvv1alpha1.WebhookDescription{
GenerateName: util.HcoMutatingWebhookNS,
@@ -980,7 +982,7 @@ func GetCSVBase(name, namespace, displayName, description, image, replaces strin
AdmissionReviewVersions: []string{"v1beta1", "v1"},
SideEffects: &sideEffectMutating,
FailurePolicy: &failurePolicy,
TimeoutSeconds: &webhookTimeout,
TimeoutSeconds: &webhookNSTimeout,
ObjectSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"name": namespace},
},
8 changes: 5 additions & 3 deletions pkg/util/util.go
Original file line number Diff line number Diff line change
@@ -234,9 +234,11 @@ func ComponentResourceRemoval(ctx context.Context, c client.Client, obj interfac

labels := resource.GetLabels()

if app, labelExists := labels[AppLabel]; !labelExists || app != hcoName {
logger.Info("Existing resource wasn't deployed by HCO, ignoring", "Kind", resource.GetObjectKind())
return nil
if resource.GetObjectKind().GroupVersionKind().Kind != "HyperConverged" {
if app, labelExists := labels[AppLabel]; !labelExists || app != hcoName {
logger.Info("Existing resource wasn't deployed by HCO, ignoring", "Kind", resource.GetObjectKind())
return nil
}
}

opts := &client.DeleteOptions{}
33 changes: 6 additions & 27 deletions pkg/webhooks/webhooks.go
Original file line number Diff line number Diff line change
@@ -11,7 +11,6 @@ import (
sspv1 "github.com/kubevirt/kubevirt-ssp-operator/pkg/apis/kubevirt/v1"
vmimportv1beta1 "github.com/kubevirt/vm-import-operator/pkg/apis/v2v/v1beta1"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
kubevirtv1 "kubevirt.io/client-go/api/v1"
@@ -24,6 +23,7 @@ import (

const (
updateDryRunTimeOut = time.Second * 3
whDeleteTimeOut = 120 * time.Second
)

type WebhookHandler struct {
@@ -180,43 +180,22 @@ func (wh WebhookHandler) ValidateDelete(hc *v1beta1.HyperConverged) error {
return nil
}

func (wh WebhookHandler) HandleMutatingNsDelete(ns *corev1.Namespace, dryRun bool) (bool, error) {
func (wh WebhookHandler) HandleMutatingNsDelete(ns *corev1.Namespace, dryRun bool) error {
wh.logger.Info("validating namespace deletion", "name", ns.Name)

if ns.Name != wh.namespace {
wh.logger.Info("ignoring request for a different namespace")
return true, nil
return nil
}

ctx := context.TODO()
tCtx, cancel := context.WithTimeout(context.Background(), whDeleteTimeOut)
defer cancel()
hco := &v1beta1.HyperConverged{
ObjectMeta: metav1.ObjectMeta{
Name: hcoutil.HyperConvergedName,
Namespace: wh.namespace,
},
}

// TODO: once the deletion of HCO CR is really safe during namespace deletion
// (foreground deletion, context timeouts...) try to automatically
// delete HCO CR if there.
// For now let's simply block the deletion if the namespace with a clear error message
// if HCO CR is still there

key, err := client.ObjectKeyFromObject(hco)
if err != nil {
wh.logger.Error(err, "failed to get object key for HyperConverged CR")
return false, err
}
found := &v1beta1.HyperConverged{}
err = wh.cli.Get(ctx, key, found)
if err != nil {
if apierrors.IsNotFound(err) {
wh.logger.Info("HCO CR doesn't not exist, allow namespace deletion")
return true, nil
}
wh.logger.Error(err, "failed getting HyperConverged CR")
return false, err
}
wh.logger.Info("HCO CR still exists, forbid namespace deletion")
return false, nil
return hcoutil.EnsureDeleted(tCtx, wh.cli, hco, hcoutil.HyperConvergedName, wh.logger, dryRun, !dryRun)
}
47 changes: 38 additions & 9 deletions pkg/webhooks/webhooks_test.go
Original file line number Diff line number Diff line change
@@ -570,19 +570,33 @@ var _ = Describe("webhooks handler", func() {
wh := &WebhookHandler{}
wh.Init(logger, cli, HcoValidNamespace)

allowed, err := wh.HandleMutatingNsDelete(ns, false)
err := wh.HandleMutatingNsDelete(ns, false)
Expect(err).ToNot(HaveOccurred())
Expect(allowed).To(BeTrue())
})

It("should not allow the delete of the namespace if Hyperconverged CR exists", func() {
cli := fake.NewFakeClientWithScheme(s, cr)
It("should return error if deletion of HCO CR returns an error", func() {
cr := &v1beta1.HyperConverged{
ObjectMeta: metav1.ObjectMeta{
Name: ResourceName,
Namespace: HcoValidNamespace,
},
Spec: v1beta1.HyperConvergedSpec{},
}

ns := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: HcoValidNamespace,
},
}

c := getFakeClient(s, cr)
cli := errorClient{c, hcDeleteFailure}
wh := &WebhookHandler{}
wh.Init(logger, cli, HcoValidNamespace)

allowed, err := wh.HandleMutatingNsDelete(ns, false)
Expect(err).ToNot(HaveOccurred())
Expect(allowed).To(BeFalse())
err := wh.HandleMutatingNsDelete(ns, false)
Expect(err).NotTo(BeNil())
Expect(err).Should(Equal(ErrFakeHCOError))
})

It("should ignore other namespaces even if Hyperconverged CR exists", func() {
@@ -596,9 +610,8 @@ var _ = Describe("webhooks handler", func() {
},
}

allowed, err := wh.HandleMutatingNsDelete(other_ns, false)
err := wh.HandleMutatingNsDelete(other_ns, false)
Expect(err).ToNot(HaveOccurred())
Expect(allowed).To(BeTrue())
})

})
@@ -664,6 +677,7 @@ const (
kubevirtMetricsAggregationUpdateFailure
vmImportUpdateFailure
timeoutError
hcDeleteFailure
)

type errorClient struct {
@@ -680,6 +694,7 @@ var (
ErrFakeTemplateValidatorError = errors.New("fake TemplateValidator error")
ErrFakeMetricsAggregationError = errors.New("fake MetricsAggregation error")
ErrFakeVMImportError = errors.New("fake VMImport error")
ErrFakeHCOError = errors.New("fake HCO error")
)

func (ec errorClient) Update(ctx context.Context, obj runtime.Object, opts ...client.UpdateOption) error {
@@ -725,3 +740,17 @@ func (ec errorClient) Update(ctx context.Context, obj runtime.Object, opts ...cl

return ec.Client.Update(ctx, obj, opts...)
}

func (ec errorClient) Delete(ctx context.Context, obj runtime.Object, opts ...client.DeleteOption) error {

if ec.failure == hcDeleteFailure {
return ErrFakeHCOError
}

if ec.failure == timeoutError {
// timeout + 100 ms
time.Sleep(updateDryRunTimeOut + time.Millisecond*100)
}

return ec.Client.Delete(ctx, obj, opts...)
}

0 comments on commit be258c1

Please sign in to comment.