refactor(repo): Simplify structure #308
Conversation
simar7
force-pushed
the
refactor-structure
branch
from
ad52a3f to
e281ce0
Compare
| const ComplianceFolder = "compliance" | ||
| // Loader access compliance specs | ||
| type Loader interface { | ||
| GetSpecByName(name string) string |
There was a problem hiding this comment.
Is there a need for this interface?
There was a problem hiding this comment.
Probably not but since we already have it, removal of it would be a breaking change that I don't want to make in this PR.
|
@itaysk for your review: do you want to keep node-collector |
ARCHITECTURE.md
Outdated
There was a problem hiding this comment.
I think the project structure should be in the readme
ARCHITECTURE.md
Outdated
| @@ -7,10 +7,9 @@ This document aims to answer the question *Where is the code that does X?* | |||
| The directory structure is broken down as follows: | |||
|
|
|||
| - `cmd/` - These CLI tools are primarily used during development for end-to-end testing without needing to pull the library into trivy/tfsec etc. | |||
There was a problem hiding this comment.
this could also be improved a bit
There was a problem hiding this comment.
are any of the tools relevant for an external contributor? if so we should mention them
ARCHITECTURE.md
Outdated
| @@ -7,10 +7,9 @@ This document aims to answer the question *Where is the code that does X?* | |||
| The directory structure is broken down as follows: | |||
|
|
|||
| - `cmd/` - These CLI tools are primarily used during development for end-to-end testing without needing to pull the library into trivy/tfsec etc. | |||
| - `checks` - All of the checks are defined in this directory. | |||
| - `checks` - All the checks are defined in this directory. | |||
| - `commands` - All Node-collector commands are defined in this directory. | |||
There was a problem hiding this comment.
let's link to node collector docs to make sure the context is undestood
There was a problem hiding this comment.
Do we want to continue developing node-collector and its associated functionality? I asked it here.
If not we should remove it from trivy-checks if this repo is solely for the purposes of keeping checks.
ARCHITECTURE.md
Outdated
| - `commands` - All Node-collector commands are defined in this directory. | ||
| - `pkg/spec` - Logic to handle standardized specs such as CIS. | ||
| - `pkg/rules` - This package exposes internal rules, and imports them accordingly (see _rules.go_). | ||
| - `pkg/rules` - This package exposes internal checks, and imports them accordingly (see _rules.go_). |
There was a problem hiding this comment.
I don't understand what this means. also 1. didn't we rename rules to checks? 2. shouldn't this code be in trivy?
There was a problem hiding this comment.
It's no longer used. I'll remove it.
ARCHITECTURE.md
Outdated
| - `commands` - All Node-collector commands are defined in this directory. | ||
| - `pkg/spec` - Logic to handle standardized specs such as CIS. | ||
| - `pkg/rules` - This package exposes internal rules, and imports them accordingly (see _rules.go_). | ||
| - `pkg/rules` - This package exposes internal checks, and imports them accordingly (see _rules.go_). | ||
| - `specs/` - Standaridized compliance specs such as CIS. |
There was a problem hiding this comment.
I think specs is oerloaded term. for a newcomer "specs" probably relate more to bdd specs or to standardidation specs. the feature in trivy is called "compliance" shouldn't the directory be called that way too?
There was a problem hiding this comment.
theres a doc about contributing compliance specs under docs/compliance.md. it's the only doc in the docs dir I thin we should make it a readme in this dir, like kubernetes related info is documented in readme under checks/kubernetes, then we can remove /docs, which should be in trivy anyway
| | Target | Description | | ||
| |----------------|------------------------------------------------------------------------------------------------------------------------------------| | ||
| | Network | Checks primarily targeting the networking stack | | ||
| | Dynamic | Checks that evaluate deprecated and removed APIs | |
There was a problem hiding this comment.
either the description is not accurate or the dir name is bad. I think the intention was that these checks rely on environmental context for evaluation. for example, the deprecated API checks rely on information of which k8s version the user is running/evaluating against in order to decide if outdated or not.
There was a problem hiding this comment.
I agree the name isn't very meaningful but then again I didn't want to change it in this PR as there maybe users who rely on this structure.
I couldn't come up with a better description, so I'm open to ideas. Would the following work based on what you said?
"Checks whose decision outcome is dynamically determined based on the environmental context."
| |----------------|------------------------------------------------------------------------------------------------------------------------------------| | ||
| | Network | Checks primarily targeting the networking stack | | ||
| | Dynamic | Checks that evaluate deprecated and removed APIs | | ||
| | CIS Benchmarks | Checks that are recommended by the CIS Benchmarks. The checks inside are targeted per each subsystem (e.g. apiserver, cni, etc.) | |
There was a problem hiding this comment.
If this dir is serving only CIS, and CIS is using only this dir then fine, but I will challange us abit here: aren't these checks valid k8s checks also outside CIS context? aren't CIS reports include k8s checks besides these checks (i presume something like non privileged pod check).
There was a problem hiding this comment.
You're right in the sense that these checks are ordinary checks even outside of CIS context.
I'm open to re-organizing all k8s checks in general (maybe based on another type) but we just need to confirm first with the stakeholders that this won't cause any breakage as in the past some users did rely on the structure. Can we do this in another PR?
| | Network | Checks primarily targeting the networking stack | | ||
| | Dynamic | Checks that evaluate deprecated and removed APIs | | ||
| | CIS Benchmarks | Checks that are recommended by the CIS Benchmarks. The checks inside are targeted per each subsystem (e.g. apiserver, cni, etc.) | | ||
| | Advanced | Checks that are recommended for the advanced uesrs of Kubernetes | |
There was a problem hiding this comment.
how do we define advanced users
| | CIS Benchmarks | Checks that are recommended by the CIS Benchmarks. The checks inside are targeted per each subsystem (e.g. apiserver, cni, etc.) | | ||
| | Advanced | Checks that are recommended for the advanced uesrs of Kubernetes | | ||
| | GKE | Checks specific to Google Kubernetes Engine | | ||
| | PSS | Checks pertaining to Pod Security Standards | |
pkg/specs/loader.go
Outdated
There was a problem hiding this comment.
shouldn't this be in trivy?
There was a problem hiding this comment.
Not quite because then users will need to import trivy as a library to load compliance specs which isn't ideal as it's a much bigger dependency.
commands/live somewhere else? They're used by k8snode-collector(see comment refactor(repo): Simplify structure #308 (comment))