feat: refactor and improve K8s sync provider

[Kavindu-Dodan]

This PR

fixes #434

This is a refactoring and internal improvement for K8s ISync provider.

Improvements include,

• Reduce K8s API load by utilizing Informer cache
• Yet, provide a fallback if cache miss occurs (Note - we rely on K8s Informer for cache refill and consistency)
• Informer now only watches a specific namespace (compared to *) - this is a potential performance improvement and a security improvement
• Reduced informer handlers with extracted common logics
• Unit tests where possible

[codecov]

Codecov Report

Merging #443 (4f551fc) into main (dc71e84) will decrease coverage by 3.91%.
The diff coverage is 33.94%.

❗ Current head 4f551fc differs from pull request most recent head 35e5e14. Consider uploading reports for the commit 35e5e14 to get more accurate results

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
- Coverage   66.17%   62.27%   -3.91%     
==========================================
  Files          11       13       +2     
  Lines        1437     1617     +180     
==========================================
+ Hits          951     1007      +56     
- Misses        430      547     +117     
- Partials       56       63       +7     

Impacted Files	Coverage Δ
pkg/sync/kubernetes/kubernetes_sync.go	36.36% <33.94%> (ø)
pkg/sync/kubernetes/inotify.go	100.00% <0.00%> (ø)
pkg/store/flags.go	86.57% <0.00%> (+5.42%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[thisthat]

I left only minor suggestions to improve logging a bit. I see that parts of the code are not tested, in particular the one with K8s details. For that, I recommend having a look at the fake client (docs)

[toddbaert]

I tested this in OFO locally (latest version) and it works as expected.

[Kavindu-Dodan]

I left only minor suggestions to improve logging a bit. I see that parts of the code are not tested, in particular the one with K8s details. For that, I recommend having a look at the fake client (docs)

Thank you. I addressed the log and renaming related comments. Regarding testing, K8s backed logic. Even with a fake client, we still need to mock the shared informer interface as we have a fallback. I am not sure how worth these mocks are against real e2e tests which easily test most of the logic.

[toddbaert]

I agree with @thisthat that we probably need a bit more coverage here (I know that there was no tests here before, so that's not your fault). I think now is the time. If there's any way to mock the k8s data I think we should do some basics here. It's worth another day or so of effort I think.

Maybe @thisthat has some specific recommendations.

Besides that everything here looks good to me and I think this is an improvement.

[thisthat]

K8s provides great fake clients that can be used to unit-test your informer logic.
I think a good starting point to see an example is this test file in Keptn: https://github.com/keptn/lifecycle-toolkit/blob/91e57cadba32fce6d873bc480408f90bcb8964d9/metrics-operator/cmd/metrics/adapter/provider/provider_test.go

[Kavindu-Dodan]

K8s provides great fake clients that can be used to unit-test your informer logic. I think a good starting point to see an example is this test file in Keptn: https://github.com/keptn/lifecycle-toolkit/blob/91e57cadba32fce6d873bc480408f90bcb8964d9/metrics-operator/cmd/metrics/adapter/provider/provider_test.go

Yes, I understand the motivation here. However, if you check the FakeInformer internals, then you see that this is incomplete ex:- [1]

The main blocker is not the K8s client but the behavior of the informer. We either need a test suite to recreate an informer mock and react on the eventing or spin up a real K8s backend based on envtest.Environment [2]

Anyway, I know testing is important and I am fully agreeing to the coverage requirements. However, this PR was specifically focusing on an improvement that is blocking (I assume) another set of tasks. So I do not know how much effort we should put into this PR and try to introduce tests that were never there before.

[1] - https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/controller/controllertest#FakeInformer.GetStore
[2] - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/controller/controllertest#pkg-overview

[toddbaert]

@Kavindu-Dodan we can create a new issue for additional testing. I think at a minimum we should get the coverage diff here to 0% with some meaningful tests unless it's really impossible. This isn't a place where we can afford to lose coverage.

[Kavindu-Dodan]

@Kavindu-Dodan we can create a new issue for additional testing. I think at a minimum we should get the coverage diff here to 0% with some meaningful tests unless it's really impossible. This isn't a place where we can afford to lose coverage.

It is not impossible. It's time to deliver. Soon there will be merged conflicts with upcoming PRs and given that we constantly change API contracts, the efforts can go wasted.

Besides, I introduced new tests and simply touched code with the refactoring, reducing complexity. Even with new tests, for some reason, we see a reduction in coverage. To be honest I do not know how this happened.

[Kavindu-Dodan]

@Kavindu-Dodan we can create a new issue for additional testing. I think at a minimum we should get the coverage diff here to 0% with some meaningful tests unless it's really impossible. This isn't a place where we can afford to lose coverage.

It is not impossible. It's time to deliver. Soon there will be merged conflicts with upcoming PRs and given that we constantly change API contracts, the efforts can go wasted.

Besides, I introduced new tests and simply touched code with the refactoring, reducing complexity. Even with new tests, for some reason, we see a reduction in coverage. To be honest I do not know how this happened.

To prove my point, here is a PR with the change we need - #451

This will work and since I am not refactoring, code coverage won't complain.

[toddbaert]

@Kavindu-Dodan we can create a new issue for additional testing. I think at a minimum we should get the coverage diff here to 0% with some meaningful tests unless it's really impossible. This isn't a place where we can afford to lose coverage.

It is not impossible. It's time to deliver. Soon there will be merged conflicts with upcoming PRs and given that we constantly change API contracts, the efforts can go wasted.
Besides, I introduced new tests and simply touched code with the refactoring, reducing complexity. Even with new tests, for some reason, we see a reduction in coverage. To be honest I do not know how this happened.

To prove my point, here is a PR with the change we need - #451

This will work and since I am not refactoring, code coverage won't complain.

Alright. I will create a new issue to get some coverage here.

[Kavindu-Dodan]

@thisthat @toddbaert I added more tests however the coverage is below the previous value due to the reason I explained [1]

Please have another round of review :)

[1] #443 (comment)

[toddbaert]

@Kavindu-Dodan @thisthat I created #460 for testing.