Skip to content

Commit 25da756

Browse files
authored
Apply suggestions from code review
https://github.blog/changelog/2022-06-08-admins-can-require-sign-off-on-web-based-commits/ Co-authored-by: Sebastian Franz <32578476+SebieF@users.noreply.github.com> Signed-off-by: Jannik Hollenbach <jannik@hollenbach.de>
1 parent 4ef1537 commit 25da756

1 file changed

Lines changed: 17 additions & 17 deletions

File tree

docs/adr/adr_0012.md

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ Currently all custom resources for the secureCodeBox are isolated into the names
2121

2222
If you then want to start other scans which for other targets you might want to create another namespace `demo-two`. To run scans in `demo-two` you'll also have to install nmap in that namespace.
2323

24-
Another (possibly even more annoying) scenario where the need to have the `ScanType`s and `ParseDefinition`s installed in every namespace is apparent when looking at the Kubernetes AutoDiscovery. The AutoDiscovery automatically starts scans for resources (e.g. a ZAP Scan for http service, trivy scans for container images) it discovers in the individual namespaces. At the moment this only works properly if the namespace where the discovered resource was discovered in has the correct `ScanType` installed.
24+
Another (possibly even more annoying) scenario with the need to have the `ScanType`s and `ParseDefinition`s installed in every namespace is apparent when looking at the Kubernetes AutoDiscovery. The AutoDiscovery automatically starts scans for resources (e.g. a ZAP Scan for http service, trivy scans for container images) it discovers in the individual namespaces. At the moment this only works properly if the namespace where the resource was discovered in has the correct `ScanType` installed.
2525

2626
## Prior Art
2727

@@ -30,44 +30,44 @@ CertManager has two different `Custom Resource Definitions` for issuers: `Issuer
3030

3131
## Assumptions
3232

33-
This proposal ais to provide a solution which makes the secureCodeBox easier to use both in single and multi-tenant cluster.
34-
For multi-tenant clusters this proposal assumes access to the cluster wide custom resources proposed in this ADR is locked down to be only accessible by cluster admins and not by everybody.
33+
This proposal aims to provide a solution which makes the secureCodeBox easier to use both in single and multi-tenant cluster.
34+
For multi-tenant clusters, this proposal assumes that access to the cluster wide custom resources, proposed in this ADR, is locked down to be only accessible by cluster admins and not by everybody.
3535

3636
## Decision
3737

3838
In addition to the existing `ScanType`, `ParseDefinition`, `ScanCompletionHook` and `CascadingRule` the following additional cluster-wide scoped resource should be introduced: `ClusterScanType`, `ClusterParseDefinition`, `ClusterScanCompletionHook` and `ClusterCascadingRule`. The behavior of these is detailed in the following sections.
3939

40-
The CRD `Scan` and `ScheduledScan` don't require `ClusterWide` variants as they are tied directly to the execution of the scan jobs which are tied to a namespace. They are don't provide any service / reusability to other secureCodeBox components (unlike the CRDs listed above).
40+
The CRD `Scan` and `ScheduledScan` don't require `ClusterWide` variants as they are tied directly to the execution of the scan jobs which are themselves tied to a namespace. They don't provide any service / re-usability to other secureCodeBox components (unlike the CRDs listed above).
4141

4242
### `ClusterScanType`
4343

44-
Other then being cluster scoped `ClusterScanType`s are identical to the existing `ScanType` CRD.
44+
Other than being cluster scoped, `ClusterScanType`s are identical to the existing `ScanType` CRD.
4545

46-
When a new scan is started and the operator requires the scan job template it should first look for `ScanType`s in the `Scan`s namespace matching the `Scan.spec.scanType` name configured in the scan. If that is not found the operator should look for `ClusterScanType`s with the same name.
47-
No matter which was picked the `Job` for the scan always has to be started in the namespace of the `Scan`.
46+
When a new scan is started and the operator requires the scan job template, it should first look for `ScanType`s in the `Scan`s namespace matching the `Scan.spec.scanType` name configured in the scan. If that is not found the operator should look for `ClusterScanType`s with the same name.
47+
No matter which one was picked, the `Job` for the scan always has to be started in the namespace of the `Scan`.
4848

4949
#### Issues with ConfigMap / Secret Mounts:
5050

51-
Some features of normal scanTypes will not function normally with ClusterScanTypes, especially mounting values from ConfigMaps / Secrets as files / environment variables into your containers as the ConfigMaps / Secrets are not existing in every namespace and there is not sensible way for to roll them out with our helm chart to roll them out.
51+
Some features of normal scanTypes will not function normally with ClusterScanTypes, especially mounting values from ConfigMaps / Secrets as files / environment variables into your containers as the ConfigMaps / Secrets are not existing in every namespace and there is no sensible way to roll them out with our helm chart.
5252

5353
When deploying cluster scans types you have to make sure that either:
5454

5555
1. All referenced ConfigMaps / Secrets exist in all namespaces. This will likely be done by a script / operator generating these configmaps and persisting them into every namespace
56-
2. Remove all dependencies to ConfigMaps / Secrets from your scanTypes and move these either into the container image of the scanner, or by specifing a initContainer in the ScanType which copies these files over to volume shared with the scanner container. Some more references and discussions on ways to improve this in the future can be found in [ADR-0009: Architecture for pre-populating the file system of scanners"](./adr-0009)
56+
2. All dependencies to ConfigMaps / Secrets are removed from your scanTypes and either moved into the container image of the scanner, or are specified by an initContainer in the ScanType. The initContainer copies these files over to volumes shared with the scanner container. Some more references and discussions on ways to improve this in the future can be found in [ADR-0009: Architecture for pre-populating the file system of scanners"](./adr-0009)
5757

5858
### `ClusterParseDefinition`
5959

60-
Other then being cluster scoped `ClusterParseDefinition`s are identical to the existing `ParseDefinition` CRD.
60+
Other than being cluster scoped, `ClusterParseDefinition`s are identical to the existing `ParseDefinition` CRD.
6161

6262
When a scan job has completed and the results need to be parsed, the operator should first look for `ParseDefinition`s in the `Scan`s namespace matching the `scantype.spec.extractResults.type` / `clusterscantype.spec.extractResults.type` name configured in the ScanType.
6363
If that is not found the operator should look for `ClusterParseDefinition`s with the same name.
6464
No matter which was picked the `Job` for the parser always has to be started in the namespace of the `Scan`.
6565

6666
### `ClusterCascadingRule`
6767

68-
Other then being cluster scoped `ClusterCascadingRule`s are identical to the existing `CascadingRule` CRD.
68+
Other than being cluster scoped, `ClusterCascadingRule`s are identical to the existing `CascadingRule` CRD.
6969

70-
When the cascading scans hook starts, the hook should fetch the list of both `CascadingRule`s and `ClusterCascadingRule`s and merge them together into one list which then get's evaluated against the scan just like it is now. All scans started from it have to be in the same namespace of the original `Scan`.
70+
When the cascading scans hook starts, the hook should fetch the list of both `CascadingRule`s and `ClusterCascadingRule`s and merge them together into one list. The list then gets evaluated against the scan just like it is now. All scans started from it have to be in the same namespace of the original `Scan`.
7171

7272
#### Deduplication of CascadingRules
7373

@@ -77,9 +77,9 @@ Potential workarounds here would be to introduce an additional field to the Casc
7777

7878
### `ClusterScanCompletionHook`
7979

80-
`ClusterScanCompletionHook` behave mostly like their namespaced counterpart, when the parser completes the operator fetches the list of ScanCompletionHooks from Scans namespace (respecting the hookSelector of the Scan) and the `ClusterScanCompletionHook`s (also respecting the hookSelector of the Scan (this might have to be discussed...)) are merged and ordered according to the respective priorities set on them.
80+
`ClusterScanCompletionHook` behave mostly like their namespaced counterpart, when the parser completes, the operator fetches the list of ScanCompletionHooks from Scans namespace (respecting the hookSelector of the Scan) and the `ClusterScanCompletionHook`s (also respecting the hookSelector of the Scan (this might have to be discussed...)) are merged and ordered according to the respective priorities set on them.
8181

82-
The execution of the hooks is different to the current model, as there be an additional setting on the `ClusterScanCompletionHook` called namespaceMode which allows users to configure to either run the hook in the same namespace as the scan it is getting executed for, or to provide a fixed namespace where all executions for this `ClusterScanCompletionHook` will be run in.
82+
The execution of the hooks is different to the current model, as there must be an additional setting on the `ClusterScanCompletionHook` called namespaceMode which allows users to configure to either run the hook in the same namespace as the scan it is getting executed for, or to provide a fixed namespace where all executions for this `ClusterScanCompletionHook` will be run in.
8383

8484
Example `ClusterScanCompletionHook` specs with this being set:
8585

@@ -107,12 +107,12 @@ spec:
107107
108108
This fixed namespace mode has been added to the `ClusterScanCompletionHook` but not for the `ClusterScanType` for the following reasons:
109109

110-
1. Hooks are usually used to interface with 3rd party systems which are often standardized for the entire company. Allowing to run them in a namespace separate from the individual teams / scans allows the hook access these systems without having to distribute and thus potentially compromise the credentials for the 3rd party systems.
111-
2. In contrast to scans, hooks usually do not need to send requests to the scanned applications. Scanning a app from outside their namespace opens up a lot of potential issues with network policies blocking inter network traffic between scanner and app. As the hooks only need to access the S3 bucket and the 3rd party system they are interfacing with, this is not a issue for hooks.
110+
1. Hooks are usually used to interface with 3rd party systems which are often standardized for the entire company. Allowing to run them in a namespace separate from the individual teams / scans allows the hook to access these systems without having to distribute and thus potentially compromise the credentials for the 3rd party systems.
111+
2. In contrast to scans, hooks usually do not need to send requests to the scanned applications. Scanning an app from outside its namespace opens up a lot of potential issues with network policies blocking inter network traffic between scanner and app. As the hooks only need to access the S3 bucket and the 3rd party system they are interfacing with, this is not an issue for hooks.
112112

113113
#### Potential Issues With the Fixed `executionNamespace` Mode
114114

115-
Can lead to inter team conflicts if a scans of "team 1" fails because of a error in the `ClusterScanCompletionHook` managed by a central cluster team. Members of "team 1" will likely not even have access to the namespace the failing hooks run in and wont be able to debug / fix these issues by their self.
115+
Can lead to conflicts between teams if a scan of "team 1" fails because of an error in the `ClusterScanCompletionHook` managed by a "central cluster team". Members of "team 1" will likely not even have access to the namespace the failing hooks run in and won't be able to debug / fix these issues by themselves.
116116

117117
## Changes to Existing Helm Charts
118118

0 commit comments

Comments
 (0)