HNC: Update hncconfig validator to avoid overwriting source

[yiqigao217]

Part of #1076 , see http://bit.ly/hnc-propagation-conflict.

Overwriting source could happen when a type is changed from another mode to Propagate mode and there happens to be an ancestor and a descendant objects with the same name.

The current hncconfig validator only checks if the new configuration meets the restrictions, e.g. no duplicate types, Propagate mode for Role and Rolebinding, etc. Now since we need to detect a mode change, we will need to get the old configuration, which can be done by adding a checkForest() func.

We also need to get all the objects, especially the object names, of the types that are changed from others to Propagate. The current forest (in-memory data-structure) only stores user-created objects if the type mode is Propagate. Therefore, we cannot get the objects from the forest for those types switched to Propagate from others.

A proposal is that we can

• Add a new field called objectNames (map[schema.GroupVersionKind]map[string]bool) to the forest.Namespace struct, to store all the names of the user-created objects by GVK as long as there's an object reconciler for the GVK.
• Ask users, who wants to change a type to Propagate, to first change it to Remove mode so that an object reconciler can be created for this type. Or we can just update the user guide to say that it requires changing to Remove mode first for any mode changes between Ignore and Propagate, since we already require that for Propagate => Ignore switch.
• In object reconciliations, always forest.ns.AddObjectName() in syncSource() func and forest.ns.RemoveObjectName() in syncMissingObject() func.

With objectNames field in the forest, the hncconfig validator can detect the object name conflicts for non-Propagate types. As for the conflicts, the data-structure will look like this:

type conflicts map[schema.GroupVersionKind]map[string]affectedNamespaces
type affectedNamespaces map[string][]string

with specified GVK and object name, and groups of affectedNamespaces grouped by the first ancestor namespace name, considering the conflicts can happen on different branches in the forest and users have to resolve them separatedly.

[yiqigao217]

/cc @adrianludwin
/cc @rjbez17
/cc @GinnyJI

I tried implementing it and it seemed working fine and I will do more tests on it. Please let me know if there's any concerns of the changes in the forest (new ns.objectNames field), object reconciler (updating ns.objectNames) and the hncconfig validator (conflict datastructure), etc.

[adrianludwin]

@rjbez17 does this lgty?

As discussed offline: this lgtm. We'll add a confirmation prompt in kubectl-hns if users try to go straight from Ignore to Propagate with a warning and a link to the documentation, but users can either say yes or just skip the prompt with the --yes command.

[rjbez17]

This LGTM aside from the potential race conditions of user created objects being created during a propagation/propagation change. Which could occur when using something like helm/kustomize to update a large amount of resources. I think our existing protections should help with this though.

Overall, I think this will work, just would like to make sure we understand most of the edge cases in the PR before merging.

[adrianludwin]

Yeah, we can never guarantee that HNC is fully "up-to-date" since any object can change at any time. But that's just life in a distributed system. Agreed to having a close look at the edge cases when the PR is ready.

…

On Mon, Sep 14, 2020 at 10:48 AM Ryan Bezdicek ***@***.***> wrote: This LGTM aside from the potential race conditions of user created objects being created during a propagation/propagation change. Which could occur when using something like helm/kustomize to update a large amount of resources. I think our existing protections should help with this though. Overall, I think this will work, just would like to make sure we understand most of the edge cases in the PR before merging. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1102 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE43PZHYP52N5EHV5LCPKVLSFYUM5ANCNFSM4RLUITWQ> .

[yiqigao217]

Update here: discussed offline with @adrianludwin :
It would be better not to introduce a new field into the forest, and instead to always store the source object and change the rest of the object to suit (e.g. don't propagate objects just because they're in the forest - we'll need this logic for exceptions anyway).

Thus now having #1106 with 2 commits (refactoring & webhook change) in it.

[adrianludwin]

@yiqigao217 @GinnyJI is there anything more we need to do for this?

[yiqigao217]

The implementation is done, so closing this.
We just need to update the doc see #1172