Skip to content

Commit fca5f92

Browse files
haywoodshhaywoodsh
committed
check nginxplus flag, add tests
Signed-off-by: Haywood Shannon <[email protected]>
1 parent 97bb428 commit fca5f92

File tree

4 files changed

+173
-93
lines changed

4 files changed

+173
-93
lines changed

‎internal/configs/configmaps.go‎

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has
208208

209209
if realIPHeader, exists := cfgm.Data["real-ip-header"]; exists {
210210
if hasTLSPassthrough {
211-
errorText := fmt.Sprintf("ConfigMap %s/%s: 'real-ip-header' is ignored because 'real_ip_header' is automatically set to 'proxy_protocol' when TLS passthrough is enabled, ignoring", cfgm.GetNamespace(), cfgm.GetName())
211+
errorText := fmt.Sprintf("ConfigMap %s/%s: 'real-ip-header' is ignored because 'real_ip_header' is automatically set to 'proxy_protocol' when TLS passthrough is expectedOpenTracingEnabled, ignoring", cfgm.GetNamespace(), cfgm.GetName())
212212
if realIPHeader == "proxy_protocol" {
213213
nl.Info(l, errorText)
214214
} else {
@@ -530,15 +530,15 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has
530530
}
531531
}
532532

533-
if openTracingTracer, exists := cfgm.Data["opentracing-tracer"]; exists {
533+
if openTracingTracer, exists := cfgm.Data["opentracing-expectedTracer"]; exists {
534534
cfgParams.MainOpenTracingTracer = openTracingTracer
535535
}
536536

537-
if openTracingTracerConfig, exists := cfgm.Data["opentracing-tracer-config"]; exists {
537+
if openTracingTracerConfig, exists := cfgm.Data["opentracing-expectedTracer-config"]; exists {
538538
cfgParams.MainOpenTracingTracerConfig = openTracingTracerConfig
539539
}
540540

541-
if cfgParams.MainOpenTracingTracer != "" || cfgParams.MainOpenTracingTracerConfig != "" {
541+
if cfgParams.MainOpenTracingTracer != "" && cfgParams.MainOpenTracingTracerConfig != "" {
542542
cfgParams.MainOpenTracingLoadModule = true
543543
}
544544

@@ -547,16 +547,19 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has
547547
nl.Error(l, err)
548548
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, err.Error())
549549
configOk = false
550+
} else if openTracing && nginxPlus {
551+
errorText := fmt.Sprintf("ConfigMap %s/%s key %s is not compatible with NGINX Plus", cfgm.Namespace, cfgm.Name, "opentracing")
552+
nl.Warn(l, errorText)
553+
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
554+
configOk = false
555+
clearOpenTracingParams(cfgParams)
550556
} else if !openTracing {
551-
cfgParams.MainOpenTracingEnabled = false
552-
cfgParams.MainOpenTracingLoadModule = false
553-
cfgParams.MainOpenTracingTracer = ""
554-
cfgParams.MainOpenTracingTracerConfig = ""
557+
clearOpenTracingParams(cfgParams)
555558
} else {
556559
if cfgParams.MainOpenTracingLoadModule {
557560
cfgParams.MainOpenTracingEnabled = openTracing
558561
} else {
559-
errorText := "ConfigMap key 'opentracing' requires both 'opentracing-tracer' and 'opentracing-tracer-config' keys configured, Opentracing will be disabled, ignoring"
562+
errorText := "ConfigMap key 'opentracing' requires both 'opentracing-expectedTracer' and 'opentracing-expectedTracer-config' keys configured, Opentracing will be disabled, ignoring"
560563
nl.Error(l, errorText)
561564
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
562565
configOk = false
@@ -674,6 +677,13 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has
674677
return cfgParams, configOk
675678
}
676679

680+
func clearOpenTracingParams(cfgParams *ConfigParams) {
681+
cfgParams.MainOpenTracingEnabled = false
682+
cfgParams.MainOpenTracingLoadModule = false
683+
cfgParams.MainOpenTracingTracer = ""
684+
cfgParams.MainOpenTracingTracerConfig = ""
685+
}
686+
677687
//nolint:gocyclo
678688
func parseConfigMapZoneSync(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *ConfigParams, eventLog record.EventRecorder, nginxPlus bool) (*ZoneSync, error) {
679689
if zoneSync, exists, err := GetMapKeyAsBool(cfgm.Data, "zone-sync", cfgm); exists {
@@ -694,7 +704,7 @@ func parseConfigMapZoneSync(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *Confi
694704

695705
if zoneSyncPort, exists, err := GetMapKeyAsInt(cfgm.Data, "zone-sync-port", cfgm); exists {
696706
if !cfgParams.ZoneSync.Enable {
697-
errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires 'zone-sync' to be enabled", cfgm.Namespace, cfgm.Name, "zone-sync-port")
707+
errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires 'zone-sync' to be expectedOpenTracingEnabled", cfgm.Namespace, cfgm.Name, "zone-sync-port")
698708
nl.Warn(l, errorText)
699709
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
700710
return nil, errors.New(errorText)
@@ -720,7 +730,7 @@ func parseConfigMapZoneSync(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *Confi
720730

721731
if zoneSyncResolverAddresses, exists := GetMapKeyAsStringSlice(cfgm.Data, "zone-sync-resolver-addresses", cfgm, ","); exists {
722732
if !cfgParams.ZoneSync.Enable {
723-
errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires 'zone-sync' to be enabled", cfgm.Namespace, cfgm.Name, "zone-sync-resolver-addresses")
733+
errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires 'zone-sync' to be expectedOpenTracingEnabled", cfgm.Namespace, cfgm.Name, "zone-sync-resolver-addresses")
724734
nl.Warn(l, errorText)
725735
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
726736
return nil, errors.New(errorText)
@@ -739,7 +749,7 @@ func parseConfigMapZoneSync(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *Confi
739749

740750
if zoneSyncResolverValid, exists := cfgm.Data["zone-sync-resolver-valid"]; exists {
741751
if !cfgParams.ZoneSync.Enable {
742-
errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires 'zone-sync' to be enabled", cfgm.Namespace, cfgm.Name, "zone-sync-resolver-valid")
752+
errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires 'zone-sync' to be expectedOpenTracingEnabled", cfgm.Namespace, cfgm.Name, "zone-sync-resolver-valid")
743753
nl.Warn(l, errorText)
744754
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
745755
return nil, errors.New(errorText)
@@ -763,7 +773,7 @@ func parseConfigMapZoneSync(l *slog.Logger, cfgm *v1.ConfigMap, cfgParams *Confi
763773
return nil, err
764774
}
765775
if !cfgParams.ZoneSync.Enable {
766-
errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires 'zone-sync' to be enabled", cfgm.Namespace, cfgm.Name, "zone-sync-resolver-ipv6")
776+
errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires 'zone-sync' to be expectedOpenTracingEnabled", cfgm.Namespace, cfgm.Name, "zone-sync-resolver-ipv6")
767777
nl.Warn(l, errorText)
768778
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
769779
return nil, errors.New(errorText)

‎internal/configs/configmaps_test.go‎

Lines changed: 150 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -134,12 +134,12 @@ func TestParseConfigMapWithTLSPassthroughProxyProtocol(t *testing.T) {
134134
{
135135
realIPheader: "proxy_protocol",
136136
want: "",
137-
msg: "valid realIPheader proxy_protocol, ignored when TLS Passthrough is enabled",
137+
msg: "valid realIPheader proxy_protocol, ignored when TLS Passthrough is expectedOpenTracingEnabled",
138138
},
139139
{
140140
realIPheader: "X-Forwarded-For",
141141
want: "",
142-
msg: "invalid realIPheader X-Forwarded-For, ignored when TLS Passthrough is enabled",
142+
msg: "invalid realIPheader X-Forwarded-For, ignored when TLS Passthrough is expectedOpenTracingEnabled",
143143
},
144144
{
145145
realIPheader: "",
@@ -1166,89 +1166,197 @@ func TestParseZoneSyncResolverIPV6MapResolverIPV6(t *testing.T) {
11661166
func makeEventLogger() record.EventRecorder {
11671167
return record.NewFakeRecorder(1024)
11681168
}
1169-
11701169
func TestOpenTracingConfiguration(t *testing.T) {
11711170
t.Parallel()
11721171
tests := []struct {
1173-
configMap *v1.ConfigMap
1174-
enabled bool
1175-
loadModule bool
1176-
tracer string
1177-
tracerConfig string
1178-
msg string
1172+
configMap *v1.ConfigMap
1173+
isPlus bool
1174+
expectedOpenTracingEnabled bool
1175+
expectedLoadModule bool
1176+
expectedTracer string
1177+
expectedTracerConfig string
1178+
expectedConfigOk bool
1179+
msg string
11791180
}{
11801181
{
11811182
configMap: &v1.ConfigMap{
11821183
Data: map[string]string{
1183-
"opentracing": "true",
1184-
"opentracing-tracer": "/usr/local/lib/libjaegertracing.so",
1185-
"opentracing-tracer-config": "/etc/nginx/opentracing.json",
1184+
"opentracing": "true",
1185+
"opentracing-expectedTracer": "/usr/local/lib/libjaegertracing.so",
1186+
"opentracing-expectedTracer-config": "/etc/nginx/opentracing.json",
1187+
},
1188+
},
1189+
isPlus: false,
1190+
expectedOpenTracingEnabled: true,
1191+
expectedLoadModule: true,
1192+
expectedTracer: "/usr/local/lib/libjaegertracing.so",
1193+
expectedTracerConfig: "/etc/nginx/opentracing.json",
1194+
expectedConfigOk: true,
1195+
msg: "oss: opentracing expectedOpenTracingEnabled (valid)",
1196+
},
1197+
{
1198+
configMap: &v1.ConfigMap{
1199+
Data: map[string]string{
1200+
"opentracing": "true",
1201+
"opentracing-expectedTracer": "/usr/local/lib/libjaegertracing.so",
1202+
},
1203+
},
1204+
isPlus: false,
1205+
expectedOpenTracingEnabled: false,
1206+
expectedLoadModule: false,
1207+
expectedTracer: "/usr/local/lib/libjaegertracing.so",
1208+
expectedTracerConfig: "",
1209+
expectedConfigOk: false,
1210+
msg: "oss: opentracing expectedOpenTracingEnabled, expectedTracer-config not set (invalid)",
1211+
},
1212+
{
1213+
configMap: &v1.ConfigMap{
1214+
Data: map[string]string{
1215+
"opentracing": "true",
1216+
},
1217+
},
1218+
isPlus: false,
1219+
expectedOpenTracingEnabled: false,
1220+
expectedLoadModule: false,
1221+
expectedTracer: "",
1222+
expectedTracerConfig: "",
1223+
expectedConfigOk: false,
1224+
msg: "oss: opentracing expectedOpenTracingEnabled, expectedTracer and expectedTracer-config not set(invalid)",
1225+
},
1226+
{
1227+
configMap: &v1.ConfigMap{
1228+
Data: map[string]string{
1229+
"opentracing": "false",
1230+
"opentracing-expectedTracer": "/usr/local/lib/libjaegertracing.so",
1231+
"opentracing-expectedTracer-config": "/etc/nginx/opentracing.json",
11861232
},
11871233
},
1188-
enabled: true,
1189-
loadModule: true,
1190-
tracer: "/usr/local/lib/libjaegertracing.so",
1191-
tracerConfig: "/etc/nginx/opentracing.json",
1192-
msg: "opentracing enabled",
1234+
isPlus: false,
1235+
expectedOpenTracingEnabled: false,
1236+
expectedLoadModule: false,
1237+
expectedTracer: "",
1238+
expectedTracerConfig: "",
1239+
expectedConfigOk: true,
1240+
msg: "oss: opentracing disabled, expectedTracer and expectedTracer-config (valid)",
11931241
},
11941242
{
11951243
configMap: &v1.ConfigMap{
11961244
Data: map[string]string{
1197-
"opentracing": "false",
1198-
"opentracing-tracer": "/usr/local/lib/libjaegertracing.so",
1199-
"opentracing-tracer-config": "/etc/nginx/opentracing.json",
1245+
"opentracing": "false",
12001246
},
12011247
},
1202-
enabled: false,
1203-
loadModule: false,
1204-
tracer: "",
1205-
tracerConfig: "",
1206-
msg: "opentracing disabled",
1248+
isPlus: false,
1249+
expectedOpenTracingEnabled: false,
1250+
expectedLoadModule: false,
1251+
expectedTracer: "",
1252+
expectedTracerConfig: "",
1253+
expectedConfigOk: true,
1254+
msg: "oss: opentracing disabled (valid)",
12071255
},
12081256
{
12091257
configMap: &v1.ConfigMap{
12101258
Data: map[string]string{
12111259
"opentracing": "false",
12121260
},
12131261
},
1214-
enabled: false,
1215-
loadModule: false,
1216-
tracer: "",
1217-
tracerConfig: "",
1218-
msg: "opentracing disabled",
1262+
isPlus: true,
1263+
expectedOpenTracingEnabled: false,
1264+
expectedLoadModule: false,
1265+
expectedTracer: "",
1266+
expectedTracerConfig: "",
1267+
expectedConfigOk: true,
1268+
msg: "plus: opentracing explicitly disabled (valid)",
1269+
},
1270+
{
1271+
configMap: &v1.ConfigMap{
1272+
Data: map[string]string{},
1273+
},
1274+
isPlus: true,
1275+
expectedOpenTracingEnabled: false,
1276+
expectedLoadModule: false,
1277+
expectedTracer: "",
1278+
expectedTracerConfig: "",
1279+
expectedConfigOk: true,
1280+
msg: "plus: no opentracing keys set (valid)",
1281+
},
1282+
{
1283+
configMap: &v1.ConfigMap{
1284+
Data: map[string]string{
1285+
"opentracing": "false",
1286+
"opentracing-expectedTracer": "/usr/local/lib/libjaegertracing.so",
1287+
"opentracing-expectedTracer-config": "/etc/nginx/opentracing.json",
1288+
},
1289+
},
1290+
isPlus: true,
1291+
expectedOpenTracingEnabled: false,
1292+
expectedLoadModule: false,
1293+
expectedTracer: "",
1294+
expectedTracerConfig: "",
1295+
expectedConfigOk: true,
1296+
msg: "plus: opentracing disabled, expectedTracer and expectedTracer-config set (valid)",
1297+
},
1298+
{
1299+
configMap: &v1.ConfigMap{
1300+
Data: map[string]string{
1301+
"opentracing": "true",
1302+
},
1303+
},
1304+
isPlus: true,
1305+
expectedOpenTracingEnabled: false,
1306+
expectedLoadModule: false,
1307+
expectedTracer: "",
1308+
expectedTracerConfig: "",
1309+
expectedConfigOk: false,
1310+
msg: "plus: opentracing expectedOpenTracingEnabled (invalid)",
1311+
},
1312+
{
1313+
configMap: &v1.ConfigMap{
1314+
Data: map[string]string{
1315+
"opentracing": "true",
1316+
"opentracing-expectedTracer": "/usr/local/lib/libjaegertracing.so",
1317+
"opentracing-expectedTracer-config": "/etc/nginx/opentracing.json",
1318+
},
1319+
},
1320+
isPlus: true,
1321+
expectedOpenTracingEnabled: false,
1322+
expectedLoadModule: false,
1323+
expectedTracer: "",
1324+
expectedTracerConfig: "",
1325+
expectedConfigOk: false,
1326+
msg: "plus: opentracing expectedOpenTracingEnabled, expectedTracer and expectedTracer-config set (invalid)",
12191327
},
12201328
}
1221-
nginxPlus := false
1329+
12221330
hasAppProtect := false
12231331
hasAppProtectDos := false
12241332
hasTLSPassthrough := false
12251333

12261334
for _, test := range tests {
12271335
t.Run(test.msg, func(t *testing.T) {
1228-
result, configOk := ParseConfigMap(context.Background(), test.configMap, nginxPlus,
1336+
result, configOk := ParseConfigMap(context.Background(), test.configMap, test.isPlus,
12291337
hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger())
12301338

1231-
if !configOk {
1232-
t.Errorf("Expected valid config, got invalid")
1339+
if configOk != test.expectedConfigOk {
1340+
t.Errorf("configOk: want %v, got %v", test.expectedConfigOk, configOk)
12331341
}
1234-
if result.MainOpenTracingEnabled != test.enabled {
1342+
if result.MainOpenTracingEnabled != test.expectedOpenTracingEnabled {
12351343
t.Errorf("MainOpenTracingEnabled: want %v, got %v",
1236-
test.enabled, result.MainOpenTracingEnabled)
1344+
test.expectedOpenTracingEnabled, result.MainOpenTracingEnabled)
12371345
}
12381346

1239-
if result.MainOpenTracingLoadModule != test.loadModule {
1347+
if result.MainOpenTracingLoadModule != test.expectedLoadModule {
12401348
t.Errorf("MainOpenTracingLoadModule: want %v, got %v",
1241-
test.loadModule, result.MainOpenTracingLoadModule)
1349+
test.expectedLoadModule, result.MainOpenTracingLoadModule)
12421350
}
12431351

1244-
if result.MainOpenTracingTracer != test.tracer {
1352+
if result.MainOpenTracingTracer != test.expectedTracer {
12451353
t.Errorf("MainOpenTracingTracer: want %q, got %q",
1246-
test.tracer, result.MainOpenTracingTracer)
1354+
test.expectedTracer, result.MainOpenTracingTracer)
12471355
}
12481356

1249-
if result.MainOpenTracingTracerConfig != test.tracerConfig {
1357+
if result.MainOpenTracingTracerConfig != test.expectedTracerConfig {
12501358
t.Errorf("MainOpenTracingTracerConfig: want %q, got %q",
1251-
test.tracerConfig, result.MainOpenTracingTracerConfig)
1359+
test.expectedTracerConfig, result.MainOpenTracingTracerConfig)
12521360
}
12531361
})
12541362
}

‎internal/nginx/version.go‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -99,12 +99,3 @@ func extractPlusVersionValues(input string) (int, int, error) {
9999

100100
return rValue, pValue, nil
101101
}
102-
103-
// IsOpenTracingSupported takes version and determines if the version
104-
// supports the Open Tracing lib.
105-
//
106-
// Support for OpenTracing is dropped in NGINX Plus R34.
107-
// Earlier NGINX Plus versions and NGINX OSS support OpenTracing.
108-
func IsOpenTracingSupported(version Version) bool {
109-
return !version.IsPlus
110-
}

0 commit comments

Comments
 (0)