Adds in-mem `PolicyReportStore` implementation (#107) #150

nnelas · 2023-12-05T15:35:34Z

Description

As specified in the original issue, this commit:

Create a store Go interface that satisfies the current store implementation at report/store.go
Refactor current report/store.go into report/store_kubernetes.go, as an implementation of the store interface
Adds a new memory store with 2 maps, one with ClusterPolicyReports and another one with PolicyReports
Provide an optional cli flag, --store TYPE, with possible types memory, kubernetes. Defaults to --store kubernetes for ease on first Kubewarden installations.

Besides that, this commit also:

Changes printJSON to outputScan to make it more aligned with what the input params is
outputScan only outputs at the end of the flow, to prevent having a lot of logs whenever someone scans the entire cluster (we were outputting the entire store on each namespace loop)

It also seems that running make fmt introduced a couple of unwanted changes that I'm happy to revert to keep this commit smaller. Finally, I also had to do a couple of tweaks to make golangci lint happy.

Test

To test this pull request, you can run the following commands:

make unit-tests

Additional Information

Tradeoff

Potential improvement

One thing that we can also do in the future is to parse (in some way) the toJSON() result and instead of logging the entire report in a single log block, we could have a log line per violation. What do you think?

Also, I'll be out-of-office for the rest of the week, but I'll return next Monday. Looking forward to seeing your comments! ❤️

jvanz

I could not finish testing the PR yet. But I'll continue soon.

internal/report/store.go

jvanz

Please, beside the comments, review the linter warning. I got some of them when building the PR locally.

cmd/root.go

jvanz · 2023-12-06T12:42:42Z

internal/policies/fetcher.go

@@ -194,7 +194,7 @@ func (f *Fetcher) getAdmissionPolicies(namespace string) ([]policiesv1.Admission
 	return policies.Items, nil
 }

-func newClient() (client.Client, error) { //nolint:ireturn
+func newClient() (client.Client, error) { //nolint:ireturn // no specific client type


Can you elaborate in the comment that you added? What do you mean exactly? ;-)

to be fair, this was a quick and dirty "solution" to try to keep ireturn lint happy. for some reason, it's a bit flaky as sometimes nolintlint complains that that comment is not needed but it's not consistent :/

Ah! I know that pain! Try remove this comment and just clean the linter cache:

./bin/golangci-lint cache clean && make lint

thank you so much for your tip! It did the trick 🎉

internal/report/store_memory.go

README.md

cmd/root.go

internal/report/store.go

internal/report/store_memory.go

internal/report/store.go

internal/report/store_memory_test.go

viccuad

Hey, thanks for this PR and tackling this! ❤️

I would like to not add logic to cmd/root.go, but keep it in internal/report/ and internal/scanner as it was.

Also, separate commits would indeed help for the review, better to have multiple trivial commits than one big one. I would like separate commits for moving a file without modifications (report/store.go -> report/store_kubernetes.go), or adding style changes.

Part of kubewarden#107 As specified in the original issue, this commit: - Create a store Go interface that satisfies the current store implementation at report/store.go - Refactor current report/store.go into report/store_kubernetes.go, as an implementation of the store interface - Adds a new memory store with 2 maps, one with `ClusterPolicyReports` and another one with `PolicyReports` - Provide an optional cli flag, --store TYPE, with possible types memory, kubernetes. Defaults to --store kubernetes for ease on first Kubewarden installations. Besides that, this commit also: - Changes `printJSON` to `outputScan` to make it more aligned with what the input params is - `outputScan` only outputs at the end of the flow, to prevent having a lot of logs whenever someone scans the entire cluster (we were outputting the entire store on each namespace loop) It also seems that running `make fmt` introduced a couple of unwanted changes that I'm happy to revert to keep this commit smaller. Finally, I also had to do a couple of tweaks to make golangci lint happy. Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

- Removes `RetryOnConflict` since we will only need them for Kubernetes backend - Changes "enums" to lower-case (and updates README accordingly) Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

- Removes Update methods from PolicyReportStore interface since there's no current usage for them Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

- Removes `mutex` since there's no parallel processing - Changes logLevels for both `PolicyReportStore` to debug to prevent output of non-relevant information by default. Also, aligns what is logged between `PolicyReport` and `ClusterPolicyReport` and also `MemoryPolicyReportStore` and `KubernetesPolicyReportStore` Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

- Removes `require` dependency from unit-tests. Instead, uses if-clauses to validate the returning errors - Given that I was changing go.mod, I also updated some of our dependencies - Still struggling with `ireturn`. Sometimes it works and build succeeds, sometimes it doesn't Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

- Changes godocs since it doesn't make much sense to have an interface knowing about it's implementations Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

- Simplifies cmd/root.go logic, since it should only read cmd flags and thus shouldn't have any business logic. There's still an `outputScan` that should also be moved after clarification of kubewarden#150 (comment) Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

nnelas · 2023-12-12T14:49:15Z

hallo @jvanz and @viccuad ! first of all, thank you so much for your suggestions/comments, really helpful to keep the codebase consistent and aligned with your standards ❤️

I think I addressed all your topics, except for #150 (comment) and #150 (comment) since I need further help/clarification for them

- As discussed in kubewarden#150 (comment), this commit removes `outputScan` logic from `cmd` and moves it to `scanner.go` where it originally was. This ensures we don't add more business logic into a package that should only handle commands Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

jvanz

Thanks @nnelas! I think we are almost merging this PR ❤️

I just have some small requests about the linter. Besides that, I would say we can wait for another @viccuad review and merge if he approved.

jvanz · 2023-12-13T13:04:45Z

internal/policies/fetcher_test.go

 var nsKubewarden = v1.Namespace{
 	ObjectMeta: metav1.ObjectMeta{
 		Name:   "kubewarden",
 		Labels: map[string]string{"kubernetes.io/metadata.name": "kubewarden"},
 	},
 }

-func mockClient(initObjs ...k8sClient.Object) k8sClient.Client { //nolint:ireturn
+func mockClient(initObjs ...k8sClient.Object) k8sClient.Client { //nolint:ireturn // no specific client type


Again, try the trick about the linter that I've shared in another comment

sorry for not replying to you on this thread! that suggestion you made worked so I removed the new comment from both methods

internal/report/store.go

jvanz · 2023-12-13T13:17:11Z

Furthermore, @nnelas can you rebase and fix the conflicts in the go.sum file? 😁

flavio

Great contribution, thanks a lot @nnelas!

flavio · 2023-12-13T16:04:00Z

internal/report/store_memory.go

+	cprCache map[string]ClusterPolicyReport
+}
+
+func NewMemoryPolicyReportStore() (*MemoryPolicyReportStore, error) {


Since it never fails, we could omit error from the return type

I had mixed feelings about this one, I was thinking of keeping all constructors for all PolicyReportStore implementations coherent, but I'm now thinking that doesn't make sense because we'll have different dependencies for each store. I'll remove the error

README.md

…emory_policy-report-store_implementation Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

jvanz

LGTM. @kubewarden/kubewarden-developers remember to squash the commits when merging this PR!

@jvanz

- Removes "dumb" comment to try to fix golangci. The solution that @jvanz gave [here](kubewarden#150 (comment)) worked like a charm! - Removes unused methods from `PolicyReportStore` interface Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

- Simplifies `NewMemoryPolicyReportStore` method signature, by removing `error` which is never used Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

…emory_policy-report-store_implementation Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

go.mod

internal/report/store_kubernetes.go

viccuad

Looks good! Many thanks for dealing with the changes!

The only blocker to me is the bump on the sigs.k8s.io/wg-policy-prototypes library, I would like to postpone that.

- Reverts `wg-policy-prototypes` upgrade since this library is not correctly tagged nor has proper releases, so it's best to separate the upgrade to another PR that we can properly test Signed-off-by: Nuno Nelas <nuno.nelas@icloud.com>

nnelas · 2023-12-15T14:10:59Z

Looks good! Many thanks for dealing with the changes!

The only blocker to me is the bump on the sigs.k8s.io/wg-policy-prototypes library, I would like to postpone that.

hallo @viccuad ! thank you so much for your review 🙇 I've reverted that upgrade under 10c4eee so I think I've addressed all your suggestions. in the meantime, I'll create a PR in https://github.com/kubewarden/helm-charts to use this new --store flag

viccuad

LGTM, thanks!

viccuad · 2023-12-18T11:20:29Z

golangci-lint was again cryptically failing, and passing locally. Ended up deleting its cache under https://github.com/kubewarden/audit-scanner/actions/caches.

GH says merging is blocked because all commits must be signed, yet DCO reports all are signed Oo. Maybe it got confused with unsigned merge commits from gh.

@flavio could you merge this please?

flavio · 2023-12-18T14:25:51Z

Squashed and merged. Thanks for your contribution @nnelas!

nnelas requested a review from a team as a code owner December 5, 2023 15:35

nnelas force-pushed the feature/107-adds_in-memory_policy-report-store_implementation branch from df03f6f to 48106e8 Compare December 5, 2023 15:37

jvanz requested changes Dec 6, 2023

View reviewed changes

internal/report/store.go Outdated Show resolved Hide resolved

jvanz requested changes Dec 6, 2023

View reviewed changes

jvanz assigned nnelas Dec 6, 2023