Excluding nodes from restoration #99
Conversation
|
@jrnt30 we need to be able to restore some endpoints: those that are managed by a user. Those are services that don't have a selector specified. |
|
Also FYI the GitHub "fixes" notation requires each issue to have "fixes" in front of it. |
|
Thanks for the clarification. I'll throw this in WIP and see if I can use what I have going or if it needs to be significantly refactored for endpoints. If so I will probably just remove the endpoint support for now. |
jrnt30
changed the title
jrnt30
changed the title
pkg/controller/restore_controller.go
Outdated
| @@ -229,6 +234,13 @@ func (controller *restoreController) processRestore(key string) error { | |||
| restore.Spec.IncludedResources = []string{"*"} | |||
| } | |||
|
|
|||
| curIncludes := sets.NewString(restore.Spec.ExcludedResources...) | |||
There was a problem hiding this comment.
Maybe call this excludedResources or something other than *Includes? 😄
|
Generally LGTM. Just the 1 comment on variable naming. @skriss any other comments? |
pkg/controller/restore_controller.go
Outdated
| @@ -284,6 +296,10 @@ func (controller *restoreController) getValidationErrors(itm *api.Restore) []str | |||
| validationErrors = append(validationErrors, "BackupName must be non-empty and correspond to the name of a backup in object storage.") | |||
| } | |||
|
|
|||
| if errs := collections.ValidateIncludesExcludes(itm.Spec.IncludedResources, nonRestorableResources); len(errs) > 0 { | |||
There was a problem hiding this comment.
This could be misleading if the error returned from ValidateIncludesExcludes is due to something other than nodes being explicitly included. Maybe rather than calling ValidateIncludesExcludes for this, just check if anything in the nonRestorable list is in IncludedResources? Otherwise LGTM
There was a problem hiding this comment.
Good point, thank you.
skriss
changed the title
pkg/controller/restore_controller.go
Outdated
| ) | ||
|
|
||
| // NonRestorableResources is a blacklist for the restoration process. Any resources |
There was a problem hiding this comment.
I am tempted to suggest moving this logic from the controller into pkg/restore. That way, anyone using Ark as a library would benefit from it. WDYT?
There was a problem hiding this comment.
I'll try and take a look on the train on that work.
There was a problem hiding this comment.
Looking at this closer again, I suppose I related the blacklist of resources similarly to the setting of a default as we do with the IncludedResources and IncludedNamespaces.
If we push this down to the pkg/restore I don't think as cleanly segregates the validation logic being exposed to the client either.
Were you thinking that we put this in the kubernetesRestorer.Restore directly or simply a utility function checking these things? I'm afraid I may be missing part of your thought process here (it may be simply that you are proposing a larger refactor than I am considering looking at the type signature here).
There was a problem hiding this comment.
Let's say that someone decides to pull in github.com/heptio/ark/pkg/... to use Ark as a library (we already know of at least one project interested in doing so). If the logic to exclude nodes is only in the controller, then anyone using the library directly and not using the controller would end up restoring nodes unless they also write logic to exclude them.
The more I think about this, given that the other defaulting logic is in the controllers, I think it's fine to include this here for now in the controller. We may decide to move more logic "down" in the future, but I don't think that possibility needs to hold up this PR.
There was a problem hiding this comment.
Understood, thanks. I do think having that more centralized may be beneficial in those cases.
Out of curiosity, is the project you are referring to public?
pkg/controller/restore_controller.go
Outdated
| @@ -284,6 +296,13 @@ func (controller *restoreController) getValidationErrors(itm *api.Restore) []str | |||
| validationErrors = append(validationErrors, "BackupName must be non-empty and correspond to the name of a backup in object storage.") | |||
| } | |||
|
|
|||
| excludedResources := sets.NewString(itm.Spec.IncludedResources...) | |||
There was a problem hiding this comment.
Rename var to includedResources
|
@jrnt30 squash please & I'll merge |
|
@jrnt30 sorry, decided to get logrus in first. Mind rebasing? |
- Introduced a blacklist of resources that are non-restorable. The goal being that the backup can still include these resources for logging/auditing purposes but they are explicitly added to ExcludedResources in the RestorController's "defaulting" logic to ensure that if someone were to explicitly ask for nodes that they would be expressly denied. Signed-off-by: Justin Nauman <justin.r.nauman@gmail.com>
|
No problem, that was a big one for sure. Should be good now. |
run go mod tidy
Fixes #71
goal being that the backup can still include these resources for
logging/auditing purposes but they are explicitly added to
ExcludedResources in the RestorController's "defaulting" logic
to ensure that if someone were to explicitly ask for Node that
they would be expressly denied.
Signed-off-by: Justin Nauman justin.r.nauman@gmail.com