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

Fix tenant cascade deletion #747

Merged
merged 3 commits into from
May 6, 2022
Merged

Fix tenant cascade deletion #747

merged 3 commits into from
May 6, 2022

Conversation

eguzki
Copy link
Member

@eguzki eguzki commented May 6, 2022

what

Fix cascading deletion issue when provider account secret is not found

Verification steps

  • Create tenant and admin pass secret
k apply -f - <<EOF
---
apiVersion: v1
kind: Secret
metadata:
  name: example-admin-secret
type: Opaque
stringData:
  admin_password: "123456"
---
apiVersion: capabilities.3scale.net/v1alpha1
kind: Tenant
metadata:
  name: example-tenant
spec:
  username: admin
  systemMasterUrl: https://master.3scale.example.com
  email: admin@example.com
  organizationName: example
  masterCredentialsRef:
    name: system-seed
  passwordCredentialsRef:
    name: example-admin-secret
  tenantSecretRef:
    name: example-tenant-secret
EOF
  • Create backend associated with that tenant
k apply -f - <<EOF
---
apiVersion: capabilities.3scale.net/v1beta1
kind: Backend
metadata:
  name: backend-a
spec:
  name: "Operated Backend A"
  systemName: "backenda"
  privateBaseURL: "https://exampleA.com"
  providerAccountRef:
    name: example-tenant-secret
  • Create product associated with that tenant
k apply -f - <<EOF
---
apiVersion: capabilities.3scale.net/v1beta1
kind: Product
metadata:
  name: product1
spec:
  name: "OperatedProduct 1"
  backendUsages:
    backenda:
      path: /v1
  providerAccountRef:
    name: example-tenant-secret
  • Remove tenant
k delete tenant  example-tenant
  • Confirm product and backend CRs are deleted as well. If deletion fails, the objects deletion process will be stuck and objects will remain available but marked for deletion

@eguzki eguzki requested a review from MStokluska May 6, 2022 07:54
@eguzki eguzki mentioned this pull request May 6, 2022
Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

This looks correct to me.

Sometimes the deletion of the API Backend will be accepted in 3scale regardless of existing associations with the API Product – i.e., when the parent Tenant has been marked for destruction as well –, while some other times it won't. This is subject to concurrency and cannot be predicted in cases of cascade deletion.

Ref: https://github.com/3scale/porta/blob/33cc4d5bcb5910295bfa28d84416ff05e1a606f3/app/models/backend_api.rb#L114


// Attempt to remove backend only if backend.Status.ID is present
if backend.Status.ID == nil {
logger.Info("could not remove backend because backend ID is missing for backend name")
Copy link
Contributor

@MStokluska MStokluska May 6, 2022

Choose a reason for hiding this comment

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

if the ID is nil I think line 305 will be throwing nil pointer?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch! I forgot the early return nil

}

providerAccount, err := controllerhelper.LookupProviderAccount(r.Client(), backend.Namespace, backend.Spec.ProviderAccountRef, logger)
if apierrors.IsNotFound(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be cleaner that way:

if err != nil {
   if apierrors.IsNotFound(err) {
       		logger.Info("backend not deleted from 3scale, provider account not found")
		return nil
   }
   return err
 }

@codeclimate
Copy link

codeclimate bot commented May 6, 2022

Code Climate has analyzed commit e7f80b6 and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3
Duplication 2

View more on Code Climate.

@eguzki eguzki requested a review from MStokluska May 6, 2022 09:18
@MStokluska
Copy link
Contributor

MStokluska commented May 6, 2022

Code looks good to me,
I've verified on cluster with
Tenant creates 2 backends and 3 products where 2 products are associated with a single backend and 3rd product associated with the second backend
Single backend deletion works as expcted, backend mentions are removed from associated products while 3rd product still points to the correct backend
When tenant gets deleted all CRs are removed correctly.

Good job!

/lgtm

@eguzki eguzki merged commit f6c3d5f into master May 6, 2022
@eguzki eguzki deleted the fix-tenant-cascade-deletion branch May 6, 2022 10:09
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