Add deletion policy CLI arguments and annotations #107
Conversation
I am writing an additional test for S3 that creates a series of different namespaces and resources with varying annotations. The only thing that we cannot test with a standard controller e2e test is the CLI argument (we can't update the controller CLI args inside a Pytest). For that the best I can do for now is manual testing, sadly |
| @@ -202,6 +202,8 @@ func (r *resourceReconciler) reconcile( | |||
| return r.deleteResource(ctx, rm, res) | |||
| } | |||
|
|
|||
| rlog := ackrtlog.FromContext(ctx) | |||
| rlog.Info("AWS resource will not be deleted - deletion policy set to retain") | |||
There was a problem hiding this comment.
Is it possible to include a mention of which resource is being retained? Is there a convenient resource name available in res that you can copy a bit above before deletion?
There was a problem hiding this comment.
This is automatically added into the logs when using the rlog object. I posted a comment underneath your last comment asking for logs with an example log message
There was a problem hiding this comment.
These tests can be found in this pull request aws-controllers-k8s/s3-controller#91 |
pkg/runtime/reconciler.go
Outdated
| @@ -1045,6 +1055,35 @@ func (r *resourceReconciler) getRegion( | |||
| return ackv1alpha1.AWSRegion(r.cfg.Region) | |||
| } | |||
|
|
|||
| // getRegion returns the resource's deletion policy based on the default | |||
There was a problem hiding this comment.
Just noticed a typo here. Fixing now
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jljaco, RedbackThomson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -177,6 +201,17 @@ func (c *NamespaceCache) setNamespaceInfoFromK8sObject(ns *corev1.Namespace) { | |||
| if ok { | |||
| nsInfo.endpointURL = EndpointURL | |||
| } | |||
|
|
|||
| nsInfo.deletionPolicies = map[string]string{} | |||
| nsDeletionPolicySuffix := "." + ackv1alpha1.AnnotationDeletionPolicy | |||
There was a problem hiding this comment.
nit: Very tiny optimization: we can declare this as a constant outside of this function to avoid making unnecessary allocations everytime the cache syncs
| continue | ||
| } | ||
| service := strings.TrimSuffix(key, nsDeletionPolicySuffix) | ||
| nsInfo.deletionPolicies[service] = elem |
There was a problem hiding this comment.
I think we can apply a string.ToLower to service variable here. Thinking users can provide an eCR or Eks ...
There was a problem hiding this comment.
I also see that you're using strings.ToLower(service) to query the deletion policy
| if r.getDeletionPolicy(res) == ackv1alpha1.DeletionPolicyDelete { | ||
| // Resolve references before deleting the resource. | ||
| // Ignore any errors while resolving the references | ||
| res, _ = rm.ResolveReferences(ctx, r.apiReader, res) |
There was a problem hiding this comment.
QQ: Any reason why we need to resolve the references here?
There was a problem hiding this comment.
I didn't modify this code, here, I just added a conditional around it. But I believe we need to resolve references so that we have all the necessary fields to call sdkDelete if they require those fields.
Issue #, if available: aws-controllers-k8s/community#82
Description of changes:
As per the proposal: aws-controllers-k8s/community#1148
Adds support for a new optional CLI argument for the controller binary
--deletion-policy, supportingdeleteandretainas available options. Setting this argument todeleteleaves the controller as the current default behaviour, whereby it deletes the AWS resource under management before it is removed from the K8s API server. Setting this argument toretainmodifies the default behaviour, leaving the AWS resources intact (taking no action on them) before removing it from the K8s API server.Adds support for a new annotation on
Namespaceobjects:{service}.services.k8s.aws/deletion-policy: delete/retain(where{service}is the service alias of the controller eg.s3). This annotation overrides the deletion policy behaviour configured for the controller for all resources deployed under the namespace.Adds support for a new annotation on any ACK resource object:
services.k8s.aws/deletion-policy: delete/retain. This annotation overrides only the deletion policy behaviour configured for the resource.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.