fix: address CodeRabbit review findings

.github/workflows/ci.yml

@@ -68,7 +68,8 @@ jobs:
       - name: Check Generated Code
         run: |
           go generate ./...
-          git diff --exit-code || (echo "Generated code is out of date. Run 'go generate ./...' and commit the changes." && exit 1)
+          test -z "$(git status --porcelain --untracked-files=all)" || \
+            (git status --short && echo "Generated code is out of date. Run 'go generate ./...' and commit the changes." && exit 1)
 
       - name: Run Unit Tests
         run: go test -v -cover ./...
@@ -96,7 +97,6 @@ jobs:
         with:
           buildkitd-flags: --debug
           version: ${{ env.DOCKER_BUILDX_VERSION }}
-          install: true
 
       - name: Checkout
         uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1

.golangci.yml

@@ -165,7 +165,6 @@ linters:
           - gochecknoinits
           - gocognit
           - gosec
-          - scopelint
           - unparam
           - embeddedstructfieldcheck
         path: _test(ing)?\.go

README.md

@@ -21,6 +21,7 @@ returns the output to specified fields.
 - [Parameters](#parameters)
 - [Error Handling and Output Capture](#error-handling-and-output-capture)
 - [Caching Function Outputs](#caching-function-outputs)
+- [Security Considerations](#security-considerations)
 - [Running as Arbitrary User IDs](#running-as-arbitrary-user-ids)
 - [Examples](#examples)
 - [Development and Test](#development-and-test)
@@ -154,6 +155,38 @@ set a new cache duration for the output.
 
 See the echo [composition.yaml](example/echo/composition.yaml) for an example.
 
+## Security Considerations
+
+### Log Sensitivity
+
+When running with `--debug` enabled, function-shell logs shell commands and their output. This can expose sensitive data in logs if:
+
+- **Shell commands contain secrets**: Environment variables with sensitive values that are interpolated into commands will appear in debug logs
+- **Command output contains secrets**: stdout/stderr from commands may contain API responses, credentials, or other sensitive data
+
+**Recommendations:**
+
+1. **Avoid interpolating secrets directly in commands** - Use environment variables instead of embedding secrets in `shellCommand`
+2. **Review log retention policies** - Ensure Kubernetes log retention and access controls are appropriate for your security requirements
+3. **Use debug mode sparingly in production** - Only enable `--debug` when actively troubleshooting
+4. **Consider log aggregation security** - If forwarding logs to external systems, ensure they have appropriate access controls
+
+Example of safer secret handling:
+
+```yaml
+# Less safe: secret visible in debug logs
+shellCommand: 'curl -H "Authorization: Bearer my-secret-token" https://api.example.com'
+
+# Safer: secret passed via environment variable
+shellEnvVars:
+  - key: API_TOKEN
+    fieldRef:
+      path: spec.credentials.token
+shellCommand: 'curl -H "Authorization: Bearer ${API_TOKEN}" https://api.example.com'
+```
+
+Note: Even with environment variables, the command output (stdout/stderr) is logged at Info level for observability. Ensure your commands don't echo sensitive data unnecessarily.
+
 ## Running as Arbitrary User IDs
 
 Some Kubernetes environments (OpenShift, restricted Pod Security Standards) force containers to run as arbitrary user IDs that don't exist in `/etc/passwd`. This can cause tools like AWS CLI, git, and ssh to fail because they can't look up the current user.

env.go

@@ -34,7 +34,7 @@ func fromFieldRef(req *fnv1.RunFunctionRequest, fieldRef v1alpha1.FieldRef) (str
 		return "", errors.New("path must be set")
 	}
 	// Check for context key presence and capture context key and path
-	contextRegex := regexp.MustCompile(`^context\[(.+?)].(.+)$`)
+	contextRegex := regexp.MustCompile(`^context\[(.+?)]\.(.+)$`)
 	if match := contextRegex.FindStringSubmatch(fieldRef.Path); match != nil {
 		if v, ok := request.GetContextKey(req, match[1]); ok {
 			context := &unstructured.Unstructured{}

example/datadog-dashboard-ids/functions.yaml

@@ -4,7 +4,7 @@ kind: Function
 metadata:
   name: function-shell
   annotations:
-    # This tells crossplane beta render to connect to the functi    on locally.
+    # This tells crossplane beta render to connect to the function locally.
     render.crossplane.io/runtime: Development
 spec:
   # This is ignored when using the Development runtime.

fn.go

@@ -113,9 +113,13 @@ func (f *Function) RunFunction(_ context.Context, req *fnv1.RunFunctionRequest)
 			}
 			shellEnvVars[envVar.Key] = envValue
 		case v1alpha1.ShellEnvVarTypeFieldRef:
+			if envVar.FieldRef == nil {
+				response.Fatal(rsp, errors.Errorf("shellEnvVars: fieldRef must be set for key %s", envVar.Key))
+				return rsp, nil
+			}
 			envValue, err := fromFieldRef(req, *envVar.FieldRef)
 			if err != nil {
-				response.Fatal(rsp, errors.Wrapf(err, "cannot process contents of fieldRef %s", envVar.ValueRef))
+				response.Fatal(rsp, errors.Wrapf(err, "cannot process contents of fieldRef for key %s", envVar.Key))
 				return rsp, nil
 			}
 			shellEnvVars[envVar.Key] = envValue
@@ -150,7 +154,12 @@ func (f *Function) RunFunction(_ context.Context, req *fnv1.RunFunctionRequest)
 	sout := strings.TrimSpace(stdout.String())
 	serr := strings.TrimSpace(stderr.String())
 
-	log.Info("Function output", "tag", req.GetMeta().GetTag(), "stdout", sout, "stderr", serr)
+	// Log output at appropriate level: errors at Info for visibility, success at Debug
+	if cmderr != nil || serr != "" {
+		log.Info("Function output", "tag", req.GetMeta().GetTag(), "stdout", sout, "stderr", serr)
+	} else {
+		log.Debug("Function output", "tag", req.GetMeta().GetTag(), "stdout", sout, "stderr", serr)
+	}
 
 	err = dxr.Resource.SetValue(stdoutField, sout)
 	if err != nil {
@@ -172,7 +181,9 @@ func (f *Function) RunFunction(_ context.Context, req *fnv1.RunFunctionRequest)
 			msg := fmt.Sprintf("shellCmd %q for %q failed with %s", shellCmd, oxr.Resource.GetKind(), exiterr.Stderr)
 			response.Fatal(rsp, errors.Wrap(cmderr, msg))
 		}
+		return rsp, nil
 	}
 
+	log.Info("Function completed successfully", "tag", req.GetMeta().GetTag())
 	return rsp, nil
 }

fn_test.go

@@ -20,6 +20,7 @@ import (
 const (
 	testTagHello = "hello"
 	testTagFoo   = "foo"
+	testKeyTag   = "tag"
 )
 
 type logEntry struct {
@@ -93,7 +94,7 @@ func TestRunFunctionLogs(t *testing.T) {
 		want   want
 	}{
 		"LogsStdoutOnSuccess": {
-			reason: "Function output log should contain stdout key and empty stderr",
+			reason: "Function output log should be at debug level on success with no stderr",
 			args: args{
 				req: &fnv1.RunFunctionRequest{
 					Meta: &fnv1.RequestMeta{Tag: testTagFoo},
@@ -107,9 +108,10 @@ func TestRunFunctionLogs(t *testing.T) {
 			},
 			want: want{
 				logs: []wantLog{
-					{level: "info", msg: "Running function", kvs: map[string]any{"tag": testTagFoo}},
+					{level: "info", msg: "Running function", kvs: map[string]any{testKeyTag: testTagFoo}},
 					{level: "debug", msg: "Executing shell command", kvs: map[string]any{"cmd": "echo hello"}},
-					{level: "info", msg: "Function output", kvs: map[string]any{"stdout": "hello", "stderr": "", "tag": testTagFoo}},
+					{level: "debug", msg: "Function output", kvs: map[string]any{"stdout": "hello", "stderr": "", "tag": testTagFoo}},
+					{level: "info", msg: "Function completed successfully", kvs: map[string]any{testKeyTag: testTagFoo}},
 				},
 			},
 		},
@@ -129,7 +131,7 @@ func TestRunFunctionLogs(t *testing.T) {
 			},
 			want: want{
 				logs: []wantLog{
-					{level: "info", msg: "Running function", kvs: map[string]any{"tag": testTagFoo}},
+					{level: "info", msg: "Running function", kvs: map[string]any{testKeyTag: testTagFoo}},
 					{level: "debug", msg: "Executing shell command", kvs: map[string]any{"cmd": "echo hello> /dev/stderr; exit 1"}},
 					{level: "info", msg: "Function output", kvs: map[string]any{"stdout": "", "stderr": "hello", "tag": testTagFoo}},
 				},
@@ -333,7 +335,7 @@ func TestRunFunction(t *testing.T) {
 					Results: []*fnv1.Result{
 						{
 							Severity: fnv1.Severity_SEVERITY_FATAL,
-							Message:  "shellCmd \"set -euo pìpefail\" for \"\" failed with : exit status 2",
+							Message:  "shellCmd.*pìpefail.*failed.*exit status.*",
 							Target:   fnv1.Target_TARGET_COMPOSITE.Enum(),
 						},
 					},
@@ -377,7 +379,7 @@ func TestRunFunction(t *testing.T) {
 					Results: []*fnv1.Result{
 						{
 							Severity: fnv1.Severity_SEVERITY_FATAL,
-							Message:  "shellCmd unknown-shell-command for failed: exit status 127",
+							Message:  "shellCmd.*unknown-shell-command.*failed.*exit status 127",
 							Target:   fnv1.Target_TARGET_COMPOSITE.Enum(),
 						},
 					},
@@ -618,6 +620,33 @@ func TestRunFunction(t *testing.T) {
 				},
 			},
 		},
+		"ResponseIsErrorWhenFieldRefTypeWithNilFieldRef": {
+			reason: "The Function should return an error when type is FieldRef but fieldRef is nil",
+			args: args{
+				req: &fnv1.RunFunctionRequest{
+					Meta: &fnv1.RequestMeta{Tag: testTagHello},
+					Input: resource.MustStructJSON(`{
+						"apiVersion": "template.fn.crossplane.io/v1alpha1",
+						"kind": "Parameters",
+						"shellEnvVars": [{"key": "TEST_ENV_VAR", "type": "FieldRef"}],
+						"shellCommand": "echo ${TEST_ENV_VAR}",
+						"stdoutField": "spec.atFunction.shell.stdout"
+					}`),
+				},
+			},
+			want: want{
+				rsp: &fnv1.RunFunctionResponse{
+					Meta: &fnv1.ResponseMeta{Tag: testTagHello, Ttl: durationpb.New(response.DefaultTTL)},
+					Results: []*fnv1.Result{
+						{
+							Severity: fnv1.Severity_SEVERITY_FATAL,
+							Message:  "shellEnvVars: fieldRef must be set for key TEST_ENV_VAR",
+							Target:   fnv1.Target_TARGET_COMPOSITE.Enum(),
+						},
+					},
+				},
+			},
+		},
 		"ResponseWithCustomCacheTTL": {
 			reason: "The Function should set custom TTL when cacheTTL is specified",
 			args: args{
@@ -695,7 +724,10 @@ func TestRunFunction(t *testing.T) {
 			rsp, err := f.RunFunction(tc.args.ctx, tc.args.req)
 
 			var cmpOpts []cmp.Option
-			cmpOpts = append(cmpOpts, protocmp.Transform(), protocmp.IgnoreFields(&fnv1.Result{}, "message"))
+			cmpOpts = append(cmpOpts, protocmp.Transform())
+			if !tc.args.useRegex {
+				cmpOpts = append(cmpOpts, protocmp.IgnoreFields(&fnv1.Result{}, "message"))
+			}
 
 			if tc.args.useRegex {
 				cmpOpts = append(cmpOpts, cmp.Comparer(func(expected, actual string) bool {

input/v1alpha1/parameters.go

@@ -76,6 +76,7 @@ type ShellEnvVar struct {
 	// FieldRef is a reference to a field in the Composition.
 	FieldRef *FieldRef `json:"fieldRef,omitempty"`
 	// Type is the type of ShellEnVar: Value, ValueRef, FieldRef.
+	// +kubebuilder:validation:Enum=Value;ValueRef;FieldRef
 	Type ShellEnvVarType `json:"type,omitempty"`
 }
 
@@ -119,6 +120,7 @@ const FieldRefDefault = ""
 // FieldRef refers to a composite field like spec.region.
 type FieldRef struct {
 	// Path is the field path of the field being referenced, i.e. spec.myfield, status.output
+	// +kubebuilder:validation:MinLength=1
 	Path string `json:"path"`
 	// Policy when the field is not available. If set to "Required" will return
 	// an error if a field is missing. If set to "Optional" will return DefaultValue.
@@ -128,6 +130,6 @@ type FieldRef struct {
 	Policy FieldRefPolicy `json:"policy,omitempty"`
 	// DefaultValue when Policy is Optional and field is not available defaults to ""
 	// +optional
-	// +kbuebuilder:default:=""
+	// +kubebuilder:default:=""
 	DefaultValue string `json:"defaultValue,omitempty"`
 }

package/input/template.fn.crossplane.io_parameters.yaml

@@ -61,12 +61,14 @@ spec:
                   description: FieldRef is a reference to a field in the Composition.
                   properties:
                     defaultValue:
+                      default: ""
                       description: DefaultValue when Policy is Optional and field
                         is not available defaults to ""
                       type: string
                     path:
                       description: Path is the field path of the field being referenced,
                         i.e. spec.myfield, status.output
+                      minLength: 1
                       type: string
                     policy:
                       default: Required
@@ -85,6 +87,10 @@ spec:
                   type: string
                 type:
                   description: 'Type is the type of ShellEnVar: Value, ValueRef, FieldRef.'
+                  enum:
+                  - Value
+                  - ValueRef
+                  - FieldRef
                   type: string
                 value:
                   description: Value is a fixed value, like http://api.example.com