Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement backup and restore for Cluster Role Bindings and OIDC #591

Merged
merged 20 commits into from
Jan 9, 2025

Conversation

akgalwas
Copy link
Contributor

@akgalwas akgalwas commented Dec 31, 2024

Description

Changes proposed in this pull request:

  • Backup for Cluster Role Bindings and OIDC
  • Restore for Cluster Role Bindings and OIDC

Related issue(s)
#557

@akgalwas akgalwas requested a review from a team as a code owner December 31, 2024 13:28
@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 31, 2024
@akgalwas akgalwas changed the title Implement backup and restore for Cluster Role Bingings Implement backup and restore for Cluster Role Bindings Dec 31, 2024
@akgalwas akgalwas changed the title Implement backup and restore for Cluster Role Bindings Implement backup and restore for Cluster Role Bindings and OIDC Jan 4, 2025
@akgalwas akgalwas changed the title Implement backup and restore for Cluster Role Bindings and OIDC Implement backup and restore for Cluster Role Bindings Jan 4, 2025
@akgalwas akgalwas changed the title Implement backup and restore for Cluster Role Bindings Implement backup and restore for Cluster Role Bindings and OIDC Jan 4, 2025
Copy link
Contributor

@koala7659 koala7659 left a comment

Choose a reason for hiding this comment

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

Please check my comments below:

@@ -96,6 +98,8 @@ func NewRestoreConfig() RestoreConfig {
flag.StringVar(&restoreConfig.InputType, "input-type", InputTypeJSON, "Type of input to be used. Possible values: **txt** (see the example hack/runtime-migrator/input/runtimeids_sample.txt), and **json** (see the example hack/runtime-migrator/input/runtimeids_sample.json).")
flag.StringVar(&restoreConfig.InputFilePath, "input-file-path", "/path/to/input/file", "Path to the input file containing RuntimeCRs to be migrated.")
flag.StringVar(&restoreConfig.BackupDir, "backup-path", "/path/to/backup/dir", "Path to the directory containing backup.")
flag.BoolVar(&restoreConfig.RestoreCRB, "restore-crbs", true, "Flag determining whether CRBs should be restored")
flag.BoolVar(&restoreConfig.RestoreOIDC, "restore-oidcs", true, "Flag determining whether OIDCs should be restored")
Copy link
Contributor

Choose a reason for hiding this comment

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

Pls add new flags to func PrintRestoreConfig(cfg RestoreConfig) function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

resultsFile, err := r.outputWriter.SaveRestoreResults(r.results)
if err != nil {
return err
}

slog.Info(fmt.Sprintf("Restore completed. Successfully restored backups: %d, Failed operations: %d", r.results.Succeeded, r.results.Failed))
slog.Info(fmt.Sprintf("Restore completed. Successfully restored backups: %d, Failed operations: %d, Skipped backups: %d, ", r.results.Succeeded, r.results.Failed, r.results.Skipped))
Copy link
Contributor

@koala7659 koala7659 Jan 7, 2025

Choose a reason for hiding this comment

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

Please also align report for the number of skipped backups with backup.go file. Currently in dry-run mode skipped backups are not reported correctly.

2025/01/07 18:08:18 INFO Backup completed. Successfully stored backups: 0, Failed backups: 0

@@ -86,14 +89,28 @@ func (r Restore) Do(ctx context.Context, runtimeIDs []string) error {
continue
}

if currentShoot.Generation == objectsToRestore.OriginalShoot.Generation {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced if we should have this condition to skip restore process. What about restoring possibly deleted or broken CRBs? Those objects are are not related with shoot generation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@koala7659 koala7659 left a comment

Choose a reason for hiding this comment

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

LGTM

@kyma-bot kyma-bot added the lgtm Looks good to me! label Jan 9, 2025
@kyma-bot kyma-bot merged commit cc31c47 into kyma-project:main Jan 9, 2025
9 checks passed
@akgalwas akgalwas deleted the backup-restore-3 branch January 10, 2025 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants