Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Prune Istio Namespace created by Operator when deleting Operator cr #484

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

Prune Istio Namespace created by Operator when deleting Operator cr #484

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: 11 additions & 0 deletions pkg/helmreconciler/pruning.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ func (h *HelmReconciler) Prune(all bool) error {
if err != nil {
allErrors = append(allErrors, err)
}
//prune namespace created by istio operator at last
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite a dangerous operation since it will delete anything that the user created in the namespace too. At the very least, we should create a flag to guard this - but IMO it may be better to leave this step up to the user since it's not very onerous. Thoughts @howardjohn @sdake ?

Copy link
Member

Choose a reason for hiding this comment

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

I am not as familiar with common operator patterns, but it seems dangerous to me. I think we should error on the side of NOT deleting stuff - will a user be more upset if they have an empty namespace around, or if we delete their critical applications?

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally speaking, we shouldn't be creating or deleting namespaces. If we are creating namespaces, access should be restricted as, by definition, the operator is managing the lifecycle of that namespace.

Typically, an operator will create resources in the same namespace as the CR defining the install/behavior or in a completely segregated namespace, where all interaction would be through the CR.

Copy link
Member

Choose a reason for hiding this comment

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

i agree, at the minimum this should be guarded with a flag... like you see a confirmation - do you really want to delete this github repo?

Also, how do you know if the namespace isn't there prior to install of the operator?

Copy link
Member Author

@irisdingbj irisdingbj Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

if the namespace is created by operator, we will mark it via adding related operator lables and annotations, eg: install.operator.istio.io/owner-generation.

If the namespace is created prior to install of the operator, this PR will make sure operator will not touch the namespace(not adding related labels and annotations)

So in this way, we can know whether the namespace is created by operator or not. and we delete them only if it is created by operator.

Copy link
Member

@sdake sdake Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Generally speaking, we shouldn't be creating or deleting namespaces. If we are creating namespaces, access should be restricted as, by definition, the operator is managing the lifecycle of that namespace.

Typically, an operator will create resources in the same namespace as the CR defining the install/behavior or in a completely segregated namespace, where all interaction would be through the CR.

Assuming the Istio CR is registered in the istio-operator namespace Is this not handled by https://github.com/istio/operator/blob/master/deploy/operator.yaml#L33-L34? If the answer is yes, perhaps there is some way to enforce the exact behavior we want as you have specified.

ns := []schema.GroupVersionKind{{
Group: "",
Version: "v1",
Kind: "Namespace",
},
}
err = h.PruneResources(ns, all, "")
if err != nil {
allErrors = append(allErrors, err)
}
return utilerrors.NewAggregate(allErrors)
}

Expand Down
32 changes: 20 additions & 12 deletions pkg/helmreconciler/rendering.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,12 @@ func (h *HelmReconciler) ProcessObject(chartName string, obj *unstructured.Unstr
return utilerrors.NewAggregate(allErrors)
}

gvk := obj.GetObjectKind().GroupVersionKind()
update := true
if gvk.Kind == "Namespace" && gvk.Version == "v1" {
annotations := obj.GetAnnotations()
_, update = annotations["install.operator.istio.io/owner-generation"]
}
mutatedObj, err := h.customizer.Listener().BeginResource(chartName, obj)
if err != nil {
log.Errorf("error preprocessing object: %s", err)
Expand All @@ -197,7 +203,7 @@ func (h *HelmReconciler) ProcessObject(chartName string, obj *unstructured.Unstr
}

receiver := &unstructured.Unstructured{}
receiver.SetGroupVersionKind(mutatedObj.GetObjectKind().GroupVersionKind())
receiver.SetGroupVersionKind(gvk)
objectKey, _ := client.ObjectKeyFromObject(mutatedObj)

var patch Patch
Expand All @@ -219,17 +225,19 @@ func (h *HelmReconciler) ProcessObject(chartName string, obj *unstructured.Unstr
}
}
}
} else if patch, err = h.CreatePatch(receiver, mutatedObj); err == nil && patch != nil {
log.Info("updating existing resource")
mutatedObj, err = patch.Apply()
if err == nil {
if err = h.customizer.Listener().ResourceUpdated(mutatedObj, receiver); err != nil {
log.Errorf("unexpected error occurred during postprocessing of updated resource: %s", err)
}
} else {
listenerErr := h.customizer.Listener().ResourceError(obj, err)
if listenerErr != nil {
log.Errorf("unexpected error occurred invoking ResourceError on listener: %s", listenerErr)
} else if update {
if patch, err = h.CreatePatch(receiver, mutatedObj); err == nil && patch != nil {
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe it's better to merge the two if condition checks.

log.Info("updating existing resource")
mutatedObj, err = patch.Apply()
if err == nil {
if err = h.customizer.Listener().ResourceUpdated(mutatedObj, receiver); err != nil {
log.Errorf("unexpected error occurred during postprocessing of updated resource: %s", err)
Copy link
Member

Choose a reason for hiding this comment

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

In very overloaded clusters where the resource request is not processed, simply logging the Error and moving on is not sufficient.

}
} else {
listenerErr := h.customizer.Listener().ResourceError(obj, err)
if listenerErr != nil {
log.Errorf("unexpected error occurred invoking ResourceError on listener: %s", listenerErr)
Copy link
Member

Choose a reason for hiding this comment

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

The same problem with error processing. There are other instances of this poor error handling in this function that you didn't add. Perhaps you can get it right in your changes and return with a followup PR to address the problems in the remainder of this code?

Cheers
-steve

}
}
}
}
Expand Down