From 2cdfec2736a8f49c3da390784991108bc2cb4939 Mon Sep 17 00:00:00 2001 From: Adrian Kostrubiak Date: Tue, 23 Nov 2021 13:55:43 -0500 Subject: [PATCH 1/6] Add liveness probe to created container if otelcol configuration supports a health_check. Fixes #571 Signed-off-by: Adrian Kostrubiak --- pkg/collector/adapters/config_to_probe.go | 171 ++++++++++++ .../adapters/config_to_probe_test.go | 243 ++++++++++++++++++ pkg/collector/container.go | 10 +- pkg/collector/container_test.go | 21 ++ 4 files changed, 444 insertions(+), 1 deletion(-) create mode 100644 pkg/collector/adapters/config_to_probe.go create mode 100644 pkg/collector/adapters/config_to_probe_test.go diff --git a/pkg/collector/adapters/config_to_probe.go b/pkg/collector/adapters/config_to_probe.go new file mode 100644 index 0000000000..9b1a60f829 --- /dev/null +++ b/pkg/collector/adapters/config_to_probe.go @@ -0,0 +1,171 @@ +package adapters + +import ( + "errors" + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "strings" +) + +var ( + // ErrNoService indicates that there is no service in the configuration. + ErrNoService = errors.New("no service available as part of the configuration") + // ErrNoExtensions indicates that there are no extensions in the configuration + ErrNoExtensions = errors.New("no extensions available as part of the configuration") + + // ErrServiceNotAMap indicates that the service property isn't a map of values. + ErrServiceNotAMap = errors.New("service property in the configuration doesn't contain valid services") + // ErrExtensionsNotAMap indicates that the extensions property isn't a map of values. + ErrExtensionsNotAMap = errors.New("extensions property in the configuration doesn't contain valid extensions") + + // ErrNoExtensionHealthCheck indicates no health_check extension was found for the + ErrNoExtensionHealthCheck = errors.New("extensions property in the configuration does not contain the expected health_check extension") + + // ErrNoServiceExtensions indicates the service configuration does not contain any extensions + ErrNoServiceExtensions = errors.New("service property in the configuration doesn't contain extensions") + + // ErrServiceExtensionsNotSlice indicates the service extensions property isn't a slice as expected + ErrServiceExtensionsNotSlice = errors.New("service extensions property in the configuration does not contain valid extensions") + + // ErrNoServiceExtensionHealthCheck indicates no health_check + ErrNoServiceExtensionHealthCheck = errors.New("no healthcheck extension available in service extension configuration") +) + +// ConfigToContainerProbe converts the incoming configuration object into a container probe or returns an error +func ConfigToContainerProbe(logger logr.Logger, config map[interface{}]interface{}) (*corev1.Probe, error) { + serviceProperty, ok := config["service"] + if !ok { + return nil, ErrNoService + } + service, ok := serviceProperty.(map[interface{}]interface{}) + if !ok { + return nil, ErrServiceNotAMap + } + + serviceExtensionsProperty, ok := service["extensions"] + if !ok { + return nil, ErrNoServiceExtensions + } + + healthcheckForProbe := "" + + serviceExtensions, ok := serviceExtensionsProperty.([]interface{}) + if !ok { + return nil, ErrServiceExtensionsNotSlice + } + // in the event of multiple health_check extensions defined, we arbitrarily take the first one found + for _, ext := range serviceExtensions { + parsedExt, ok := ext.(string) + if ok && strings.HasPrefix(parsedExt, "health_check") { + healthcheckForProbe = parsedExt + break + } + } + + if healthcheckForProbe == "" { + return nil, ErrNoServiceExtensionHealthCheck + } + + extensionsProperty, ok := config["extensions"] + if !ok { + return nil, ErrNoExtensions + } + extensions, ok := extensionsProperty.(map[interface{}]interface{}) + if !ok { + return nil, ErrExtensionsNotAMap + } + healthcheckExtension, ok := extensions[healthcheckForProbe] + if !ok { + return nil, ErrNoExtensionHealthCheck + } + + return createProbeFromExtension(healthcheckExtension) +} + +func createProbeFromExtension(extension interface{}) (*corev1.Probe, error) { + probeCfg := extractProbeConfigurationFromExtension(extension) + return &corev1.Probe{ + Handler: corev1.Handler{ + HTTPGet: &corev1.HTTPGetAction{ + Path: probeCfg.path, + Port: probeCfg.port, + Host: probeCfg.host, + }, + }, + }, nil +} + +type probeConfiguration struct { + path string + port intstr.IntOrString + host string +} + +const ( + defaultHealthCheckPath = "/" + defaultHealthCheckPort = 13133 + defaultHealthCheckHost = "0.0.0.0" +) + +func extractProbeConfigurationFromExtension(ext interface{}) probeConfiguration { + extensionCfg, ok := ext.(map[interface{}]interface{}) + if !ok { + return defaultProbeConfiguration() + } + endpoint := extractEndpointFromExtensionConfig(extensionCfg) + return probeConfiguration{ + path: extractPathFromExtensionConfig(extensionCfg), + port: endpoint.port, + host: endpoint.host, + } +} + +func defaultProbeConfiguration() probeConfiguration { + return probeConfiguration{ + path: defaultHealthCheckPath, + port: intstr.FromInt(defaultHealthCheckPort), + host: defaultHealthCheckHost, + } +} + +type healthCheckEndpoint struct { + port intstr.IntOrString + host string +} + +func defaultHealthCheckEndpoint() healthCheckEndpoint { + defaultProbe := defaultProbeConfiguration() + return healthCheckEndpoint{ + port: defaultProbe.port, + host: defaultProbe.host, + } +} + +func extractEndpointFromExtensionConfig(cfg map[interface{}]interface{}) healthCheckEndpoint { + endpoint, ok := cfg["endpoint"] + if !ok { + return defaultHealthCheckEndpoint() + } + parsedEndpoint, ok := endpoint.(string) + if !ok { + return defaultHealthCheckEndpoint() + } + endpointComponents := strings.Split(parsedEndpoint, ":") + if len(endpointComponents) != 2 { + return defaultHealthCheckEndpoint() + } + return healthCheckEndpoint{ + port: intstr.Parse(endpointComponents[1]), + host: endpointComponents[0], + } +} + +func extractPathFromExtensionConfig(cfg map[interface{}]interface{}) string { + if path, ok := cfg["path"]; ok { + if parsedPath, ok := path.(string); ok { + return parsedPath + } + } + return defaultHealthCheckPath +} diff --git a/pkg/collector/adapters/config_to_probe_test.go b/pkg/collector/adapters/config_to_probe_test.go new file mode 100644 index 0000000000..555d3d3259 --- /dev/null +++ b/pkg/collector/adapters/config_to_probe_test.go @@ -0,0 +1,243 @@ +package adapters_test + +import ( + "github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "testing" +) + +func TestSimpleCase(t *testing.T) { + configStr := `extensions: + health_check: +service: + extensions: [health_check]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + actualProbe, err := adapters.ConfigToContainerProbe(logger, config) + assert.NoError(t, err) + assert.Equal(t, "/", actualProbe.HTTPGet.Path) + assert.Equal(t, int32(13133), actualProbe.HTTPGet.Port.IntVal) + assert.Equal(t, "0.0.0.0", actualProbe.HTTPGet.Host) +} + +func TestShouldUseCustomEndpointAndPath(t *testing.T) { + configStr := `extensions: + health_check: + endpoint: localhost:1234 + path: /checkit +service: + extensions: [health_check]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + actualProbe, err := adapters.ConfigToContainerProbe(logger, config) + assert.NoError(t, err) + assert.Equal(t, "/checkit", actualProbe.HTTPGet.Path) + assert.Equal(t, int32(1234), actualProbe.HTTPGet.Port.IntVal) + assert.Equal(t, "localhost", actualProbe.HTTPGet.Host) +} + +func TestShouldUseCustomEndpointAndDefaultPath(t *testing.T) { + configStr := `extensions: + health_check: + endpoint: localhost:1234 +service: + extensions: [health_check]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + actualProbe, err := adapters.ConfigToContainerProbe(logger, config) + assert.NoError(t, err) + assert.Equal(t, "/", actualProbe.HTTPGet.Path) + assert.Equal(t, int32(1234), actualProbe.HTTPGet.Port.IntVal) + assert.Equal(t, "localhost", actualProbe.HTTPGet.Host) +} + +func TestShouldUseDefaultEndpointAndCustomPath(t *testing.T) { + configStr := `extensions: + health_check: + path: /checkit +service: + extensions: [health_check]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + actualProbe, err := adapters.ConfigToContainerProbe(logger, config) + assert.NoError(t, err) + assert.Equal(t, "/checkit", actualProbe.HTTPGet.Path) + assert.Equal(t, int32(13133), actualProbe.HTTPGet.Port.IntVal) + assert.Equal(t, "0.0.0.0", actualProbe.HTTPGet.Host) +} + +func TestShouldUseDefaultEndpointForUnexpectedEndpoint(t *testing.T) { + configStr := `extensions: + health_check: + endpoint: 0:0:0" +service: + extensions: [health_check]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + actualProbe, err := adapters.ConfigToContainerProbe(logger, config) + assert.NoError(t, err) + assert.Equal(t, "/", actualProbe.HTTPGet.Path) + assert.Equal(t, int32(13133), actualProbe.HTTPGet.Port.IntVal) + assert.Equal(t, "0.0.0.0", actualProbe.HTTPGet.Host) +} + +func TestShouldErrorIfNoService(t *testing.T) { + configStr := `extensions: + health_check:` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + _, err = adapters.ConfigToContainerProbe(logger, config) + assert.Equal(t, adapters.ErrNoService, err) +} + +func TestShouldErrorIfBadlyFormattedService(t *testing.T) { + configStr := `extensions: + health_check: +service: [hi]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + _, err = adapters.ConfigToContainerProbe(logger, config) + assert.Equal(t, adapters.ErrServiceNotAMap, err) +} + +func TestShouldErrorIfNoServiceExtensions(t *testing.T) { + configStr := `service: + pipelines: + traces: + receivers: [otlp]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + _, err = adapters.ConfigToContainerProbe(logger, config) + assert.Equal(t, adapters.ErrNoServiceExtensions, err) +} + +func TestShouldErrorIfBadlyFormattedServiceExtensions(t *testing.T) { + configStr := `service: + extensions: + this: should-not-be-a-map` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + _, err = adapters.ConfigToContainerProbe(logger, config) + assert.Equal(t, adapters.ErrServiceExtensionsNotSlice, err) +} + +func TestShouldErrorIfNoHealthCheckInServiceExtensions(t *testing.T) { + configStr := `service: + extensions: [pprof]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + _, err = adapters.ConfigToContainerProbe(logger, config) + assert.Equal(t, adapters.ErrNoServiceExtensionHealthCheck, err) +} + +func TestShouldErrorIfNoExtensions(t *testing.T) { + configStr := `service: + extensions: [health_check]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + _, err = adapters.ConfigToContainerProbe(logger, config) + assert.Equal(t, adapters.ErrNoExtensions, err) +} + +func TestShouldErrorIfBadlyFormattedExtensions(t *testing.T) { + configStr := `extensions: [hi] +service: + extensions: [health_check]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + _, err = adapters.ConfigToContainerProbe(logger, config) + assert.Equal(t, adapters.ErrExtensionsNotAMap, err) +} + +func TestShouldErrorIfNoHealthCheckExtension(t *testing.T) { + configStr := `extensions: + pprof: +service: + extensions: [health_check]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + _, err = adapters.ConfigToContainerProbe(logger, config) + assert.Equal(t, adapters.ErrNoExtensionHealthCheck, err) +} + +func TestShouldErrorIfNoHealthCheckExtension_mustMatchFirstHealthCheck(t *testing.T) { + configStr := `extensions: + health_check: +service: + extensions: [health_check/1, health_check]` + + // prepare + config, err := adapters.ConfigFromString(configStr) + require.NoError(t, err) + require.NotEmpty(t, config) + + // test + _, err = adapters.ConfigToContainerProbe(logger, config) + assert.Equal(t, adapters.ErrNoExtensionHealthCheck, err) +} diff --git a/pkg/collector/container.go b/pkg/collector/container.go index c97f8db540..9c89827464 100644 --- a/pkg/collector/container.go +++ b/pkg/collector/container.go @@ -16,8 +16,8 @@ package collector import ( "fmt" - "github.com/go-logr/logr" + "github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters" corev1 "k8s.io/api/core/v1" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" @@ -77,6 +77,13 @@ func Container(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelem }, }) + var livenessProbe *corev1.Probe + if config, err := adapters.ConfigFromString(otelcol.Spec.Config); err == nil { + if probe, err := adapters.ConfigToContainerProbe(logger, config); err == nil { + livenessProbe = probe + } + } + return corev1.Container{ Name: naming.Container(), Image: image, @@ -87,5 +94,6 @@ func Container(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelem EnvFrom: otelcol.Spec.EnvFrom, Resources: otelcol.Spec.Resources, SecurityContext: otelcol.Spec.SecurityContext, + LivenessProbe: livenessProbe, } } diff --git a/pkg/collector/container_test.go b/pkg/collector/container_test.go index 76dd1c8270..328f65de0c 100644 --- a/pkg/collector/container_test.go +++ b/pkg/collector/container_test.go @@ -273,3 +273,24 @@ func TestContainerEnvFrom(t *testing.T) { assert.Contains(t, c.EnvFrom, envFrom1) assert.Contains(t, c.EnvFrom, envFrom2) } + +func TestContainerProbe(t *testing.T) { + // prepare + otelcol := v1alpha1.OpenTelemetryCollector{ + Spec: v1alpha1.OpenTelemetryCollectorSpec{ + Config: `extensions: + health_check: +service: + extensions: [health_check]`, + }, + } + cfg := config.New() + + // test + c := Container(cfg, logger, otelcol) + + // verify + assert.Equal(t, "0.0.0.0", c.LivenessProbe.HTTPGet.Host) + assert.Equal(t, "/", c.LivenessProbe.HTTPGet.Path) + assert.Equal(t, int32(13133), c.LivenessProbe.HTTPGet.Port.IntVal) +} From 3333fd25900b7ff36054c77eee64a0862909fd87 Mon Sep 17 00:00:00 2001 From: Adrian Kostrubiak Date: Wed, 24 Nov 2021 22:51:13 -0500 Subject: [PATCH 2/6] PR feedback. adjust logic for generating probe; fix probe host to be empty; refine how we choose a healthcheck extension; rework tests to be table drive, flesh out a few more cases; --- pkg/collector/adapters/config_to_probe.go | 98 ++--- .../adapters/config_to_probe_test.go | 343 +++++++----------- pkg/collector/container.go | 2 +- pkg/collector/container_test.go | 2 +- 4 files changed, 169 insertions(+), 276 deletions(-) diff --git a/pkg/collector/adapters/config_to_probe.go b/pkg/collector/adapters/config_to_probe.go index 9b1a60f829..0575a5d358 100644 --- a/pkg/collector/adapters/config_to_probe.go +++ b/pkg/collector/adapters/config_to_probe.go @@ -2,38 +2,38 @@ package adapters import ( "errors" - "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/intstr" "strings" ) var ( - // ErrNoService indicates that there is no service in the configuration. - ErrNoService = errors.New("no service available as part of the configuration") - // ErrNoExtensions indicates that there are no extensions in the configuration + ErrNoService = errors.New("no service available as part of the configuration") ErrNoExtensions = errors.New("no extensions available as part of the configuration") - // ErrServiceNotAMap indicates that the service property isn't a map of values. - ErrServiceNotAMap = errors.New("service property in the configuration doesn't contain valid services") - // ErrExtensionsNotAMap indicates that the extensions property isn't a map of values. + ErrServiceNotAMap = errors.New("service property in the configuration doesn't contain valid services") ErrExtensionsNotAMap = errors.New("extensions property in the configuration doesn't contain valid extensions") - // ErrNoExtensionHealthCheck indicates no health_check extension was found for the ErrNoExtensionHealthCheck = errors.New("extensions property in the configuration does not contain the expected health_check extension") - // ErrNoServiceExtensions indicates the service configuration does not contain any extensions ErrNoServiceExtensions = errors.New("service property in the configuration doesn't contain extensions") - // ErrServiceExtensionsNotSlice indicates the service extensions property isn't a slice as expected - ErrServiceExtensionsNotSlice = errors.New("service extensions property in the configuration does not contain valid extensions") - - // ErrNoServiceExtensionHealthCheck indicates no health_check + ErrServiceExtensionsNotSlice = errors.New("service extensions property in the configuration does not contain valid extensions") ErrNoServiceExtensionHealthCheck = errors.New("no healthcheck extension available in service extension configuration") ) +type probeConfiguration struct { + path string + port intstr.IntOrString +} + +const ( + defaultHealthCheckPath = "/" + defaultHealthCheckPort = 13133 +) + // ConfigToContainerProbe converts the incoming configuration object into a container probe or returns an error -func ConfigToContainerProbe(logger logr.Logger, config map[interface{}]interface{}) (*corev1.Probe, error) { +func ConfigToContainerProbe(config map[interface{}]interface{}) (*corev1.Probe, error) { serviceProperty, ok := config["service"] if !ok { return nil, ErrNoService @@ -48,22 +48,19 @@ func ConfigToContainerProbe(logger logr.Logger, config map[interface{}]interface return nil, ErrNoServiceExtensions } - healthcheckForProbe := "" - serviceExtensions, ok := serviceExtensionsProperty.([]interface{}) if !ok { return nil, ErrServiceExtensionsNotSlice } - // in the event of multiple health_check extensions defined, we arbitrarily take the first one found + healthCheckServiceExtensions := make([]string, 0) for _, ext := range serviceExtensions { parsedExt, ok := ext.(string) if ok && strings.HasPrefix(parsedExt, "health_check") { - healthcheckForProbe = parsedExt - break + healthCheckServiceExtensions = append(healthCheckServiceExtensions, parsedExt) } } - if healthcheckForProbe == "" { + if len(healthCheckServiceExtensions) == 0 { return nil, ErrNoServiceExtensionHealthCheck } @@ -75,12 +72,15 @@ func ConfigToContainerProbe(logger logr.Logger, config map[interface{}]interface if !ok { return nil, ErrExtensionsNotAMap } - healthcheckExtension, ok := extensions[healthcheckForProbe] - if !ok { - return nil, ErrNoExtensionHealthCheck + // in the event of multiple health_check service extensions defined, we arbitrarily take the first one found + for _, healthCheckForProbe := range healthCheckServiceExtensions { + healthCheckExtension, ok := extensions[healthCheckForProbe] + if ok { + return createProbeFromExtension(healthCheckExtension) + } } - return createProbeFromExtension(healthcheckExtension) + return nil, ErrNoExtensionHealthCheck } func createProbeFromExtension(extension interface{}) (*corev1.Probe, error) { @@ -90,34 +90,19 @@ func createProbeFromExtension(extension interface{}) (*corev1.Probe, error) { HTTPGet: &corev1.HTTPGetAction{ Path: probeCfg.path, Port: probeCfg.port, - Host: probeCfg.host, }, }, }, nil } -type probeConfiguration struct { - path string - port intstr.IntOrString - host string -} - -const ( - defaultHealthCheckPath = "/" - defaultHealthCheckPort = 13133 - defaultHealthCheckHost = "0.0.0.0" -) - func extractProbeConfigurationFromExtension(ext interface{}) probeConfiguration { extensionCfg, ok := ext.(map[interface{}]interface{}) if !ok { return defaultProbeConfiguration() } - endpoint := extractEndpointFromExtensionConfig(extensionCfg) return probeConfiguration{ path: extractPathFromExtensionConfig(extensionCfg), - port: endpoint.port, - host: endpoint.host, + port: extractPortFromExtensionConfig(extensionCfg), } } @@ -125,24 +110,19 @@ func defaultProbeConfiguration() probeConfiguration { return probeConfiguration{ path: defaultHealthCheckPath, port: intstr.FromInt(defaultHealthCheckPort), - host: defaultHealthCheckHost, } } -type healthCheckEndpoint struct { - port intstr.IntOrString - host string -} - -func defaultHealthCheckEndpoint() healthCheckEndpoint { - defaultProbe := defaultProbeConfiguration() - return healthCheckEndpoint{ - port: defaultProbe.port, - host: defaultProbe.host, +func extractPathFromExtensionConfig(cfg map[interface{}]interface{}) string { + if path, ok := cfg["path"]; ok { + if parsedPath, ok := path.(string); ok { + return parsedPath + } } + return defaultHealthCheckPath } -func extractEndpointFromExtensionConfig(cfg map[interface{}]interface{}) healthCheckEndpoint { +func extractPortFromExtensionConfig(cfg map[interface{}]interface{}) intstr.IntOrString { endpoint, ok := cfg["endpoint"] if !ok { return defaultHealthCheckEndpoint() @@ -155,17 +135,9 @@ func extractEndpointFromExtensionConfig(cfg map[interface{}]interface{}) healthC if len(endpointComponents) != 2 { return defaultHealthCheckEndpoint() } - return healthCheckEndpoint{ - port: intstr.Parse(endpointComponents[1]), - host: endpointComponents[0], - } + return intstr.Parse(endpointComponents[1]) } -func extractPathFromExtensionConfig(cfg map[interface{}]interface{}) string { - if path, ok := cfg["path"]; ok { - if parsedPath, ok := path.(string); ok { - return parsedPath - } - } - return defaultHealthCheckPath +func defaultHealthCheckEndpoint() intstr.IntOrString { + return intstr.FromInt(defaultHealthCheckPort) } diff --git a/pkg/collector/adapters/config_to_probe_test.go b/pkg/collector/adapters/config_to_probe_test.go index 555d3d3259..89735381e8 100644 --- a/pkg/collector/adapters/config_to_probe_test.go +++ b/pkg/collector/adapters/config_to_probe_test.go @@ -7,237 +7,158 @@ import ( "testing" ) -func TestSimpleCase(t *testing.T) { - configStr := `extensions: +func TestConfigToProbeShouldCreateProbeFor(t *testing.T) { + tests := []struct { + desc string + config string + expectedPort int32 + expectedPath string + }{ + { + desc: "SimpleHappyPath", + expectedPort: int32(13133), + expectedPath: "/", + config: `extensions: health_check: service: - extensions: [health_check]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - actualProbe, err := adapters.ConfigToContainerProbe(logger, config) - assert.NoError(t, err) - assert.Equal(t, "/", actualProbe.HTTPGet.Path) - assert.Equal(t, int32(13133), actualProbe.HTTPGet.Port.IntVal) - assert.Equal(t, "0.0.0.0", actualProbe.HTTPGet.Host) -} - -func TestShouldUseCustomEndpointAndPath(t *testing.T) { - configStr := `extensions: + extensions: [health_check]`, + }, { + desc: "CustomEndpointAndPath", + expectedPort: int32(1234), + expectedPath: "/checkit", + config: `extensions: health_check: endpoint: localhost:1234 path: /checkit service: - extensions: [health_check]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - actualProbe, err := adapters.ConfigToContainerProbe(logger, config) - assert.NoError(t, err) - assert.Equal(t, "/checkit", actualProbe.HTTPGet.Path) - assert.Equal(t, int32(1234), actualProbe.HTTPGet.Port.IntVal) - assert.Equal(t, "localhost", actualProbe.HTTPGet.Host) -} - -func TestShouldUseCustomEndpointAndDefaultPath(t *testing.T) { - configStr := `extensions: + extensions: [health_check]`, + }, { + desc: "CustomEndpointAndDefaultPath", + expectedPort: int32(1234), + expectedPath: "/", + config: `extensions: health_check: endpoint: localhost:1234 service: - extensions: [health_check]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - actualProbe, err := adapters.ConfigToContainerProbe(logger, config) - assert.NoError(t, err) - assert.Equal(t, "/", actualProbe.HTTPGet.Path) - assert.Equal(t, int32(1234), actualProbe.HTTPGet.Port.IntVal) - assert.Equal(t, "localhost", actualProbe.HTTPGet.Host) -} - -func TestShouldUseDefaultEndpointAndCustomPath(t *testing.T) { - configStr := `extensions: + extensions: [health_check]`, + }, { + desc: "DefaultEndpointAndCustomPath", + expectedPort: int32(13133), + expectedPath: "/checkit", + config: `extensions: health_check: path: /checkit service: - extensions: [health_check]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - actualProbe, err := adapters.ConfigToContainerProbe(logger, config) - assert.NoError(t, err) - assert.Equal(t, "/checkit", actualProbe.HTTPGet.Path) - assert.Equal(t, int32(13133), actualProbe.HTTPGet.Port.IntVal) - assert.Equal(t, "0.0.0.0", actualProbe.HTTPGet.Host) -} - -func TestShouldUseDefaultEndpointForUnexpectedEndpoint(t *testing.T) { - configStr := `extensions: + extensions: [health_check]`, + }, { + desc: "DefaultEndpointForUnexpectedEndpoint", + expectedPort: int32(13133), + expectedPath: "/", + config: `extensions: health_check: endpoint: 0:0:0" service: - extensions: [health_check]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - actualProbe, err := adapters.ConfigToContainerProbe(logger, config) - assert.NoError(t, err) - assert.Equal(t, "/", actualProbe.HTTPGet.Path) - assert.Equal(t, int32(13133), actualProbe.HTTPGet.Port.IntVal) - assert.Equal(t, "0.0.0.0", actualProbe.HTTPGet.Host) -} - -func TestShouldErrorIfNoService(t *testing.T) { - configStr := `extensions: - health_check:` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - _, err = adapters.ConfigToContainerProbe(logger, config) - assert.Equal(t, adapters.ErrNoService, err) -} - -func TestShouldErrorIfBadlyFormattedService(t *testing.T) { - configStr := `extensions: + extensions: [health_check]`, + }, { + desc: "DefaultEndpointForUnparseablendpoint", + expectedPort: int32(13133), + expectedPath: "/", + config: `extensions: health_check: -service: [hi]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - _, err = adapters.ConfigToContainerProbe(logger, config) - assert.Equal(t, adapters.ErrServiceNotAMap, err) -} - -func TestShouldErrorIfNoServiceExtensions(t *testing.T) { - configStr := `service: - pipelines: - traces: - receivers: [otlp]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - _, err = adapters.ConfigToContainerProbe(logger, config) - assert.Equal(t, adapters.ErrNoServiceExtensions, err) -} - -func TestShouldErrorIfBadlyFormattedServiceExtensions(t *testing.T) { - configStr := `service: - extensions: - this: should-not-be-a-map` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - _, err = adapters.ConfigToContainerProbe(logger, config) - assert.Equal(t, adapters.ErrServiceExtensionsNotSlice, err) -} - -func TestShouldErrorIfNoHealthCheckInServiceExtensions(t *testing.T) { - configStr := `service: - extensions: [pprof]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - _, err = adapters.ConfigToContainerProbe(logger, config) - assert.Equal(t, adapters.ErrNoServiceExtensionHealthCheck, err) -} - -func TestShouldErrorIfNoExtensions(t *testing.T) { - configStr := `service: - extensions: [health_check]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - _, err = adapters.ConfigToContainerProbe(logger, config) - assert.Equal(t, adapters.ErrNoExtensions, err) -} - -func TestShouldErrorIfBadlyFormattedExtensions(t *testing.T) { - configStr := `extensions: [hi] + endpoint: + this: should-not-be-a-map" service: - extensions: [health_check]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - _, err = adapters.ConfigToContainerProbe(logger, config) - assert.Equal(t, adapters.ErrExtensionsNotAMap, err) + extensions: [health_check]`, + }, { + desc: "WillUseSecondServiceExtension", + config: `extensions: + health_check: +service: + extensions: [health_check/1, health_check]`, + expectedPort: int32(13133), + expectedPath: "/", + }, + } + + for _, test := range tests { + // prepare + config, err := adapters.ConfigFromString(test.config) + require.NoError(t, err, test.desc) + require.NotEmpty(t, config, test.desc) + + // test + actualProbe, err := adapters.ConfigToContainerProbe(config) + assert.NoError(t, err) + assert.Equal(t, test.expectedPath, actualProbe.HTTPGet.Path, test.desc) + assert.Equal(t, test.expectedPort, actualProbe.HTTPGet.Port.IntVal, test.desc) + assert.Equal(t, "", actualProbe.HTTPGet.Host, test.desc) + } } -func TestShouldErrorIfNoHealthCheckExtension(t *testing.T) { - configStr := `extensions: +func TestConfigToProbeShouldErrorIf(t *testing.T) { + tests := []struct { + desc string + config string + expectedErr error + }{ + { + desc: "NoHealthCheckExtension", + config: `extensions: pprof: service: - extensions: [health_check]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - _, err = adapters.ConfigToContainerProbe(logger, config) - assert.Equal(t, adapters.ErrNoExtensionHealthCheck, err) -} - -func TestShouldErrorIfNoHealthCheckExtension_mustMatchFirstHealthCheck(t *testing.T) { - configStr := `extensions: - health_check: + extensions: [health_check]`, + expectedErr: adapters.ErrNoExtensionHealthCheck, + }, { + desc: "BadlyFormattedExtensions", + config: `extensions: [hi] service: - extensions: [health_check/1, health_check]` - - // prepare - config, err := adapters.ConfigFromString(configStr) - require.NoError(t, err) - require.NotEmpty(t, config) - - // test - _, err = adapters.ConfigToContainerProbe(logger, config) - assert.Equal(t, adapters.ErrNoExtensionHealthCheck, err) + extensions: [health_check]`, + expectedErr: adapters.ErrExtensionsNotAMap, + }, { + desc: "NoExtensions", + config: `service: + extensions: [health_check]`, + expectedErr: adapters.ErrNoExtensions, + }, { + desc: "NoHealthCheckInServiceExtensions", + config: `service: + extensions: [pprof]`, + expectedErr: adapters.ErrNoServiceExtensionHealthCheck, + }, { + desc: "BadlyFormattedServiceExtensions", + config: `service: + extensions: + this: should-not-be-a-map`, + expectedErr: adapters.ErrServiceExtensionsNotSlice, + }, { + desc: "NoServiceExtensions", + config: `service: + pipelines: + traces: + receivers: [otlp]`, + expectedErr: adapters.ErrNoServiceExtensions, + }, { + desc: "BadlyFormattedService", + config: `extensions: + health_check: +service: [hi]`, + expectedErr: adapters.ErrServiceNotAMap, + }, { + desc: "NoService", + config: `extensions: + health_check:`, + expectedErr: adapters.ErrNoService, + }, + } + + for _, test := range tests { + // prepare + config, err := adapters.ConfigFromString(test.config) + require.NoError(t, err, test.desc) + require.NotEmpty(t, config, test.desc) + + // test + _, err = adapters.ConfigToContainerProbe(config) + assert.Equal(t, test.expectedErr, err, test.desc) + } } diff --git a/pkg/collector/container.go b/pkg/collector/container.go index 9c89827464..5eedc7c7bd 100644 --- a/pkg/collector/container.go +++ b/pkg/collector/container.go @@ -79,7 +79,7 @@ func Container(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelem var livenessProbe *corev1.Probe if config, err := adapters.ConfigFromString(otelcol.Spec.Config); err == nil { - if probe, err := adapters.ConfigToContainerProbe(logger, config); err == nil { + if probe, err := adapters.ConfigToContainerProbe(config); err == nil { livenessProbe = probe } } diff --git a/pkg/collector/container_test.go b/pkg/collector/container_test.go index 328f65de0c..60820c9c2b 100644 --- a/pkg/collector/container_test.go +++ b/pkg/collector/container_test.go @@ -290,7 +290,7 @@ service: c := Container(cfg, logger, otelcol) // verify - assert.Equal(t, "0.0.0.0", c.LivenessProbe.HTTPGet.Host) assert.Equal(t, "/", c.LivenessProbe.HTTPGet.Path) assert.Equal(t, int32(13133), c.LivenessProbe.HTTPGet.Port.IntVal) + assert.Equal(t, "", c.LivenessProbe.HTTPGet.Host) } From 54db59024ce1bcb26644cb3d561c1c370c5c8d5d Mon Sep 17 00:00:00 2001 From: Adrian Kostrubiak Date: Wed, 24 Nov 2021 22:55:08 -0500 Subject: [PATCH 3/6] pr feedback, additional test case --- pkg/collector/adapters/config_to_probe_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/collector/adapters/config_to_probe_test.go b/pkg/collector/adapters/config_to_probe_test.go index 89735381e8..31d0802b2a 100644 --- a/pkg/collector/adapters/config_to_probe_test.go +++ b/pkg/collector/adapters/config_to_probe_test.go @@ -39,6 +39,15 @@ service: config: `extensions: health_check: endpoint: localhost:1234 +service: + extensions: [health_check]`, + }, { + desc: "CustomEndpointWithJustPortAndDefaultPath", + expectedPort: int32(1234), + expectedPath: "/", + config: `extensions: + health_check: + endpoint: :1234 service: extensions: [health_check]`, }, { From 49a12c1c5158dccc04bbe644e5411579ac816d8e Mon Sep 17 00:00:00 2001 From: Adrian Kostrubiak Date: Thu, 25 Nov 2021 08:03:40 -0500 Subject: [PATCH 4/6] pr feedback - don't export errors --- pkg/collector/adapters/config_to_probe.go | 32 +++++++++---------- .../adapters/config_to_probe_test.go | 27 ++++++++-------- 2 files changed, 29 insertions(+), 30 deletions(-) diff --git a/pkg/collector/adapters/config_to_probe.go b/pkg/collector/adapters/config_to_probe.go index 0575a5d358..857527a71f 100644 --- a/pkg/collector/adapters/config_to_probe.go +++ b/pkg/collector/adapters/config_to_probe.go @@ -8,18 +8,18 @@ import ( ) var ( - ErrNoService = errors.New("no service available as part of the configuration") - ErrNoExtensions = errors.New("no extensions available as part of the configuration") + errNoService = errors.New("no service available as part of the configuration") + errNoExtensions = errors.New("no extensions available as part of the configuration") - ErrServiceNotAMap = errors.New("service property in the configuration doesn't contain valid services") - ErrExtensionsNotAMap = errors.New("extensions property in the configuration doesn't contain valid extensions") + errServiceNotAMap = errors.New("service property in the configuration doesn't contain valid services") + errExtensionsNotAMap = errors.New("extensions property in the configuration doesn't contain valid extensions") - ErrNoExtensionHealthCheck = errors.New("extensions property in the configuration does not contain the expected health_check extension") + errNoExtensionHealthCheck = errors.New("extensions property in the configuration does not contain the expected health_check extension") - ErrNoServiceExtensions = errors.New("service property in the configuration doesn't contain extensions") + errNoServiceExtensions = errors.New("service property in the configuration doesn't contain extensions") - ErrServiceExtensionsNotSlice = errors.New("service extensions property in the configuration does not contain valid extensions") - ErrNoServiceExtensionHealthCheck = errors.New("no healthcheck extension available in service extension configuration") + errServiceExtensionsNotSlice = errors.New("service extensions property in the configuration does not contain valid extensions") + errNoServiceExtensionHealthCheck = errors.New("no healthcheck extension available in service extension configuration") ) type probeConfiguration struct { @@ -36,21 +36,21 @@ const ( func ConfigToContainerProbe(config map[interface{}]interface{}) (*corev1.Probe, error) { serviceProperty, ok := config["service"] if !ok { - return nil, ErrNoService + return nil, errNoService } service, ok := serviceProperty.(map[interface{}]interface{}) if !ok { - return nil, ErrServiceNotAMap + return nil, errServiceNotAMap } serviceExtensionsProperty, ok := service["extensions"] if !ok { - return nil, ErrNoServiceExtensions + return nil, errNoServiceExtensions } serviceExtensions, ok := serviceExtensionsProperty.([]interface{}) if !ok { - return nil, ErrServiceExtensionsNotSlice + return nil, errServiceExtensionsNotSlice } healthCheckServiceExtensions := make([]string, 0) for _, ext := range serviceExtensions { @@ -61,16 +61,16 @@ func ConfigToContainerProbe(config map[interface{}]interface{}) (*corev1.Probe, } if len(healthCheckServiceExtensions) == 0 { - return nil, ErrNoServiceExtensionHealthCheck + return nil, errNoServiceExtensionHealthCheck } extensionsProperty, ok := config["extensions"] if !ok { - return nil, ErrNoExtensions + return nil, errNoExtensions } extensions, ok := extensionsProperty.(map[interface{}]interface{}) if !ok { - return nil, ErrExtensionsNotAMap + return nil, errExtensionsNotAMap } // in the event of multiple health_check service extensions defined, we arbitrarily take the first one found for _, healthCheckForProbe := range healthCheckServiceExtensions { @@ -80,7 +80,7 @@ func ConfigToContainerProbe(config map[interface{}]interface{}) (*corev1.Probe, } } - return nil, ErrNoExtensionHealthCheck + return nil, errNoExtensionHealthCheck } func createProbeFromExtension(extension interface{}) (*corev1.Probe, error) { diff --git a/pkg/collector/adapters/config_to_probe_test.go b/pkg/collector/adapters/config_to_probe_test.go index 31d0802b2a..376443c6c8 100644 --- a/pkg/collector/adapters/config_to_probe_test.go +++ b/pkg/collector/adapters/config_to_probe_test.go @@ -1,7 +1,6 @@ -package adapters_test +package adapters import ( - "github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "testing" @@ -91,12 +90,12 @@ service: for _, test := range tests { // prepare - config, err := adapters.ConfigFromString(test.config) + config, err := ConfigFromString(test.config) require.NoError(t, err, test.desc) require.NotEmpty(t, config, test.desc) // test - actualProbe, err := adapters.ConfigToContainerProbe(config) + actualProbe, err := ConfigToContainerProbe(config) assert.NoError(t, err) assert.Equal(t, test.expectedPath, actualProbe.HTTPGet.Path, test.desc) assert.Equal(t, test.expectedPort, actualProbe.HTTPGet.Port.IntVal, test.desc) @@ -116,58 +115,58 @@ func TestConfigToProbeShouldErrorIf(t *testing.T) { pprof: service: extensions: [health_check]`, - expectedErr: adapters.ErrNoExtensionHealthCheck, + expectedErr: errNoExtensionHealthCheck, }, { desc: "BadlyFormattedExtensions", config: `extensions: [hi] service: extensions: [health_check]`, - expectedErr: adapters.ErrExtensionsNotAMap, + expectedErr: errExtensionsNotAMap, }, { desc: "NoExtensions", config: `service: extensions: [health_check]`, - expectedErr: adapters.ErrNoExtensions, + expectedErr: errNoExtensions, }, { desc: "NoHealthCheckInServiceExtensions", config: `service: extensions: [pprof]`, - expectedErr: adapters.ErrNoServiceExtensionHealthCheck, + expectedErr: errNoServiceExtensionHealthCheck, }, { desc: "BadlyFormattedServiceExtensions", config: `service: extensions: this: should-not-be-a-map`, - expectedErr: adapters.ErrServiceExtensionsNotSlice, + expectedErr: errServiceExtensionsNotSlice, }, { desc: "NoServiceExtensions", config: `service: pipelines: traces: receivers: [otlp]`, - expectedErr: adapters.ErrNoServiceExtensions, + expectedErr: errNoServiceExtensions, }, { desc: "BadlyFormattedService", config: `extensions: health_check: service: [hi]`, - expectedErr: adapters.ErrServiceNotAMap, + expectedErr: errServiceNotAMap, }, { desc: "NoService", config: `extensions: health_check:`, - expectedErr: adapters.ErrNoService, + expectedErr: errNoService, }, } for _, test := range tests { // prepare - config, err := adapters.ConfigFromString(test.config) + config, err := ConfigFromString(test.config) require.NoError(t, err, test.desc) require.NotEmpty(t, config, test.desc) // test - _, err = adapters.ConfigToContainerProbe(config) + _, err = ConfigToContainerProbe(config) assert.Equal(t, test.expectedErr, err, test.desc) } } From 36e9369907bce6db84efccbf690c589b3f5cb5ec Mon Sep 17 00:00:00 2001 From: Adrian Kostrubiak Date: Thu, 25 Nov 2021 11:33:44 -0500 Subject: [PATCH 5/6] fix golangci-lint issues --- pkg/collector/adapters/config_to_probe.go | 19 +++++++++++++++++-- .../adapters/config_to_probe_test.go | 17 ++++++++++++++++- pkg/collector/container.go | 4 +++- 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/pkg/collector/adapters/config_to_probe.go b/pkg/collector/adapters/config_to_probe.go index 857527a71f..b01be9712a 100644 --- a/pkg/collector/adapters/config_to_probe.go +++ b/pkg/collector/adapters/config_to_probe.go @@ -1,10 +1,25 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package adapters import ( "errors" + "strings" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/intstr" - "strings" ) var ( @@ -32,7 +47,7 @@ const ( defaultHealthCheckPort = 13133 ) -// ConfigToContainerProbe converts the incoming configuration object into a container probe or returns an error +// ConfigToContainerProbe converts the incoming configuration object into a container probe or returns an error. func ConfigToContainerProbe(config map[interface{}]interface{}) (*corev1.Probe, error) { serviceProperty, ok := config["service"] if !ok { diff --git a/pkg/collector/adapters/config_to_probe_test.go b/pkg/collector/adapters/config_to_probe_test.go index 376443c6c8..65a6a609b3 100644 --- a/pkg/collector/adapters/config_to_probe_test.go +++ b/pkg/collector/adapters/config_to_probe_test.go @@ -1,9 +1,24 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package adapters import ( + "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "testing" ) func TestConfigToProbeShouldCreateProbeFor(t *testing.T) { diff --git a/pkg/collector/container.go b/pkg/collector/container.go index 5eedc7c7bd..21f9aec84d 100644 --- a/pkg/collector/container.go +++ b/pkg/collector/container.go @@ -16,10 +16,12 @@ package collector import ( "fmt" + "github.com/go-logr/logr" - "github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters" corev1 "k8s.io/api/core/v1" + "github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters" + "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/pkg/naming" From db11311de1390ef41f9c096b039c9875bdea9d3b Mon Sep 17 00:00:00 2001 From: Adrian Kostrubiak Date: Thu, 25 Nov 2021 15:16:35 -0500 Subject: [PATCH 6/6] fix golangci-lint/import order issues --- pkg/collector/container.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/collector/container.go b/pkg/collector/container.go index 21f9aec84d..5c0548feb4 100644 --- a/pkg/collector/container.go +++ b/pkg/collector/container.go @@ -20,10 +20,9 @@ import ( "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" - "github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters" - "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters" "github.com/open-telemetry/opentelemetry-operator/pkg/naming" )