EVEREST-1709: backup rbac tests

[ghost]

EVEREST-1709

[github-actions]

🚫 [golangci-lint] _{reported by reviewdog 🐶}
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)

[github-actions]

🚫 [golangci-lint] _{reported by reviewdog 🐶}
response body must be closed (bodyclose)

[github-actions]

🚫 [golangci-lint] _{reported by reviewdog 🐶}
response body must be closed (bodyclose)

[github-actions]

🚫 [golangci-lint] _{reported by reviewdog 🐶}
should rewrite http.NewRequestWithContext or add (*Request).WithContext (noctx)

[github-actions]

🚫 [golangci-lint] _{reported by reviewdog 🐶}
ST1003: struct field DbClusterName should be DBClusterName (stylecheck)

[recharte]

Overall I like the unit test based approach, it was a good suggestion 👏 However, I have 2 concerns with the tight coupling with the actual implementation.

1. We are checking for enforce calls but we are not asserting what was done with those enforcements. For example, if we had a bug here and called something other than continue the unit tests would pass because the enforce call was done but the end result would be wrong, the list wouldn't have been properly filtered.
2. The tight coupling with the implementation can make these tests brittle with refactors. I know that @mayankshah1607 had some plans to refactor the way we are handling the filtering functions and I'm concern that this testing approach will need many adjustments as part of that refactoring. Let's hear from @mayankshah1607

[recharte]

I think these tests just helped us find a bug 😅
The enforce for ResourceBackupStorages shouldn't be ns/name1. Instead, it should be ns/backup-storage-name that is part of the Spec like so:

Suggested change

	Spec: everestv1alpha1.DatabaseClusterBackupSpec{
	DBClusterName: "db-cluster-name",
	},
	},
	},
	},
	expectedEnforce: []enforceStrings{
	{"alice", rbac.ResourceBackupStorages, rbac.ActionRead, "ns/name1"},
	Spec: everestv1alpha1.DatabaseClusterBackupSpec{
	DBClusterName: "db-cluster-name",
	BackupStorageName: "backup-storage-name",
	},
	},
	},
	},
	expectedEnforce: []enforceStrings{
	{"alice", rbac.ResourceBackupStorages, rbac.ActionRead, "ns/backup-storage-name"},

The bug is here. It should be dbbackup.Spec.BackupStorageName instead of dbbackup.GetName()

[recharte]

The other functions have the same bug 🙈

[mayankshah1607]

The unit-test like approach seems fine, but I share the same concerns as Diogo:

We are checking for enforce calls but we are not asserting what was done with those enforcements.

Agreed with this. Mocking the very component we aim to test—in this case, the RBAC enforcer—may not add much value. Checking if enforce is called with the correct arguments is somewhat useful, but it’s not enough for catching deeper issues such as:

• What happens if the underlying model.conf changes over time?
• What if the enforcement itself is correct but the API logic contains a bug that fails to honour the enforcement?

An important part of testing the integration between the RBAC enforcer and the API logic is verifying that the code correctly respects the decisions made by the enforcer. But in the above cases, we risk missing regressions because the current tests only check if enforce was called with the right arguments, and not what was done with the result.

When I created the ticket for 1709, I imagined a slightly different approach. I'd suggest we mock the Kubernetes API but use a real Everest API and RBAC enforcer to better test the integration:

1. (Optional) Consider using envtest instead of mocking K8s responses. It allows you to easily spin-up and tear down an instance of Kube API (and etcd) from your tests.

2. Use the Kube API from step 1, and populate it with test resources like DatabaseClusters, BackupStorages, etc. that resemble real-world data. Next, our tests will use this data to validate scenarios such as - the ListXX API calls return filtered data, or check if a DatabaseCluster is not accessible if one of the BackupStorage in the schedules is not accessible (we have many such permission dependencies that require explicitly checking the result of enforce).

3. Use a real RBAC enforcer and test different policies with it. We can either manipulate the ConfigMap in each test case, or create a test-specific enforcer whose policy can be manipulated from code.

I feel that with such an approach, we’re able to correctly test the integration between Everest API, RBAC enforcer and Kubernetes, and perform assertions on different kinds of RBAC policies.

---

The tight coupling with the implementation can make these tests brittle with refactors.

Agreed with this again. And I feel this is true not only for the tests, but even with how the API code is currently implemented. Right now, the business logic is embedded directly within the HTTP handler methods. This works fine, but I think there are a few problems:

• It’s tricky to test parts of the business logic in isolation. And this is apparent from this PR, since these tests require setting up and mocking all dependencies, which can be cumbersome and also make them brittle while refactoring.
• Refactoring or adding new functionality becomes harder since everything is intertwined.

To address this, I’m working on a refactor to decouple the business logic from the API handler methods. The core idea is to define an interface for the business logic, with multiple actors implementing this interface. Each actor would handle a specific responsibility, such as RBAC enforcements, request validation, Kubernetes integration, etc. All these actors will be executed serially, in a specific order, forming something similar to a chain of responsibility. This has many advantages:

• Each actor can be tested in isolation by mocking just the "next" actor in the chain. No need to mock the entire dependency graph. This makes testing a lot easier.
• Since responsibilities are isolated and abstracted, making changes to one part of the logic won’t impact others.
• As our business logic grows more complex, this modular structure will make it easier to manage and evolve. For example, if we need to add another external dependency to our business logic, we can create a new implementation of that interface and link it to the chain.

Coming back to the point on testing, I feel like these kinds of unit tests will become a lot simpler once we introduce this kind of a modular structure to our code. I’ve worked with this design pattern at my previous work, and it proved invaluable for maintaining clean and testable code. I'm happy to provide a short PoC if needed.

If it’s okay, I’d like to propose that we hold this PR until we review the refactoring and then re-think how we should proceed with the integration testing. There may not necessarily be overlapping work, but it would be good to ensure we’re not heading in a direction that might require rework later.

Let me know what you think @recharte @michal-kralik 🙂