resolving comments, adding nil check and tests

Signed-off-by: Nick Beenham <1985327+superbeeny@users.noreply.github.com>

pkg/corerp/api/v20231001preview/container_conversion.go

@@ -153,13 +153,12 @@ func (src *ContainerResource) ConvertTo() (v1.DataModelInterface, error) {
 			Resource:   to.String(src.Properties.Identity.Resource),
 		}
 	}
-
 	return converted, nil
 }
 
 // toEnvironmentVariableDataModel: Converts from versioned datamodel to base datamodel
 func toEnvironmentVariableDataModel(e map[string]*EnvironmentVariable) (map[string]datamodel.EnvironmentVariable, error) {
-	m := map[string]datamodel.EnvironmentVariable{}
+	environmentVariableMap := map[string]datamodel.EnvironmentVariable{}
 
 	for key, val := range e {
 		if val == nil {
@@ -170,12 +169,17 @@ func toEnvironmentVariableDataModel(e map[string]*EnvironmentVariable) (map[stri
 			return nil, v1.NewClientErrInvalidRequest(fmt.Sprintf("Environment variable %s has both value and secret value", key))
 		}
 
+		// An environment variable must have either value(Value) or secret value(ValueFrom)
+		if val.Value == nil && val.ValueFrom == nil {
+			return nil, v1.NewClientErrInvalidRequest(fmt.Sprintf("Environment variable %s has neither value nor secret value", key))
+		}
+
 		if val.Value != nil {
-			m[key] = datamodel.EnvironmentVariable{
+			environmentVariableMap[key] = datamodel.EnvironmentVariable{
 				Value: val.Value,
 			}
 		} else {
-			m[key] = datamodel.EnvironmentVariable{
+			environmentVariableMap[key] = datamodel.EnvironmentVariable{
 				ValueFrom: &datamodel.EnvironmentVariableReference{
 					SecretRef: &datamodel.EnvironmentVariableSecretReference{
 						Source: to.String(val.ValueFrom.SecretRef.Source),
@@ -187,32 +191,31 @@ func toEnvironmentVariableDataModel(e map[string]*EnvironmentVariable) (map[stri
 		}
 
 	}
-	return m, nil
+	return environmentVariableMap, nil
 }
 
 // fromEnvironmentVariableDataModel: Converts from base datamodel to versioned datamodel
 func fromEnvironmentVariableDataModel(e map[string]datamodel.EnvironmentVariable) map[string]*EnvironmentVariable {
-	m := map[string]*EnvironmentVariable{}
+	environmentVariableMap := map[string]*EnvironmentVariable{}
 
 	for key, val := range e {
 		if val.Value != nil {
-			m[key] = &EnvironmentVariable{
+			environmentVariableMap[key] = &EnvironmentVariable{
 				Value: val.Value,
 			}
-		} else {
-			m[key] = &EnvironmentVariable{
+		} else if val.ValueFrom != nil {
+			environmentVariableMap[key] = &EnvironmentVariable{
 				ValueFrom: &EnvironmentVariableReference{
 					SecretRef: &SecretReference{
 						Source: to.Ptr(val.ValueFrom.SecretRef.Source),
 						Key:    to.Ptr(val.ValueFrom.SecretRef.Key),
 					},
 				},
 			}
-
 		}
 	}
 
-	return m
+	return environmentVariableMap
 }
 
 // ConvertFrom converts from version-agnostic datamodel to the versioned Container resource.

pkg/corerp/api/v20231001preview/container_conversion_test.go

@@ -61,6 +61,11 @@ func TestContainerConvertVersionedToDataModel(t *testing.T) {
 			err:      nil,
 			emptyExt: true,
 		},
+		{
+			filename: "containerresource-nil-env-variables.json",
+			err:      v1.NewClientErrInvalidRequest("Environment variable DB_USER has neither value nor secret value"),
+			emptyExt: false,
+		},
 	}
 
 	for _, tt := range conversionTests {

pkg/corerp/api/v20231001preview/testdata/containerresource-nil-env-variables.json

@@ -0,0 +1,80 @@
+{
+  "id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/radius-test-rg/providers/Applications.Core/containers/container0",
+  "name": "container0",
+  "type": "Applications.Core/containers",
+  "properties": {
+    "status": {
+      "outputResources": [
+        {
+          "id": "/planes/test/local/providers/Test.Namespace/testResources/test-resource"
+        }
+      ]
+    },
+    "provisioningState": "Succeeded",
+    "application": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Applications.Core/applications/app0",
+    "connections": {
+      "inventory": {
+        "source": "inventory_route_id",
+        "disableDefaultEnvVars": true,
+        "iam": {
+          "kind": "azure",
+          "roles": [
+            "read"
+          ]
+        }
+      }
+    },
+    "restartPolicy": "Always",
+    "container": {
+      "image": "ghcr.io/radius-project/webapptutorial-todoapp",
+      "livenessProbe": {
+        "kind": "tcp",
+        "failureThreshold": 5,
+        "initialDelaySeconds": 5,
+        "periodSeconds": 5,
+        "timeoutSeconds": 5,
+        "containerPort": 8080
+      },
+      "env": {
+        "DB_USER": { }
+      },
+      "command": [
+        "/bin/sh"
+      ],
+      "args": [
+        "-c",
+        "while true; do echo hello; sleep 10;done"
+      ],
+      "workingDir": "/app"
+    },
+    "identity": {
+      "kind": "azure.com.workload",
+      "oidcIssuer": "https://oidcuri/id",
+      "resource": "resourceid"
+    },
+    "extensions": [
+      {
+        "kind": "manualScaling",
+        "replicas": 2
+      },
+      {
+        "kind": "daprSidecar",
+        "appId": "app-id",
+        "appPort": 80,
+        "config": "config",
+        "protocol": "http"
+      },
+      {
+        "kind": "kubernetesMetadata",
+        "annotations": {
+          "prometheus.io/scrape": "true",
+          "prometheus.io/port": "80"
+        },
+        "labels": {
+          "foo/bar/team": "credit",
+          "foo/bar/contact": "radiususer"
+        }
+      }
+    ]
+  }
+}

pkg/corerp/renderers/container/render.go

@@ -640,10 +640,10 @@ func (r Renderer) makeDeployment(
 func convertEnvVar(key string, env datamodel.EnvironmentVariable, options renderers.RenderOptions) (corev1.EnvVar, error) {
 	if env.Value != nil {
 		return corev1.EnvVar{Name: key, Value: *env.Value}, nil
-	} else {
+	} else if env.ValueFrom != nil {
 		// There are two cases to handle here:
 		// 1. The value comes from a kubernetes secret
-		// 2. The value comes from a radius resource id.
+		// 2. The value comes from a Applications.Core/SecretStore resource id.
 
 		// If the value comes from a kubernetes secret, we'll reference it.
 		if strings.HasPrefix(env.ValueFrom.SecretRef.Source, "/") {
@@ -652,7 +652,8 @@ func convertEnvVar(key string, env datamodel.EnvironmentVariable, options render
 				return corev1.EnvVar{}, fmt.Errorf("failed to find source in dependencies: %s", env.ValueFrom.SecretRef.Source)
 			}
 
-			// The format may be <namespace>/<name> or <name>, as an example "default/my-secret" or "my-secret"
+			// The format may be <namespace>/<name> or <name>, as an example "default/my-secret" or "my-secret". We split the string on '/'
+			// and take the second part if the secret is namespace qualified.
 			var name string
 			if strings.Contains(secretStore.Properties.Resource, "/") {
 				parts := strings.Split(secretStore.Properties.Resource, "/")
@@ -691,6 +692,8 @@ func convertEnvVar(key string, env datamodel.EnvironmentVariable, options render
 			}, nil
 		}
 
+	} else {
+		return corev1.EnvVar{}, fmt.Errorf("failed to convert environment variable: %s, both value and valueFrom cannot be nil", key)
 	}
 }
 

pkg/corerp/renderers/container/render_test.go

@@ -436,6 +436,28 @@ func Test_Render_Basic(t *testing.T) {
 	require.Len(t, output.Resources, 4)
 }
 
+func Test_Render_WithInvalidEnvironmentVariables(t *testing.T) {
+	properties := datamodel.ContainerProperties{
+		BasicResourceProperties: rpv1.BasicResourceProperties{
+			Application: applicationResourceID,
+		},
+		Container: datamodel.Container{
+			Image: "someimage:latest",
+			Env: map[string]datamodel.EnvironmentVariable{
+				envVarName1: {},
+			},
+		},
+	}
+	resource := makeResource(properties)
+	dependencies := map[string]renderers.RendererDependency{}
+
+	ctx := testcontext.New(t)
+	renderer := Renderer{}
+	_, err := renderer.Render(ctx, resource, renderers.RenderOptions{Dependencies: dependencies})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "failed to convert environment variable: failed to convert environment variable: TEST_VAR_1, both value and valueFrom cannot be nil")
+}
+
 func Test_Render_WithCommandArgsWorkingDir(t *testing.T) {
 	properties := datamodel.ContainerProperties{
 		BasicResourceProperties: rpv1.BasicResourceProperties{