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 seeing resources after rolebinding has been deleted for user #3433

Merged

Conversation

opudrovs
Copy link
Contributor

@opudrovs opudrovs commented Oct 4, 2023

Closes #2733

  • Added being able to create ClusterRoles and Roles without rules to simulate manual creation of objects for delete transactions.

  • Added creating a role or binding client object for delete transactions manually in case of an "object not found" error (which was previously returned + ignored for objects without finalizers).

  • Added sending delete object transactions with manually created RBAC client objects for deleted roles and bindings in case of an "object not found" error.

  • As discussed with @enekofb , added IsValidID method to role to be able to delete roles without policy rules (those, which are manually constructed for passing them with delete transactions).

  • Updated corresponding tests in reconciler, rolecollector, and store packages.

  • Minor refactoring in variable names for consistency.

Notes:

  • Added a CategoryRBAC category to objectkind to support assertObjectTransaction calls when running reconciler tests on RBAC objects. If there is another option, please let me know.

  • In rolecollector_test, count of upsert, delete, and deleteAll transactions (or calls?) are retained between tests. Is it expected behavior?

  • Comment that "// Explorer should support aggregated clusteroles." was added based on a discussion with @enekofb

Testing:
I test it when running the app in Tilt. So, first run WGE with Tilt as a developer.

The export the wego-admin-read-apps ClusterRoleBinding and gitops-apps-reader ClusterRole as YAML and copy and paste the output to a YAML file to re-apply them later.

kubectl get clusterrolebinding wego-admin-read-apps -o yaml
kubectl get clusterrole gitops-apps-reader -o yaml

Then open Explorer, confirm that objects are visible in Explorer as expected, and delete these ClusterRoleBinding and ClusterRole:

kubectl delete clusterrolebinding wego-admin-read-apps
kubectl delete clusterrole gitops-apps-reader

The objects should disappear in Explorer. Then apply the YAML to which you saved the output to re-create these ClusterRoleBinding and ClusterRole — and the objects should appear in Explorer again.

Here is a screen recording of a testing session:

Screen.Recording.2023-10-05.at.18.45.55.mov

Not sure why the only object appears after deleting the ClusterRoleBinding, but I don't think it is related to Explorer, probably smth. related to how RBAC is setup for development.

@opudrovs opudrovs added the bug Something isn't working label Oct 4, 2023
@opudrovs opudrovs self-assigned this Oct 4, 2023
@opudrovs opudrovs force-pushed the 2733-fix-resources-visible-after-role-binding-has-been-deleted branch 13 times, most recently from d3c2b57 to cc2d22a Compare October 9, 2023 10:48
@opudrovs opudrovs marked this pull request as ready for review October 9, 2023 11:33
@opudrovs opudrovs requested a review from a team October 9, 2023 11:33
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

LGTM! Love the extra test coverage and clever demo.

pkg/query/collector/reconciler/reconciler.go Show resolved Hide resolved
@opudrovs opudrovs force-pushed the 2733-fix-resources-visible-after-role-binding-has-been-deleted branch 2 times, most recently from 3195090 to 59bf3f9 Compare October 9, 2023 22:54
@opudrovs
Copy link
Contributor Author

opudrovs commented Oct 9, 2023

@jpellizzari thank you! 😸

…manually in case of an "object not found" error.

Add sending delete object transactions for deleted roles and bindings in case of an "object not found" error.

Add `IsValidID` method to role to be able to delete roles without policy rules (those, which are manually constructed to pass with delete transactions).

Update corresponding tests in `reconciler`, `rolecollector`, and `store` packages.
@opudrovs opudrovs force-pushed the 2733-fix-resources-visible-after-role-binding-has-been-deleted branch from 59bf3f9 to 10143b7 Compare October 11, 2023 09:32
@opudrovs opudrovs merged commit 0ab2321 into main Oct 11, 2023
@opudrovs opudrovs deleted the 2733-fix-resources-visible-after-role-binding-has-been-deleted branch October 11, 2023 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

explorer: i can see resources after rolebinding has been deleted for my user
2 participants