Implement CRB cleanup #576
Conversation
kyma-bot
added
do-not-merge/work-in-progress
kyma-bot
added
size/XL
kyma-bot
removed
the
do-not-merge/work-in-progress
|
File output might be a problem for running this in a loop on all shoots. |
| flag.StringVar(&cfg.OldLabel, "oldLabel", LabelSelectorOld, "Label marking old CRBs") | ||
| flag.StringVar(&cfg.NewLabel, "newLabel", LabelSelectorNew, "Label marking new CRBs") | ||
| flag.StringVar(&cfg.Output, "output", "", "Output folder for created files. Can also contain file prefix, if it doesn't end with `/` (can be a folder, eg ./foo/)") | ||
| flag.BoolVar(&cfg.DryRun, "dry-run", false, "Don't remove CRBs, write what would be removed to ./removed.json") |
There was a problem hiding this comment.
I think true is a better default for the dry-run flag.
| @@ -0,0 +1,48 @@ | |||
| # CRB Cleanup script | |||
|
|
|||
| This script is used to clean up old ClusterRoleBindings (CRBs) after migration. | |||
There was a problem hiding this comment.
This tool has one particular use case: matching CRBs created by Provisioner with those created with KIM. It doesn't need to be generic, however, I think we can leave it as it is.
| if len(compared.missing) != 0 { | ||
| slog.Warn("Old CRBs not found in new CRBs", "crbs", compared.missing) | ||
| if unmatched != nil { | ||
| err := json.NewEncoder(unmatched).Encode(compared.missing) |
There was a problem hiding this comment.
We store whole CRB objects in the output files. Actually the names of the objects would be sufficient, but I think we can leave it as it is.
| failures, err := ProcessCRBs(fetcher, cleaner, missingFile, cfg) | ||
| if err != nil { |
There was a problem hiding this comment.
Two observations:
- The ticket mentions that if there is no matching crb for the one created by the Provisioner we should lookup into the Runtime CR. The code doesn't do that, but the case is detected by the tool so that I think it is acceptable.
- Once some crbs that doesn't have a counterpart are detected the application stops processing. It could remove the crbs that match, but now everything needs to be done manually.
I don't consider those as defects.
| compared := Compare(ctx, oldCRBs, newCRBs) | ||
|
|
||
| if len(compared.additional) != 0 { | ||
| slog.Info("New CRBs not found in old CRBs", "crbs", compared.additional) |
There was a problem hiding this comment.
It would be more readable if we would print the names of the crbs, not the whole objects.
| slog.Info("New CRBs not found in old CRBs", "crbs", compared.additional) | ||
| } | ||
| if len(compared.missing) != 0 { | ||
| slog.Warn("Old CRBs not found in new CRBs", "crbs", compared.missing) |
There was a problem hiding this comment.
It would be more readable if we would print the names of the crbs, not the whole objects.
| @@ -0,0 +1,106 @@ | |||
| package main | |||
There was a problem hiding this comment.
I think the tool should be more kcp taskrun friendly.
There are two use cases:
- I just want to see what will be removed, and where are the problems that require manual intervention
- I want to do actual remove and learn what needs manual intervention
Exemplary run:
kcp taskrun --target plan=aws -- go run ./cmd/crb-cleanup -output=./run-aws/ -dry-run=true
The above call invokes crb-cleanup for all AWS clusters.
Problems:
run_awsfolder contains only three files (failures.json,missing.json,removed.json) ; what I would expect is to have many files (set of files for each runtime)- The tool prints the following message: "New CRBs not found in old CRBs" ; it happens in case there are crbs for particular subject that exist for KIM but didn't exist for Provisioner - the information is completely redundant, as we don't want to be notified about new CRBs that were added by the user (we want to learn when there is no counterpart and some manual intervention is required)
- I'm now convinced that the tool should not use
oldandnewwords. It should be more specific (old=Provisioner, new=Kim) - output
jsonfiles are created before we know if there will be anything to write ; I need to open the file to see if it actually contains anything
What would make it more friendly:
- output should be stored for each runtime separately ; there are two options:
- files are prefixed with shoot name and stored in the output folder
- there is one folder with shoot name where all the files are stored
- I'm wondering whether in case it was detected that there are some Provisioner crbs that doesn't have KIM counterpart we should exit with error
- printing some "completed" message ; it would be easier to grep the output, and see what runtimes are sound
| flag.StringVar(&cfg.Output, "output", "", "Output folder for created files. Can also contain file prefix, if it doesn't end with `/` (can be a folder, eg ./foo/)") | ||
| flag.BoolVar(&cfg.DryRun, "dry-run", false, "Don't remove CRBs, write what would be removed to ./removed.json") | ||
| flag.BoolVar(&cfg.Verbose, "verbose", false, "Increase the log level to debug (default: info)") | ||
| flag.BoolVar(&cfg.Force, "force", false, "Force remove CRBs without checking migration status") |
There was a problem hiding this comment.
I'm not convinced to have this force option. IMO it's risky.
There was a problem hiding this comment.
It was specifically listed in requirements by @tobiscr, IIRC
| } | ||
|
|
||
| // Failures implements Filer. | ||
| func (j JSONFiler) Failures(failures []Failure) error { |
There was a problem hiding this comment.
Small thing: the Failures, Missing, and Removed functions are almost identical we could have a helper function called in all those three functions.
|
|
||
| All of the log files will be created either way. | ||
|
|
||
| ### prefixing logs based on env |
There was a problem hiding this comment.
I would express that it is about running with kcp tool
Description
Changes proposed in this pull request:
Related issue(s)