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

feat: application resource deletion protection #20497

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

alexmt
Copy link
Collaborator

@alexmt alexmt commented Oct 23, 2024

Depends on argoproj/gitops-engine#630

PR implements resource deletion with confirmation proposal: introduces Delete=confirm and Prune=confirm sync options that allow end users to require manual confirmation before pruning/deleting of annotated resources.

  • As discussed in the proposal PR prune/deletion confirmation is performed on a whole app
  • The approval requires adding annotation argocd.argoproj.io/deletion-approved: <ISO formatted timestamp> instead of just true. This allows end user to confirm pruning multiple times for different sync operations.
Screen.Recording.2024-10-22.at.11.17.50.PM.mov

Copy link

bunnyshell bot commented Oct 23, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@alexmt alexmt force-pushed the resource-deletion-approval branch from 23f4655 to afdeee8 Compare October 23, 2024 07:14
@alexmt alexmt marked this pull request as ready for review October 23, 2024 07:15
@alexmt alexmt requested review from a team as code owners October 23, 2024 07:15
Comment on lines +750 to +751
RequiresDeletionConfirmation: targetObj != nil && resourceutil.HasAnnotationOption(targetObj, synccommon.AnnotationSyncOptions, synccommon.SyncOptionDeleteRequireConfirm) ||
liveObj != nil && resourceutil.HasAnnotationOption(liveObj, synccommon.AnnotationSyncOptions, synccommon.SyncOptionDeleteRequireConfirm),
Copy link
Member

Choose a reason for hiding this comment

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

Do we also need to deal with users deleting the resource directly like using actions to deleted the resource from the cluster? If so did we handle it already (couldn't find it so wanted to verify)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The RequiresDeletionConfirmation is used during app deletion finalizing. So if user delete resource manually then controller won't attempt to delete it and confirmation won't be necessary. I think this is expected behavior.

@alexmt alexmt force-pushed the resource-deletion-approval branch 2 times, most recently from f4c0df5 to ebc8581 Compare October 23, 2024 15:08
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
@alexmt alexmt force-pushed the resource-deletion-approval branch from ebc8581 to 7e64cd8 Compare October 23, 2024 15:23
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 30.88235% with 47 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@ea46572). Learn more about missing BASE report.
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
cmd/argocd/commands/app.go 0.00% 43 Missing ⚠️
pkg/apis/application/v1alpha1/types.go 76.47% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master   #20497   +/-   ##
=========================================
  Coverage          ?   55.04%           
=========================================
  Files             ?      322           
  Lines             ?    54999           
  Branches          ?        0           
=========================================
  Hits              ?    30272           
  Misses            ?    22127           
  Partials          ?     2600           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexmt alexmt merged commit 7c9bd2d into argoproj:master Oct 24, 2024
27 checks passed
@alexmt alexmt deleted the resource-deletion-approval branch October 24, 2024 07:08
adriananeci pushed a commit to adriananeci/argo-cd that referenced this pull request Dec 4, 2024
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
Signed-off-by: Adrian Aneci <aneci@adobe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants