address review comments

Author: dashpole

cluster/gce/gci/configure-helper.sh

@@ -999,7 +999,6 @@ EOF
     cat <<EOF >/etc/srv/kubernetes/opentelemetry_config.yaml
 apiVersion: apiserver.k8s.io/v1alpha1
 kind: OpenTelemetryClientConfiguration
-url: localhost:55680
 EOF
   fi
 

staging/src/k8s.io/apiserver/pkg/opentelemetry/config.go

@@ -29,7 +29,12 @@ import (
 	"sigs.k8s.io/yaml"
 )
 
-var cfgScheme = runtime.NewScheme()
+var (
+	cfgScheme = runtime.NewScheme()
+
+	defaultPort = int32(55680)
+	defaultURL  = "localhost:55680"
+)
 
 func init() {
 	install.Install(cfgScheme)
@@ -38,7 +43,7 @@ func init() {
 // ReadOpenTelemetryConfiguration reads the opentelemetry configuration from a file
 func ReadOpenTelemetryConfiguration(configFilePath string) (*apiserver.OpenTelemetryClientConfiguration, error) {
 	if configFilePath == "" {
-		return nil, nil
+		return nil, fmt.Errorf("opentelemetry config file was empty")
 	}
 	data, err := ioutil.ReadFile(configFilePath)
 	if err != nil {
@@ -61,6 +66,21 @@ func ReadOpenTelemetryConfiguration(configFilePath string) (*apiserver.OpenTelem
 	return internalConfig, nil
 }
 
+// DefaultOpenTelemetryConfiguration defaults unset fields in the OpenTelemetryClientConfiguration
+func DefaultOpenTelemetryConfiguration(config *apiserver.OpenTelemetryClientConfiguration) {
+	if config == nil {
+		return
+	}
+	// Default the service port to the default OTLP port
+	if config.Service != nil && config.Service.Port == nil {
+		config.Service.Port = &defaultPort
+	}
+	// If niether URL or service is set, use the default URL
+	if config.Service == nil && config.URL == nil {
+		config.URL = &defaultURL
+	}
+}
+
 // ValidateOpenTelemetryConfiguration validates the opentelemetry configuration
 func ValidateOpenTelemetryConfiguration(config *apiserver.OpenTelemetryClientConfiguration) field.ErrorList {
 	allErrs := field.ErrorList{}

staging/src/k8s.io/apiserver/pkg/opentelemetry/config_test.go

@@ -131,6 +131,73 @@ spec:
 	}
 }
 
+func TestDefaultOpenTelemetryConfiguration(t *testing.T) {
+	otherPort := int32(12378)
+	otherURL := "foo:12345"
+	testcases := []struct {
+		name                string
+		expectedURL         *string
+		expectedServicePort *int32
+		config              *apiserver.OpenTelemetryClientConfiguration
+	}{
+		{
+			name:        "all-empty",
+			expectedURL: &defaultURL,
+			config: &apiserver.OpenTelemetryClientConfiguration{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "",
+					APIVersion: "",
+				},
+			},
+		},
+		{
+			name:                "empty-service",
+			expectedServicePort: &defaultPort,
+			config: &apiserver.OpenTelemetryClientConfiguration{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "",
+					APIVersion: "",
+				},
+				Service: &apiserver.ServiceReference{},
+			},
+		},
+		{
+			name:        "existing-url",
+			expectedURL: &otherURL,
+			config: &apiserver.OpenTelemetryClientConfiguration{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "",
+					APIVersion: "",
+				},
+				URL: &otherURL,
+			},
+		},
+		{
+			name:                "existing-service-port",
+			expectedServicePort: &otherPort,
+			config: &apiserver.OpenTelemetryClientConfiguration{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "",
+					APIVersion: "",
+				},
+				Service: &apiserver.ServiceReference{Port: &otherPort},
+			},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			DefaultOpenTelemetryConfiguration(tc.config)
+			if tc.expectedURL != nil && *tc.expectedURL != *tc.config.URL {
+				t.Errorf("Calling DefaultOpenTelemetryConfiguration expected URL %v, got %v", *tc.expectedURL, *tc.config.URL)
+			}
+			if tc.expectedServicePort != nil && *tc.expectedServicePort != *tc.config.Service.Port {
+				t.Errorf("Calling DefaultOpenTelemetryConfiguration expected Service.Port %v, got %v", *tc.expectedServicePort, *tc.config.Service.Port)
+			}
+		})
+	}
+}
+
 func TestValidateOpenTelemetryConfiguration(t *testing.T) {
 	port := int32(12378)
 	testcases := []struct {

staging/src/k8s.io/apiserver/pkg/server/options/opentelemetry.go

@@ -24,11 +24,11 @@ import (
 	"github.com/spf13/pflag"
 	"go.opentelemetry.io/otel/exporters/otlp"
 	"google.golang.org/grpc"
-	"k8s.io/utils/path"
 
 	"k8s.io/apiserver/pkg/opentelemetry"
 	"k8s.io/apiserver/pkg/server/egressselector"
 	"k8s.io/component-base/traces"
+	"k8s.io/utils/path"
 )
 
 // OpenTelemetryOptions contain configuration options for opentelemetry
@@ -55,24 +55,21 @@ func (o *OpenTelemetryOptions) AddFlags(fs *pflag.FlagSet) {
 
 // Apply adds the opentelemetry settings to the global configuration.
 func (o *OpenTelemetryOptions) Apply(es *egressselector.EgressSelector) error {
-	if o == nil {
+	if o == nil || o.ConfigFile == "" {
 		return nil
 	}
 
 	npConfig, err := opentelemetry.ReadOpenTelemetryConfiguration(o.ConfigFile)
 	if err != nil {
 		return fmt.Errorf("failed to read opentelemetry config: %v", err)
 	}
+
+	opentelemetry.DefaultOpenTelemetryConfiguration(npConfig)
 	errs := opentelemetry.ValidateOpenTelemetryConfiguration(npConfig)
 	if len(errs) > 0 {
 		return fmt.Errorf("failed to validate opentelemetry configuration: %v", errs.ToAggregate())
 	}
 
-	if npConfig == nil {
-		// No config file was specified, so don't enable exporting
-		return nil
-	}
-
 	opts := []otlp.ExporterOption{}
 	if npConfig.URL != nil {
 		opts = append(opts, otlp.WithAddress(*npConfig.URL))
@@ -92,12 +89,7 @@ func (o *OpenTelemetryOptions) Apply(es *egressselector.EgressSelector) error {
 		}
 	}
 	if npConfig.Service != nil {
-		// Default port is 55680
-		port := int32(55680)
-		if npConfig.Service.Port != nil {
-			port = *npConfig.Service.Port
-		}
-		addr := fmt.Sprintf("%s.%s:%d", npConfig.Service.Name, npConfig.Service.Namespace, port)
+		addr := fmt.Sprintf("%s.%s:%d", npConfig.Service.Name, npConfig.Service.Namespace, *npConfig.Service.Port)
 		opts = append(opts, otlp.WithAddress(addr))
 
 		if es != nil {
@@ -125,16 +117,13 @@ func (o *OpenTelemetryOptions) Apply(es *egressselector.EgressSelector) error {
 }
 
 // Validate verifies flags passed to OpenTelemetryOptions.
-func (o *OpenTelemetryOptions) Validate() []error {
+func (o *OpenTelemetryOptions) Validate() (errs []error) {
 	if o == nil || o.ConfigFile == "" {
-		return nil
+		return
 	}
 
-	errs := []error{}
-
 	if exists, err := path.Exists(path.CheckFollowSymlink, o.ConfigFile); !exists || err != nil {
 		errs = append(errs, fmt.Errorf("opentelemetry-config-file %s does not exist", o.ConfigFile))
 	}
-
-	return errs
+	return
 }