config: fix panic on nil value in headers name/value pair (#6425)

Found a bug during testing of v0.3.0 support with the Collector where the value passed in to the headers name/value pair list could be nil, causing a dereferencing error. This fixes that bug. --------- Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
open-telemetry · Dec 12, 2024 · 378d704 · 378d704
1 parent 1606707
commit 378d704
Show file tree

Hide file tree

Showing 8 changed files with 191 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -30,6 +30,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Removed the redundant handling of panic from the `HTML` function in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#6373)
 - The `code.function` attribute emitted by `go.opentelemetry.io/contrib/bridges/otelslog` now stores just the function name instead the package path-qualified function name. The `code.namespace` attribute now stores the package path. (#6415)
 - The `code.function` attribute emitted by `go.opentelemetry.io/contrib/bridges/otelzap` now stores just the function name instead the package path-qualified function name. The `code.namespace` attribute now stores the package path. (#6423)
+- Return an error for `nil` values when unmarshaling `NameStringValuePair` in `go.opentelemetry.io/contrib/config`. (#6425)
 
 <!-- Released section -->
 <!-- Don't change this section unless doing release -->

diff --git a/config/testdata/invalid_nil_name.json b/config/testdata/invalid_nil_name.json
@@ -0,0 +1,49 @@
+{
+    "file_format": "0.3",
+    "disabled": false,
+    "logger_provider": {
+        "processors": [
+            {
+                "batch": {
+                    "schedule_delay": 5000,
+                    "export_timeout": 30000,
+                    "max_queue_size": 2048,
+                    "max_export_batch_size": 512,
+                    "exporter": {
+                        "otlp": {
+                            "protocol": "http/protobuf",
+                            "endpoint": "http://localhost:4318/v1/logs",
+                            "certificate": "/app/cert.pem",
+                            "client_key": "/app/cert.pem",
+                            "client_certificate": "/app/cert.pem",
+                            "headers": [
+                                {
+                                    "name": "api-key",
+                                    "value": "1234"
+                                },
+                                {
+                                    "value": "nil-name"
+                                }
+                            ],
+                            "headers_list": "api-key=1234",
+                            "compression": "gzip",
+                            "timeout": 10000,
+                            "insecure": false
+                        }
+                    }
+                }
+            },
+            {
+                "simple": {
+                    "exporter": {
+                        "console": {}
+                    }
+                }
+            }
+        ],
+        "limits": {
+            "attribute_value_length_limit": 4096,
+            "attribute_count_limit": 128
+        }
+    }
+}
diff --git a/config/testdata/invalid_nil_name.yaml b/config/testdata/invalid_nil_name.yaml
@@ -0,0 +1,13 @@
+file_format: "0.3"
+disabled: false
+logger_provider:
+  processors:
+    - batch:
+        exporter:
+          otlp:
+            protocol: http/protobuf
+            endpoint: http://localhost:4318/v1/logs
+            headers:
+              - name: api-key
+                value: "1234"
+              - value: nil-name
diff --git a/config/testdata/invalid_nil_value.json b/config/testdata/invalid_nil_value.json
@@ -0,0 +1,49 @@
+{
+    "file_format": "0.3",
+    "disabled": false,
+    "logger_provider": {
+        "processors": [
+            {
+                "batch": {
+                    "schedule_delay": 5000,
+                    "export_timeout": 30000,
+                    "max_queue_size": 2048,
+                    "max_export_batch_size": 512,
+                    "exporter": {
+                        "otlp": {
+                            "protocol": "http/protobuf",
+                            "endpoint": "http://localhost:4318/v1/logs",
+                            "certificate": "/app/cert.pem",
+                            "client_key": "/app/cert.pem",
+                            "client_certificate": "/app/cert.pem",
+                            "headers": [
+                                {
+                                    "name": "api-key",
+                                    "value": "1234"
+                                },
+                                {
+                                    "name": "nil-value"
+                                }
+                            ],
+                            "headers_list": "api-key=1234",
+                            "compression": "gzip",
+                            "timeout": 10000,
+                            "insecure": false
+                        }
+                    }
+                }
+            },
+            {
+                "simple": {
+                    "exporter": {
+                        "console": {}
+                    }
+                }
+            }
+        ],
+        "limits": {
+            "attribute_value_length_limit": 4096,
+            "attribute_count_limit": 128
+        }
+    }
+}
diff --git a/config/testdata/invalid_nil_value.yaml b/config/testdata/invalid_nil_value.yaml
@@ -0,0 +1,13 @@
+file_format: "0.3"
+disabled: false
+logger_provider:
+  processors:
+    - batch:
+        exporter:
+          otlp:
+            protocol: http/protobuf
+            endpoint: http://localhost:4318/v1/logs
+            headers:
+              - name: api-key
+                value: "1234"
+              - name: nil-value
diff --git a/config/v0.3.0/config_json.go b/config/v0.3.0/config_json.go
@@ -112,18 +112,25 @@ func (j *NameStringValuePair) UnmarshalJSON(b []byte) error {
 	if err := json.Unmarshal(b, &raw); err != nil {
 		return err
 	}
-	if _, ok := raw["name"]; raw != nil && !ok {
-		return errors.New("field name in NameStringValuePair: required")
+	if _, ok := raw["name"]; !ok {
+		return errors.New("json: cannot unmarshal field name in NameStringValuePair required")
 	}
-	if _, ok := raw["value"]; raw != nil && !ok {
-		return errors.New("field value in NameStringValuePair: required")
+	if _, ok := raw["value"]; !ok {
+		return errors.New("json: cannot unmarshal field value in NameStringValuePair required")
 	}
-	type Plain NameStringValuePair
-	var plain Plain
-	if err := json.Unmarshal(b, &plain); err != nil {
-		return err
+	var name, value string
+	var ok bool
+	if name, ok = raw["name"].(string); !ok {
+		return errors.New("yaml: cannot unmarshal field name in NameStringValuePair must be string")
+	}
+	if value, ok = raw["value"].(string); !ok {
+		return errors.New("yaml: cannot unmarshal field value in NameStringValuePair must be string")
+	}
+
+	*j = NameStringValuePair{
+		Name:  name,
+		Value: &value,
 	}
-	*j = NameStringValuePair(plain)
 	return nil
 }
 

diff --git a/config/v0.3.0/config_test.go b/config/v0.3.0/config_test.go
@@ -405,6 +405,16 @@ func TestParseYAML(t *testing.T) {
 			wantErr: errors.New(`yaml: unmarshal errors:
   line 2: cannot unmarshal !!str ` + "`notabool`" + ` into bool`),
 		},
+		{
+			name:    "invalid nil name",
+			input:   "invalid_nil_name.yaml",
+			wantErr: errors.New(`yaml: cannot unmarshal field name in NameStringValuePair required`),
+		},
+		{
+			name:    "invalid nil value",
+			input:   "invalid_nil_value.yaml",
+			wantErr: errors.New(`yaml: cannot unmarshal field value in NameStringValuePair required`),
+		},
 		{
 			name:  "valid v0.2 config",
 			input: "v0.2.yaml",
@@ -429,6 +439,7 @@ func TestParseYAML(t *testing.T) {
 
 			got, err := ParseYAML(b)
 			if tt.wantErr != nil {
+				require.Error(t, err)
 				require.Equal(t, tt.wantErr.Error(), err.Error())
 			} else {
 				require.NoError(t, err)
@@ -459,6 +470,16 @@ func TestSerializeJSON(t *testing.T) {
 			input:   "invalid_bool.json",
 			wantErr: errors.New(`json: cannot unmarshal string into Go struct field Plain.disabled of type bool`),
 		},
+		{
+			name:    "invalid nil name",
+			input:   "invalid_nil_name.json",
+			wantErr: errors.New(`json: cannot unmarshal field name in NameStringValuePair required`),
+		},
+		{
+			name:    "invalid nil value",
+			input:   "invalid_nil_value.json",
+			wantErr: errors.New(`json: cannot unmarshal field value in NameStringValuePair required`),
+		},
 		{
 			name:    "valid v0.2 config",
 			input:   "v0.2.json",
@@ -480,6 +501,7 @@ func TestSerializeJSON(t *testing.T) {
 			err = json.Unmarshal(b, &got)
 
 			if tt.wantErr != nil {
+				require.Error(t, err)
 				require.Equal(t, tt.wantErr.Error(), err.Error())
 			} else {
 				require.NoError(t, err)

diff --git a/config/v0.3.0/config_yaml.go b/config/v0.3.0/config_yaml.go
@@ -4,6 +4,7 @@
 package config // import "go.opentelemetry.io/contrib/config/v0.3.0"
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 )
@@ -30,6 +31,33 @@ func (j *AttributeNameValueType) UnmarshalYAML(unmarshal func(interface{}) error
 	return nil
 }
 
+// UnmarshalYAML implements yaml.Unmarshaler.
+func (j *NameStringValuePair) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	var raw map[string]interface{}
+	if err := unmarshal(&raw); err != nil {
+		return err
+	}
+	if _, ok := raw["name"]; !ok {
+		return errors.New("yaml: cannot unmarshal field name in NameStringValuePair required")
+	}
+	if _, ok := raw["value"]; !ok {
+		return errors.New("yaml: cannot unmarshal field value in NameStringValuePair required")
+	}
+	var name, value string
+	var ok bool
+	if name, ok = raw["name"].(string); !ok {
+		return errors.New("yaml: cannot unmarshal field name in NameStringValuePair must be string")
+	}
+	if value, ok = raw["value"].(string); !ok {
+		return errors.New("yaml: cannot unmarshal field value in NameStringValuePair must be string")
+	}
+	*j = NameStringValuePair{
+		Name:  name,
+		Value: &value,
+	}
+	return nil
+}
+
 // UnmarshalYAML implements yaml.Unmarshaler.
 func (j *LanguageSpecificInstrumentation) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	var raw map[string]interface{}