Feat/deployment config #718

ivanstanev · 2021-04-19T14:58:05Z

Tests written and linted ℹ︎
Documentation written ℹ︎
Commit history is tidy ℹ︎

What this does

Add support for DeploymentConfig on OpenShift.

Before we set up Informers in the cluster for all the different workloads (e.g. Deployment, ReplicaSet), specifically for DeploymentConfig we now do a small query to the API server to understand if it's supported. If the API request returns successfully (non-404 code) then DeploymentConfig is supported and we set up a Watch following the routes in the API reference. If the K8s API returns 404 (meaning the resource is unknown) then we skip creating a DeploymentConfig Informer.

What was added:

an integration test for OpenShift 4 that applies a DeploymentConfig and checks that the workload was scanned
TypeScript types for DeploymentConfigV1 based on the API reference
some functions are now async to allow querying the API server for supported workloads before deciding whether to set an Informer; we also await informer.start() calls since it returns a Promise

More information

Commit by commit 🙏

Jira ticket RUN-1505

package.json

src/supervisor/types.ts

src/supervisor/watchers/handlers/index.ts

dkontorovskyy · 2021-04-19T15:23:04Z

src/supervisor/watchers/handlers/index.ts

+}
+
+export async function setupInformer(namespace: string, workloadKind: WorkloadKind): Promise<void> {
+  const isSupported = await isSupportedWorkload(namespace, workloadKind);


Is workload support supposed to be cluster-wide check, not per ns?

💡
But it would complicate the code a bit, we now need to check this only once and we need to differentiate between snyk-monitor scanning the cluster or snyk-monitor scanning a single namespace (won't have permission to query the cluster if scanning a single namespace).

Opting not to do it because it would complicate the code quite a bit, LMK if you think it's a really bad idea and the benefits would outweigh the downsides 😉

dkontorovskyy · 2021-04-19T15:23:11Z

src/supervisor/watchers/handlers/index.ts

+    const resourceVersion = undefined; // List anything in the cluster
+    const timeoutSeconds = 10; // Don't block the snyk-monitor indefinitely
+    const attemptedApiCall = await k8sApi.customObjectsClient.listNamespacedCustomObject(
+      'apps.openshift.io',


nit: Why don't store this in an object and just reference GVK parts from the static map?

Could you share example code? Not sure what you mean

src/supervisor/watchers/handlers/index.ts

src/supervisor/watchers/handlers/types.ts

src/supervisor/watchers/handlers/index.ts

The snyk-monitor will do a best-effort attempt to set up a Watch/Informer for DeploymentConfigs in the apps.openshift.io/v1 API. If this does not succeed, scanning will proceed as normal while printing a warning.

This reverts the change in 07f4f34. DeploymentConfigs on OpenShift create ReplicationControllers which in turn create Pods. However, the ReplicationControllers in this case are not marked as controllers, hence we cannot walk the list of owner references and can never scan DeploymentConfigs. This change ensures we do not strictly check for controllers to allow us to read DeploymentConfigs.

github-actions · 2021-04-19T16:46:16Z

Expected release notes (by @ivanstanev)

features:
add DeploymentConfig support for OpenShift (3b9c842)

fixes:
revert checking for parent workloads being controllers (9c08163)
upgrade kubernetes/client-node to remove net-keepalive dependency (eae1eb7)
log the Promise for debugging when an unhandledRejections occur (d39d9ba)

I hereby acknowledge these release notes are 🥙 AWESOME 🥙

src/supervisor/watchers/handlers/deployment-config.ts

snyksec · 2021-04-21T11:29:45Z

🎉 This PR is included in version 1.51.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

fix: log the Promise for debugging when an unhandledRejections occur

d39d9ba

ivanstanev self-assigned this Apr 19, 2021

ivanstanev requested a review from a team as a code owner April 19, 2021 14:58