fix initial logic, update template to support -jmx tag, improve unit-test and documentation wording

Author: misteriaud

charts/datadog/README.md

@@ -906,17 +906,17 @@ helm install <RELEASE_NAME> \
 | existingClusterAgent.serviceName | string | `nil` | Existing service name to use for reaching the external Cluster Agent |
 | existingClusterAgent.tokenSecretName | string | `nil` | Existing secret name to use for external Cluster Agent token |
 | fips.customFipsConfig | object | `{}` | Configure a custom configMap to provide the FIPS configuration. Specify custom contents for the FIPS proxy sidecar container config (/etc/datadog-fips-proxy/datadog-fips-proxy.cfg). If empty, the default FIPS proxy sidecar container config is used. |
-| fips.enabled | bool | `false` |  |
+| fips.enabled | bool | `false` | Enable fips proxy sidecar. The fips-proxy method is being progressively phased out in favor of FIPS-compliant images (refer to the `useFIPSAgent` setting). |
 | fips.image.digest | string | `""` | Define the FIPS sidecar image digest to use, takes precedence over `fips.image.tag` if specified. |
 | fips.image.name | string | `"fips-proxy"` |  |
 | fips.image.pullPolicy | string | `"IfNotPresent"` | Datadog the FIPS sidecar image pull policy |
 | fips.image.repository | string | `nil` | Override default registry + image.name for the FIPS sidecar container. |
 | fips.image.tag | string | `"1.1.9"` | Define the FIPS sidecar container version to use. |
-| fips.local_address | string | `"127.0.0.1"` | Set local IP address This setting is only used for the fips-proxy sidecar. |
+| fips.local_address | string | `"127.0.0.1"` | Set local IP address. This setting is only used for the fips-proxy sidecar. |
 | fips.port | int | `9803` | Specifies which port is used by the containers to communicate to the FIPS sidecar. This setting is only used for the fips-proxy sidecar. |
-| fips.portRange | int | `15` | Specifies the number of ports used, defaults to 13 https://github.com/DataDog/datadog-agent/blob/7.44.x/pkg/config/config.go#L1564-L1577 This setting is only used for the fips-proxy sidecar. |
+| fips.portRange | int | `15` | Specifies the number of ports used, defaults to 13 https://github.com/DataDog/datadog-agent/blob/7.44.x/pkg/config/config.go#L1564-L1577. This setting is only used for the fips-proxy sidecar. |
 | fips.resources | object | `{}` | Resource requests and limits for the FIPS sidecar container. This setting is only used for the fips-proxy sidecar. |
-| fips.use_https | bool | `false` | Option to enable https This setting is only used for the fips-proxy sidecar. |
+| fips.use_https | bool | `false` | Option to enable https. This setting is only used for the fips-proxy sidecar. |
 | fullnameOverride | string | `nil` | Override the full qualified app name |
 | kube-state-metrics.image.repository | string | `"registry.k8s.io/kube-state-metrics/kube-state-metrics"` | Default kube-state-metrics image repository. |
 | kube-state-metrics.nodeSelector | object | `{"kubernetes.io/os":"linux"}` | Node selector for KSM. KSM only supports Linux. |
@@ -934,7 +934,7 @@ helm install <RELEASE_NAME> \
 | registry | string | `nil` | Registry to use for all Agent images (default to [gcr.io | eu.gcr.io | asia.gcr.io | datadoghq.azurecr.io | public.ecr.aws/datadog] depending on datadog.site value) |
 | remoteConfiguration.enabled | bool | `true` | Set to true to enable remote configuration on the Cluster Agent (if set) and the node agent. Can be overridden if `datadog.remoteConfiguration.enabled` Preferred way to enable Remote Configuration. |
 | targetSystem | string | `"linux"` | Target OS for this deployment (possible values: linux, windows) |
-| useFIPSAgent | bool | `false` |  |
+| useFIPSAgent | bool | `false` | Setting useFIPSAgent to true makes the helm chart use Agent images that are FIPS-compliant for use in GOVCLOUD environments Setting this to true disables the fips-proxy sidecar. This is the recommended method for enabling FIPS compliance. |
 
 ## Configuration options for Windows deployments
 <a name="windows-config"></a>

charts/datadog/templates/_helpers.tpl

@@ -363,7 +363,7 @@ Return a remote image path based on `.Values` (passed as root) and `.` (any `.im
 {{- $tagSuffix = printf "-%s" "fips" -}}
 {{- end -}}
 {{- if .image.tagSuffix -}}
-{{- $tagSuffix = printf "-%s" .image.tagSuffix -}}
+{{- $tagSuffix = printf "%s-%s" $tagSuffix .image.tagSuffix -}}
 {{- end -}}
 {{- if .image.repository -}}
 {{- .image.repository -}}:{{ .image.tag }}{{ $tagSuffix }}
@@ -426,7 +426,7 @@ false
 Return true if the fips side car container should be created.
 */}}
 {{- define "should-enable-fips-proxy" -}}
-{{- if and (not (or (eq (include "use-fips-images" $) "true") (or .Values.providers.gke.autopilot .Values.providers.gke.gdc ))) (eq .Values.targetSystem "linux") .Values.fips.enabled -}}
+{{- if and (not (or (eq (include "use-fips-images" .Values) "true") (or .Values.providers.gke.autopilot .Values.providers.gke.gdc ))) (eq .Values.targetSystem "linux") .Values.fips.enabled -}}
 true
 {{- else -}}
 false

charts/datadog/values.yaml

@@ -1575,17 +1575,15 @@ existingClusterAgent:
   # existingClusterAgent.clusterchecksEnabled -- set this to false if you don’t want the agents to run the cluster checks of the joined external cluster agent
   clusterchecksEnabled: true
 
-## useFIPSAgent -- Setting useFIPSAgent: true makes the helm chart install FIPS compliant image tags for use in GOVCLOUD environments
-## NOTE:
-##    - setting this to true disables the fips-proxy sidecar
-##    - this is the recommended method for enabling FIPS compliance
+# useFIPSAgent -- Setting useFIPSAgent to true makes the helm chart use Agent images that are FIPS-compliant for use in GOVCLOUD environments
+# Setting this to true disables the fips-proxy sidecar.
+# This is the recommended method for enabling FIPS compliance.
 useFIPSAgent: false
 
-## fips is used to enable and configure the FIPS compliant mode for the Datadog Agent.
-## The current method uses the fips-proxy sidecar to enable FIPS compliance.
-## The fips-proxy will be progressively deprecated in the future in favor of the use of FIPS compliant images (please refer to useFIPSAgent setting)
+## fips is used to enable and configure the fips-proxy sidecar.
 fips:
-  ## fips.enabled -- Enable fips proxy sidecar
+  # fips.enabled -- Enable fips proxy sidecar.
+  # The fips-proxy method is being progressively phased out in favor of FIPS-compliant images (refer to the `useFIPSAgent` setting).
   enabled: false
 
   # TODO: Option to override config of the FIPS side car: /etc/datadog-fips-proxy/datadog-fips-proxy.cfg
@@ -1595,11 +1593,11 @@ fips:
   # This setting is only used for the fips-proxy sidecar.
   port: 9803
 
-  # fips.portRange -- Specifies the number of ports used, defaults to 13 https://github.com/DataDog/datadog-agent/blob/7.44.x/pkg/config/config.go#L1564-L1577
+  # fips.portRange -- Specifies the number of ports used, defaults to 13 https://github.com/DataDog/datadog-agent/blob/7.44.x/pkg/config/config.go#L1564-L1577.
   # This setting is only used for the fips-proxy sidecar.
   portRange: 15
 
-  # fips.use_https -- Option to enable https
+  # fips.use_https -- Option to enable https.
   # This setting is only used for the fips-proxy sidecar.
   use_https: false
 
@@ -1613,12 +1611,11 @@ fips:
     #   cpu: 20m
     #   memory: 64Mi
 
-  # fips.local_address -- Set local IP address
+  # fips.local_address -- Set local IP address.
   # This setting is only used for the fips-proxy sidecar.
   local_address: "127.0.0.1"
 
   ## Define the Datadog image to work with
-  # This setting is only used for the fips-proxy sidecar.
   image:
     ## fips.image.name -- Define the FIPS sidecar container image name.
     name: fips-proxy
@@ -1639,7 +1636,6 @@ fips:
 
   ## Note: Use `|` to declare multi-line configuration.
   ## ref: https://docs.datadoghq.com/agent/guide/agent-fips-proxy
-  # This setting is only used for the fips-proxy sidecar.
   customFipsConfig: {}  # |
   #  foobar
   #     foo bar baz

test/datadog/fips_mode_test.go

@@ -5,59 +5,73 @@ import (
 	"strings"
 	"testing"
 
+	"strconv"
+
 	"github.com/DataDog/helm-charts/test/common"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
-	"strconv"
 )
 
 func TestFIPSModeConditions(t *testing.T) {
 	tests := []struct {
-		name                   string
-		setFipsEnabledSetting  bool
-		setUseFipsImageSetting bool
-		expectFipsProxy        bool
-		expectFipsImageSuffix  bool
+		name            string
+		enableFIPSProxy bool
+		enableFIPSAgent bool
+		expectFIPSProxy bool
+		expectFIPSAgent bool
+		enableJMX       bool
 	}{
 		{
-			name:                   "fips.useFipsImages true should not use fips-proxy and use fips image",
-			setFipsEnabledSetting:  true,
-			setUseFipsImageSetting: true,
-			expectFipsProxy:        false,
-			expectFipsImageSuffix:  true,
+			name:            "without any fips configuration",
+			enableFIPSProxy: false,
+			enableFIPSAgent: false,
+			expectFIPSProxy: false,
+			expectFIPSAgent: false,
+		},
+		{
+			name:            "fips proxy only",
+			enableFIPSProxy: true,
+			enableFIPSAgent: false,
+			expectFIPSProxy: true,
+			expectFIPSAgent: false,
 		},
 		{
-			name:                   "fips.useFipsImages false and fips.enabled true should use fips-proxy and not use fips image",
-			setFipsEnabledSetting:  true,
-			setUseFipsImageSetting: false,
-			expectFipsProxy:        true,
-			expectFipsImageSuffix:  false,
+			name:            "fips image only",
+			enableFIPSProxy: false,
+			enableFIPSAgent: true,
+			expectFIPSProxy: false,
+			expectFIPSAgent: true,
 		},
 		{
-			name:                   "fips.useFipsImages false and fips.enabled false should not use fips-proxy or fips image",
-			setFipsEnabledSetting:  false,
-			setUseFipsImageSetting: true,
-			expectFipsProxy:        false,
-			expectFipsImageSuffix:  false,
+			name:            "fips proxy and fips image",
+			enableFIPSProxy: true,
+			enableFIPSAgent: true,
+			expectFIPSProxy: false, // fips proxy should be disabled when fips agent is enabled
+			expectFIPSAgent: true,
 		},
 		{
-			name:                   "fips.useFipsImages false and fips.enabled false should not use fips-proxy or fips image",
-			setFipsEnabledSetting:  false,
-			setUseFipsImageSetting: true,
-			expectFipsProxy:        false,
-			expectFipsImageSuffix:  false,
+			name:            "fips image with JMX enabled",
+			enableFIPSProxy: false,
+			enableFIPSAgent: true,
+			expectFIPSProxy: false,
+			expectFIPSAgent: true,
+			enableJMX:       true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			values := map[string]string{
-				"fips.useFipsImages": strconv.FormatBool(tt.setUseFipsImageSetting),
-				"fips.enabled":       strconv.FormatBool(tt.setFipsEnabledSetting),
+				"useFIPSAgent":                 strconv.FormatBool(tt.enableFIPSAgent),
+				"fips.enabled":                 strconv.FormatBool(tt.enableFIPSProxy),
 				"datadog.apiKeyExistingSecret": "datadog-secret",
-	            "datadog.appKeyExistingSecret": "datadog-secret",
+				"datadog.appKeyExistingSecret": "datadog-secret",
+			}
+
+			if tt.enableJMX {
+				values["agents.image.tagSuffix"] = "jmx"
 			}
 
 			manifest, err := common.RenderChart(t, common.HelmCommand{
@@ -81,15 +95,41 @@ func TestFIPSModeConditions(t *testing.T) {
 			// Check FIPS proxy setting
 			if value, ok := configMap.Data["should-enable-fips-proxy"]; ok {
 				fmt.Printf("should-enable-fips-proxy: %s\n", value)
-				assert.Equal(t, tt.expectFipsProxy, value == "true", "should-enable-fips-proxy value is incorrect")
+				assert.Equal(t, tt.expectFIPSProxy, value == "true", "should-enable-fips-proxy value is incorrect")
 			}
 
-			// Check FIPS image suffix
-			for _, container := range daemonSet.Spec.Template.Spec.Containers {
-				fmt.Printf("container.Image: %s\n", container.Image)
-				assert.Equal(t, tt.expectFipsImageSuffix, strings.Contains(container.Image, "-fips"), "fips image suffix is incorrect")
-			}
+			// Checking that daemonSet contains or not fips-proxy container based on the fips proxy configuration
+			checkFIPSProxy(t, daemonSet.Spec.Template.Spec.Containers, tt.expectFIPSProxy)
 
+			// Checking that all containers have the fips image suffix if fips agent is enabled
+			checkFIPSImage(t, daemonSet.Spec.Template.Spec.Containers, tt.expectFIPSAgent)
 		})
 	}
 }
+
+func checkFIPSProxy(t *testing.T, containers []corev1.Container, expectFIPSProxy bool) {
+	hasFIPSProxy := false
+	for _, container := range containers {
+		if strings.Contains(container.Image, "fips-proxy") {
+			hasFIPSProxy = true
+			break
+		}
+	}
+	if expectFIPSProxy {
+		require.True(t, hasFIPSProxy, "fips proxy container should be present")
+	} else {
+		require.False(t, hasFIPSProxy, "fips proxy container should not be present")
+	}
+}
+
+func checkFIPSImage(t *testing.T, containers []corev1.Container, expectFIPSImage bool) {
+	if expectFIPSImage {
+		for _, container := range containers {
+			require.Contains(t, container.Image, "-fips", fmt.Sprintf("fips container %s should have the fips image suffix: %s", container.Name, container.Image))
+		}
+	} else {
+		for _, container := range containers {
+			require.NotContains(t, container.Image, "-fips", fmt.Sprintf("fips container %s should not have the fips image suffix: %s", container.Name, container.Image))
+		}
+	}
+}