Repository: sse-secure-systems/connaisseur
Pull request #1608 (2024-05-10 10:50:59)

feat: Allow to configure whether to cache errors

## Description

This PR adds configuration options to decide whether to cache errors

## Checklist
<!--- Mark as done if a point is not necessary. Feel free to reach out if help on any items in the checklist is needed. -->

- [x] PR is rebased to/aimed at branch `develop`
- [x] PR follows [Contributing Guide](https://github.com/sse-secure-systems/connaisseur/blob/master/docs/CONTRIBUTING.md)
- [x] Added tests (if necessary)
- [x] Extended README/Documentation (if necessary)
- [x] Adjusted versions of image and Helm chart in `Chart.yaml` (if necessary)



Patch:
diff --git a/charts/connaisseur/templates/env.yaml b/charts/connaisseur/templates/env.yaml
index 462a43be..17abd0fa 100644
--- a/charts/connaisseur/templates/env.yaml
+++ b/charts/connaisseur/templates/env.yaml
@@ -13,6 +13,7 @@ data:
   RESOURCE_VALIDATION_MODE: {{ print .resourceValidationMode | default "all" | quote }}
   {{- with .cache }}
   CACHE_EXPIRY_SECONDS: {{ print .expirySeconds | default 30 | quote }}
+  CACHE_ERRORS: {{ print .cacheErrors | default "true" | quote }}
   {{- end }}
   {{- end }}
   LOG_LEVEL: {{ .Values.application.logLevel | default "info" | quote }}
diff --git a/charts/connaisseur/values.yaml b/charts/connaisseur/values.yaml
index fa58c284..0ebaf93c 100644
--- a/charts/connaisseur/values.yaml
+++ b/charts/connaisseur/values.yaml
@@ -155,6 +155,7 @@ application:
     resourceValidationMode: "all" # 'all' or 'podsOnly'
     cache:
       expirySeconds: 30
+      cacheErrors: true
 
   logLevel: info
 
diff --git a/docs/features/caching.md b/docs/features/caching.md
index 9e977c47..5f61fc92 100644
--- a/docs/features/caching.md
+++ b/docs/features/caching.md
@@ -13,6 +13,7 @@ If set to 0, no caching will be performed and the cache will not be deployed as
 | Key | Default | Required | Description |
 | - | - | - | - |
 | `expirySeconds` | `30` | - | Number of seconds for which validation results are cached. If set to 0, the Connaisseur deployment will omit the caching infrastructure in its entirety. |
+| `cacheErrors` | `true` | - | Whether validation failures are cached. If set to false, Connaisseur will only cache successfully validated image digests instead of also caching errors. |
 
 ## Example
 
@@ -22,5 +23,6 @@ In `charts/connaisseur/values.yaml`:
 application:
   features:
     cache:
-      expirySeconds: 30
-```
\ No newline at end of file
+      expirySeconds: 15
+      cacheErrors: false
+```
diff --git a/internal/caching/caching.go b/internal/caching/caching.go
index 40185791..c24247af 100644
--- a/internal/caching/caching.go
+++ b/internal/caching/caching.go
@@ -20,7 +20,7 @@ type Cacher interface {
 }
 
 func NewCacher() Cacher {
-	configuredExpirySeconds, ok := os.LookupEnv(constants.CacheExpirySeconds)
+	configuredExpirySeconds, ok := os.LookupEnv(constants.CacheExpirySecondsKey)
 
 	var expirySeconds int64
 	if !ok {
@@ -41,3 +41,16 @@ func NewCacher() Cacher {
 
 	return NewRedis(time.Duration(expirySeconds) * time.Second)
 }
+
+func CacheErrors() bool {
+	configuredCacheErrors, ok := os.LookupEnv(constants.CacheErrorsKey)
+	defaultCacheErrors := true
+	if !ok {
+		return defaultCacheErrors
+	}
+	parsedCacheErrors, err := strconv.ParseBool(configuredCacheErrors)
+	if err != nil {
+		logrus.Warnf("Couldn't parse error caching configuration '%s', defaulting to %t", configuredCacheErrors, defaultCacheErrors)
+	}
+	return parsedCacheErrors
+}
diff --git a/internal/constants/constants.go b/internal/constants/constants.go
index b344a942..e4b47254 100644
--- a/internal/constants/constants.go
+++ b/internal/constants/constants.go
@@ -23,7 +23,8 @@ const (
 	DefaultDockerRegistry      = "index.docker.io"
 	DefaultRedisPort           = 6379
 	DefaultCacheExpirySeconds  = 30
-	CacheExpirySeconds         = "CACHE_EXPIRY_SECONDS"
+	CacheExpirySecondsKey      = "CACHE_EXPIRY_SECONDS"
+	CacheErrorsKey             = "CACHE_ERRORS"
 	EmptyAuthRegistry          = "EMPTYAUTH"
 )
 
diff --git a/internal/handler/validation/validation.go b/internal/handler/validation/validation.go
index ad308eaa..e5624a54 100644
--- a/internal/handler/validation/validation.go
+++ b/internal/handler/validation/validation.go
@@ -235,7 +235,12 @@ func ValidateImage(ctx context.Context, in ValidationInput, out chan<- Validatio
 	// cache digest or error
 	if cacheErr != "" || img.Digest() != "" {
 		if cacheErr != "" {
-			img.SetDigest("") // delete digest for clearer caching entries
+			// don't cache errors, if so configured
+			if !caching.CacheErrors() {
+				return
+			}
+			// otherwise, delete digest for clearer caching entries
+			img.SetDigest("")
 		}
 
 		err = cache.Set(


Test patch:
diff --git a/internal/caching/caching_test.go b/internal/caching/caching_test.go
index 936a1df7..02795f03 100644
--- a/internal/caching/caching_test.go
+++ b/internal/caching/caching_test.go
@@ -17,13 +17,32 @@ func TestNewCacherDefault(t *testing.T) {
 }
 
 func TestNewCacherCachingDisabled(t *testing.T) {
-	t.Setenv(constants.CacheExpirySeconds, "0")
+	t.Setenv(constants.CacheExpirySecondsKey, "0")
 	c := NewCacher()
 	assert.IsType(t, EmptyCache{}, c)
 }
 
 func TestNewCacherInvalidConfig(t *testing.T) {
-	t.Setenv(constants.CacheExpirySeconds, "not a number")
+	t.Setenv(constants.CacheExpirySecondsKey, "not a number")
 	c := NewCacher()
 	assert.IsType(t, Redis{}, c)
 }
+
+func TestCacheErrorsDefault(t *testing.T) {
+	assert.True(t, CacheErrors())
+}
+
+func TestCacheErrorsTrue(t *testing.T) {
+	t.Setenv(constants.CacheErrorsKey, "true")
+	assert.True(t, CacheErrors())
+}
+
+func TestCacheErrorsFalse(t *testing.T) {
+	t.Setenv(constants.CacheErrorsKey, "0")
+	assert.False(t, CacheErrors())
+}
+
+func TestCacheErrorsInvalid(t *testing.T) {
+	t.Setenv(constants.CacheErrorsKey, "not a bool")
+	assert.True(t, CacheErrors())
+}
diff --git a/internal/handler/validation/validation_test.go b/internal/handler/validation/validation_test.go
index 057dea23..8be2f423 100644
--- a/internal/handler/validation/validation_test.go
+++ b/internal/handler/validation/validation_test.go
@@ -422,6 +422,55 @@ func TestValidateImageCancelledContext(t *testing.T) {
 	assert.Equal(t, float64(1), metric)
 }
 
+func TestValidateImageCachingBevhaiour(t *testing.T) {
+	cfg, _ := config.Load(PRE, "10_mutate_test.yaml")
+	cache := testhelper.MockCache(t, []testhelper.KeyValuePair{})
+	defer cache.Close()
+
+	cfg.Validators = append(cfg.Validators, validator.Validator{
+		Name:              "mock",
+		Type:              "mock",
+		SpecificValidator: testhelper.MockValidator{},
+	})
+
+	ctx := context.Background()
+	ctx = context.WithValue(ctx, constants.ConnaisseurConfig, cfg)
+	ctx = context.WithValue(ctx, constants.Cache, cache)
+
+	// Normally, errors will be cached
+	cachedOutputChannel := make(chan ValidationOutput, 1)
+	ValidateImage(
+		ctx,
+		ValidationInput{
+			IdxsTypes:        []kubernetes.IdxType{{Index: 0, Type: "containers"}},
+			Image:            "will-be-denied-1",
+			ParentImagesFunc: func(ctx context.Context) []string { return []string{} },
+		},
+		cachedOutputChannel,
+	)
+	<-cachedOutputChannel
+	cached, err := cache.Get(ctx, "will-be-denied-1")
+	assert.NotEmpty(t, cached)
+	assert.Nil(t, err)
+
+	// Set option to not cache errors
+	t.Setenv(constants.CacheErrorsKey, strconv.FormatBool(false))
+	nonCachedOutputChannel := make(chan ValidationOutput, 1)
+	ValidateImage(
+		ctx,
+		ValidationInput{
+			IdxsTypes:        []kubernetes.IdxType{{Index: 0, Type: "containers"}},
+			Image:            "will-be-denied-2",
+			ParentImagesFunc: func(ctx context.Context) []string { return []string{} },
+		},
+		nonCachedOutputChannel,
+	)
+	<-nonCachedOutputChannel
+	cached, err = cache.Get(ctx, "will-be-denied-2")
+	assert.Empty(t, cached)
+	assert.NotNil(t, err)
+}
+
 func metricValue(name, label1, label2, label3 string) (float64, error) {
 	var m = &dto.Metric{}
 	switch name {