Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

feat: add support for batch v1 cronjob #522

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bluebrown
Copy link

@bluebrown bluebrown commented Jan 30, 2023

Signed-off-by: Nico Braun [email protected]

Description

Add support for batch/v1 CronJob

Fixes #520

Type of change
  • New feature ✨
How Has This Been Tested?
  • Test A
  • Test B

The test has failed on me.

go test ./internal/k8sinternal/
# throws errors

But building the project and testing it via curl has worked.

make build
curl -fsSL https://raw.githubusercontent.com/kubernetes/website/snapshot-initial-v1.26/content/en/examples/application/job/cronjob.yaml   \
| ./kubeaudit all -f -
# finds issues as desired

I didn't really understand the test suite. Maybe someone can give me a hint.

Checklist:
  • I have 🎩 my changes (A 🎩 specifically includes pulling down changes, setting them up, and manually testing the changed features and potential side effects to make sure nothing is broken)
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The test coverage did not decrease
  • I have signed the appropriate Contributor License Agreement

@genevieveluyt
Copy link
Contributor

Thanks for the quick fix!

I didn't really understand the test suite

Testing is documented here. You will likely want the USE_KIND=false make test if you do not have Kind installed locally. This will only test manifest mode but the other modes will be tested by CI with Kind. CI is currently broken but should hopefully be fixed by #521

  1. Please sign the CLA
  2. Please update the cronjob test fixture to make sure we're testing the new version
  3. You will likely have to rebase / merge main into your branch after Fix CI: tidy with 1.17 #521 in order for CI to pass, apologies for the blocker!

@bluebrown
Copy link
Author

bluebrown commented Jan 31, 2023

So I figured out why the test is failing. Kind of. Since my PR would support both beta and v1, there is a mismatch in the resource count here and here.

First error ends with ...should have 26 item(s), but has 24 which is 2*1, since doing ns*rsources. And the second one is ...should have 13 item(s), but has 12 for that extra resource, I guess.

I haven't figured out how to teach it, that there are more resources now.

@genevieveluyt
Copy link
Contributor

So for resources in a cluster, the kubernetes API returns all of the resources in the same version, regardless of what version the resource was originally created as. This is why we don't test for multiple versions of the same resource (kubeaudit can't tell them apart once the versions get normalized). So instead of adding a version in the test, I think we just want to replace the old one.

@bluebrown
Copy link
Author

I can remove the beta one from the test. But it would be still better to keep the actual thing around so that both versions are supported right?

Maybe we can find a way to have the tests work with multiple versions.

@bluebrown
Copy link
Author

bluebrown commented Feb 1, 2023

I guess this is failing now because the test is still using 1.20 for the kind image. But batch v1 cronjob was only released in 1.21.

Given that kubernetes supports the last 3 versions officially, I think we should use 1.24 as kind image.

@bluebrown
Copy link
Author

I tested locally to bump the version. Now there is the next problem. It throws deprecated errors for the beta cronjob. So those tests need be adjusted as well, if we want to bump the version.

@genevieveluyt
Copy link
Contributor

We indeed only support the latest Kubernetes version. If you are bumping the Kind version, here is a reference PR of the last time it was bumped. Unfortunately the test suite is not set up to support backwards compatibility with older Kubernetes versions.

@bluebrown
Copy link
Author

Sorry, I think I misread your comment. I have also removed the fixture containg the beta job but it still errors.

Here is one of the errors. They are all from TestAuditDeprecatedAPIs. Since this test is supposed to test deprecated apis, I dont think I should also remove beta from those fixtures.

--- FAIL: TestAuditDeprecatedAPIs (0.00s)
    --- FAIL: TestAuditDeprecatedAPIs/cronjob.yml-1.20-1.21 (16.71s)
        test.go:53: 
                Error Trace:    /home/blue/src/kubeaudit/auditors/deprecatedapis/test.go:53
                                                        /home/blue/src/kubeaudit/auditors/deprecatedapis/test.go:30
                                                        /home/blue/src/kubeaudit/auditors/deprecatedapis/depreceatedapis_test.go:57
                Error:          Not equal: 
                                expected: map[string]bool{"DeprecatedAPIUsed":true}
                                actual  : map[string]bool{}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,3 +1,2 @@
                                -(map[string]bool) (len=1) {
                                - (string) (len=17) "DeprecatedAPIUsed": (bool) true
                                +(map[string]bool) {
                                 }
                Test:           TestAuditDeprecatedAPIs/cronjob.yml-1.20-1.21

@genevieveluyt
Copy link
Contributor

Sorry I was out of office last week, will take a look this week

@bluebrown
Copy link
Author

Yeah. I didn't find time to look further into this either. I am also not entirely sure how to proceed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubeaudit does not detect issues in cronjobs
2 participants