Skip to content

Commit ab7fcf5

Browse files
haywoodshpdabelf5
haywoodsh
authored and
pdabelf5
committed
allow partially valid otel config, add validation and unit tests
Signed-off-by: Haywood Shannon <[email protected]> Signed-off-by: Haywood Shannon <[email protected]>
1 parent 89b1579 commit ab7fcf5

File tree

2 files changed

+116
-21
lines changed

2 files changed

+116
-21
lines changed

‎internal/configs/configmaps.go‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,16 @@ import (
99
"strings"
1010
"time"
1111

12+
"github.com/nginx/kubernetes-ingress/internal/validation"
13+
1214
"github.com/nginx/kubernetes-ingress/internal/configs/commonhelpers"
1315

1416
v1 "k8s.io/api/core/v1"
1517
"k8s.io/client-go/tools/record"
1618

1719
"github.com/nginx/kubernetes-ingress/internal/configs/version1"
1820
nl "github.com/nginx/kubernetes-ingress/internal/logger"
19-
"github.com/nginx/kubernetes-ingress/internal/validation"
21+
k8s_validation "k8s.io/apimachinery/pkg/util/validation"
2022
)
2123

2224
const (
@@ -747,6 +749,8 @@ func parseConfigMapZoneSync(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *Confi
747749

748750
//nolint:gocyclo
749751
func parseConfigMapOpenTelemetry(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *ConfigParams, eventLog record.EventRecorder) (*ConfigParams, error) {
752+
otelValid := true
753+
750754
if otelExporterEndpoint, exists := cfgm.Data["otel-exporter-endpoint"]; exists {
751755
otelExporterEndpoint = strings.TrimSpace(otelExporterEndpoint)
752756
if otelExporterEndpoint != "" {
@@ -757,7 +761,15 @@ func parseConfigMapOpenTelemetry(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *
757761
if otelExporterHeaderName, exists := cfgm.Data["otel-exporter-header-name"]; exists {
758762
otelExporterHeaderName = strings.TrimSpace(otelExporterHeaderName)
759763
if otelExporterHeaderName != "" {
760-
cfgParams.MainOtelExporterHeaderName = otelExporterHeaderName
764+
errorMessages := k8s_validation.IsHTTPHeaderName(otelExporterHeaderName)
765+
if len(errorMessages) > 0 {
766+
errorText := fmt.Sprintf("ConfigMap %s/%s: invalid value for 'otel-exporter-header-name': %q, %v", cfgm.GetNamespace(), cfgm.GetName(), otelExporterHeaderName, errorMessages)
767+
nl.Error(l, errorText)
768+
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
769+
otelValid = false
770+
} else {
771+
cfgParams.MainOtelExporterHeaderName = otelExporterHeaderName
772+
}
761773
}
762774
}
763775

@@ -775,8 +787,6 @@ func parseConfigMapOpenTelemetry(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *
775787
}
776788
}
777789

778-
otelValid := true
779-
780790
if otelTraceInHTTP, exists, err := GetMapKeyAsBool(cfgm.Data, "otel-trace-in-http", cfgm); exists {
781791
if err != nil {
782792
nl.Error(l, err)
@@ -788,6 +798,8 @@ func parseConfigMapOpenTelemetry(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *
788798

789799
if (cfgParams.MainOtelExporterHeaderName != "" && cfgParams.MainOtelExporterHeaderValue == "") ||
790800
(cfgParams.MainOtelExporterHeaderName == "" && cfgParams.MainOtelExporterHeaderValue != "") {
801+
cfgParams.MainOtelExporterHeaderName = ""
802+
cfgParams.MainOtelExporterHeaderValue = ""
791803
errorText := "Both 'otel-exporter-header-name' and 'otel-exporter-header-value' must be set or neither"
792804
nl.Error(l, errorText)
793805
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
@@ -808,6 +820,10 @@ func parseConfigMapOpenTelemetry(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *
808820
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
809821
otelValid = false
810822
cfgParams.MainOtelTraceInHTTP = false
823+
cfgParams.MainOtelExporterHeaderName = ""
824+
cfgParams.MainOtelExporterHeaderValue = ""
825+
cfgParams.MainOtelServiceName = ""
826+
cfgParams.MainOtelTraceInHTTP = false
811827
}
812828

813829
if !otelValid {

‎internal/configs/configmaps_test.go‎

Lines changed: 96 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1440,7 +1440,7 @@ func TestOpenTelemetryConfigurationSuccess(t *testing.T) {
14401440
hasAppProtect := false
14411441
hasAppProtectDos := false
14421442
hasTLSPassthrough := false
1443-
const expectedConfigOk = true
1443+
expectedConfigOk := true
14441444

14451445
for _, test := range tests {
14461446
t.Run(test.msg, func(t *testing.T) {
@@ -1474,19 +1474,44 @@ func TestOpenTelemetryConfigurationSuccess(t *testing.T) {
14741474
func TestOpenTelemetryConfigurationInvalid(t *testing.T) {
14751475
t.Parallel()
14761476
tests := []struct {
1477-
configMap *v1.ConfigMap
1478-
expectedLoadModule bool
1479-
msg string
1477+
configMap *v1.ConfigMap
1478+
expectedLoadModule bool
1479+
expectedExporterEndpoint string
1480+
expectedExporterHeaderName string
1481+
expectedExporterHeaderValue string
1482+
expectedServiceName string
1483+
expectedTraceInHTTP bool
1484+
msg string
14801485
}{
14811486
{
14821487
configMap: &v1.ConfigMap{
14831488
Data: map[string]string{
1484-
"otel-exporter-endpoint": " ",
1489+
"otel-exporter-endpoint": "",
14851490
"otel-service-name": "nginx-ingress-controller:nginx",
14861491
},
14871492
},
1488-
expectedLoadModule: false,
1489-
msg: "empty endpoint, service name set",
1493+
expectedLoadModule: false,
1494+
expectedExporterEndpoint: "",
1495+
expectedExporterHeaderName: "",
1496+
expectedExporterHeaderValue: "",
1497+
expectedServiceName: "",
1498+
expectedTraceInHTTP: false,
1499+
msg: "invalid, endpoint missing, service name set",
1500+
},
1501+
{
1502+
configMap: &v1.ConfigMap{
1503+
Data: map[string]string{
1504+
"otel-exporter-header-name": "X-Custom-Header",
1505+
"otel-exporter-header-value": "custom-value",
1506+
},
1507+
},
1508+
expectedLoadModule: false,
1509+
expectedExporterEndpoint: "",
1510+
expectedExporterHeaderName: "",
1511+
expectedExporterHeaderValue: "",
1512+
expectedServiceName: "",
1513+
expectedTraceInHTTP: false,
1514+
msg: "invalid, endpoint missing, header name and value set",
14901515
},
14911516
{
14921517
configMap: &v1.ConfigMap{
@@ -1496,8 +1521,13 @@ func TestOpenTelemetryConfigurationInvalid(t *testing.T) {
14961521
"otel-service-name": "nginx-ingress-controller:nginx",
14971522
},
14981523
},
1499-
expectedLoadModule: true,
1500-
msg: "endpoint set, header name only",
1524+
expectedLoadModule: true,
1525+
expectedExporterEndpoint: "https://otel-collector:4317",
1526+
expectedExporterHeaderName: "",
1527+
expectedExporterHeaderValue: "",
1528+
expectedServiceName: "nginx-ingress-controller:nginx",
1529+
expectedTraceInHTTP: false,
1530+
msg: "partially invalid, header value missing",
15011531
},
15021532
{
15031533
configMap: &v1.ConfigMap{
@@ -1507,18 +1537,30 @@ func TestOpenTelemetryConfigurationInvalid(t *testing.T) {
15071537
"otel-service-name": "nginx-ingress-controller:nginx",
15081538
},
15091539
},
1510-
expectedLoadModule: true,
1511-
msg: "endpoint set, header value only",
1540+
expectedLoadModule: true,
1541+
expectedExporterEndpoint: "https://otel-collector:4317",
1542+
expectedExporterHeaderName: "",
1543+
expectedExporterHeaderValue: "",
1544+
expectedServiceName: "nginx-ingress-controller:nginx",
1545+
expectedTraceInHTTP: false,
1546+
msg: "partially invalid, header name missing",
15121547
},
15131548
{
15141549
configMap: &v1.ConfigMap{
15151550
Data: map[string]string{
1516-
"otel-exporter-header-name": "X-Custom-Header",
1551+
"otel-exporter-endpoint": "https://otel-collector:4317",
1552+
"otel-exporter-header-name": "X-Custom-H$eader",
15171553
"otel-exporter-header-value": "custom-value",
1554+
"otel-service-name": "nginx-ingress-controller:nginx",
15181555
},
15191556
},
1520-
expectedLoadModule: false,
1521-
msg: "no endpoint, headers set",
1557+
expectedLoadModule: true,
1558+
expectedExporterEndpoint: "https://otel-collector:4317",
1559+
expectedExporterHeaderName: "",
1560+
expectedExporterHeaderValue: "",
1561+
expectedServiceName: "nginx-ingress-controller:nginx",
1562+
expectedTraceInHTTP: false,
1563+
msg: "partially invalid, header value invalid",
15221564
},
15231565
{
15241566
configMap: &v1.ConfigMap{
@@ -1528,12 +1570,34 @@ func TestOpenTelemetryConfigurationInvalid(t *testing.T) {
15281570
"otel-trace-in-http": "invalid",
15291571
},
15301572
},
1531-
expectedLoadModule: true,
1532-
msg: "endpoint set, invalid trace flag",
1573+
expectedLoadModule: true,
1574+
expectedExporterEndpoint: "https://otel-collector:4317",
1575+
expectedExporterHeaderName: "",
1576+
expectedExporterHeaderValue: "",
1577+
expectedServiceName: "nginx-ingress-controller:nginx",
1578+
expectedTraceInHTTP: false,
1579+
msg: "partially invalid, trace flag invalid",
1580+
},
1581+
{
1582+
configMap: &v1.ConfigMap{
1583+
Data: map[string]string{
1584+
"otel-exporter-endpoint": "https://otel-collector:4317",
1585+
"otel-exporter-header-value": "custom-value",
1586+
"otel-service-name": "nginx-ingress-controller:nginx",
1587+
"otel-trace-in-http": "true",
1588+
},
1589+
},
1590+
expectedLoadModule: true,
1591+
expectedExporterEndpoint: "https://otel-collector:4317",
1592+
expectedExporterHeaderName: "",
1593+
expectedExporterHeaderValue: "",
1594+
expectedServiceName: "nginx-ingress-controller:nginx",
1595+
expectedTraceInHTTP: true,
1596+
msg: "partially invalid, header name missing, trace in http set",
15331597
},
15341598
}
15351599

1536-
isPlus := true
1600+
isPlus := false
15371601
hasAppProtect := false
15381602
hasAppProtectDos := false
15391603
hasTLSPassthrough := false
@@ -1549,6 +1613,21 @@ func TestOpenTelemetryConfigurationInvalid(t *testing.T) {
15491613
if result.MainOtelLoadModule != test.expectedLoadModule {
15501614
t.Errorf("MainOtelLoadModule: want %v, got %v", test.expectedLoadModule, result.MainOtelLoadModule)
15511615
}
1616+
if result.MainOtelExporterEndpoint != test.expectedExporterEndpoint {
1617+
t.Errorf("MainOtelExporterEndpoint: want %q, got %q", test.expectedExporterEndpoint, result.MainOtelExporterEndpoint)
1618+
}
1619+
if result.MainOtelExporterHeaderName != test.expectedExporterHeaderName {
1620+
t.Errorf("MainOtelExporterHeaderName: want %q, got %q", test.expectedExporterHeaderName, result.MainOtelExporterHeaderName)
1621+
}
1622+
if result.MainOtelExporterHeaderValue != test.expectedExporterHeaderValue {
1623+
t.Errorf("MainOtelExporterHeaderValue: want %q, got %q", test.expectedExporterHeaderValue, result.MainOtelExporterHeaderValue)
1624+
}
1625+
if result.MainOtelServiceName != test.expectedServiceName {
1626+
t.Errorf("MainOtelServiceName: want %q, got %q", test.expectedServiceName, result.MainOtelServiceName)
1627+
}
1628+
if result.MainOtelTraceInHTTP != test.expectedTraceInHTTP {
1629+
t.Errorf("MainOtelTraceInHTTP: want %v, got %v", test.expectedTraceInHTTP, result.MainOtelTraceInHTTP)
1630+
}
15521631
})
15531632
}
15541633
}

0 commit comments

Comments
 (0)