Addressed review feedback; removed getter, merged into action. Renamed and regenerated protobufs to reflect. Some tests to make codecov happy.

Signed-off-by: Matthew Burns <[email protected]>

Author: mattcburns

api/v1/diagnostic.proto

@@ -13,8 +13,8 @@ service Diagnostic {
     rpc Screenshot (ScreenshotRequest) returns (ScreenshotResponse);
     rpc ClearSystemEventLog (ClearSystemEventLogRequest) returns (ClearSystemEventLogResponse);
     rpc SendNMI (SendNMIRequest) returns (google.protobuf.Empty);
-    rpc GetSystemEventLog (GetSystemEventLogRequest) returns (GetSystemEventLogResponse);
-    rpc GetSystemEventLogRaw (GetSystemEventLogRawRequest) returns (GetSystemEventLogRawResponse);
+    rpc SystemEventLog (SystemEventLogRequest) returns (SystemEventLogResponse);
+    rpc SystemEventLogRaw (SystemEventLogRawRequest) returns (SystemEventLogRawResponse);
 }
 
 message ScreenshotRequest {
@@ -40,7 +40,7 @@ message SendNMIRequest {
     v1.Authn authn = 1;
 }
 
-message GetSystemEventLogRequest {
+message SystemEventLogRequest {
     v1.Authn authn = 1;
     v1.Vendor vendor = 2;
 }
@@ -52,15 +52,15 @@ message SystemEventLogEntry {
     string message = 4;
 }
 
-message GetSystemEventLogResponse {
+message SystemEventLogResponse {
     repeated SystemEventLogEntry events = 1;
 }
 
-message GetSystemEventLogRawRequest {
+message SystemEventLogRawRequest {
     v1.Authn authn = 1;
     v1.Vendor vendor = 2;
 }
 
-message GetSystemEventLogRawResponse {
+message SystemEventLogRawResponse {
     string log = 1;
 }

client/client.go

@@ -150,13 +150,14 @@ func ClearSystemEventLog(ctx context.Context, client v1.DiagnosticClient, taskCl
 func SendNMI(ctx context.Context, client v1.DiagnosticClient, request *v1.SendNMIRequest) error {
 	_, err := client.SendNMI(ctx, request)
 	return err
+}
 
-// GetSystemEventLog retrieves the System Event Log of the server.
-func GetSystemEventLog(ctx context.Context, client v1.DiagnosticClient, request *v1.GetSystemEventLogRequest) (*v1.GetSystemEventLogResponse, error) {
-	return client.GetSystemEventLog(ctx, request)
+// SystemEventLog retrieves the System Event Log of the server.
+func SystemEventLog(ctx context.Context, client v1.DiagnosticClient, request *v1.SystemEventLogRequest) (*v1.SystemEventLogResponse, error) {
+	return client.SystemEventLog(ctx, request)
 }
 
-// GetSystemEventLogRaw retrieves the System Event Log of the server.
-func GetSystemEventLogRaw(ctx context.Context, client v1.DiagnosticClient, request *v1.GetSystemEventLogRawRequest) (*v1.GetSystemEventLogRawResponse, error) {
-	return client.GetSystemEventLogRaw(ctx, request)
+// SystemEventLogRaw retrieves the System Event Log of the server.
+func SystemEventLogRaw(ctx context.Context, client v1.DiagnosticClient, request *v1.SystemEventLogRawRequest) (*v1.SystemEventLogRawResponse, error) {
+	return client.SystemEventLogRaw(ctx, request)
 }

cmd/sel.go

@@ -79,7 +79,7 @@ var (
 			defer conn.Close()
 			client := v1.NewDiagnosticClient(conn)
 
-			resp, err := v1Client.GetSystemEventLog(ctx, client, &v1.GetSystemEventLogRequest{
+			resp, err := v1Client.SystemEventLog(ctx, client, &v1.SystemEventLogRequest{
 				Authn: &v1.Authn{
 					Authn: &v1.Authn_DirectAuthn{
 						DirectAuthn: &v1.DirectAuthn{
@@ -123,7 +123,7 @@ var (
 			defer conn.Close()
 			client := v1.NewDiagnosticClient(conn)
 
-			resp, err := v1Client.GetSystemEventLogRaw(ctx, client, &v1.GetSystemEventLogRawRequest{
+			resp, err := v1Client.SystemEventLogRaw(ctx, client, &v1.SystemEventLogRawRequest{
 				Authn: &v1.Authn{
 					Authn: &v1.Authn_DirectAuthn{
 						DirectAuthn: &v1.DirectAuthn{

grpc/oob/diagnostic/diagnostic.go

@@ -8,13 +8,13 @@ import (
 
 type Action struct {
 	common.Accessory
-	ScreenshotRequest           *v1.ScreenshotRequest
-	ClearSystemEventLogRequest  *v1.ClearSystemEventLogRequest
-	SendNMIRequest              *v1.SendNMIRequest
-	ScreenshotRequest           *v1.ScreenshotRequest
-	ClearSystemEventLogRequest  *v1.ClearSystemEventLogRequest
-	GetSystemEventLogRequest    *v1.GetSystemEventLogRequest
-	GetSystemEventLogRawRequest *v1.GetSystemEventLogRawRequest
+	ScreenshotRequest          *v1.ScreenshotRequest
+	ClearSystemEventLogRequest *v1.ClearSystemEventLogRequest
+	SendNMIRequest             *v1.SendNMIRequest
+	SystemEventLogRequest      *v1.SystemEventLogRequest
+	SystemEventLogRawRequest   *v1.SystemEventLogRawRequest
+	ActionName                 string
+	RPCName                    string
 }
 
 // WithLogger adds a logr to an Action struct.
@@ -33,5 +33,14 @@ func WithStatusMessage(s chan string) Option {
 	}
 }
 
+// WithLabels adds the custom tracing and logging labels to an Action struct.
+func WithLabels(actionName string, rpcName string) Option {
+	return func(a *Action) error {
+		a.ActionName = actionName
+		a.RPCName = rpcName
+		return nil
+	}
+}
+
 // Option to add to an Actions.
 type Option func(a *Action) error

grpc/oob/diagnostic/getsel.go

@@ -18,53 +18,50 @@ import (
 	"go.opentelemetry.io/otel/trace"
 )
 
-func NewSystemEventLogGetter(req *v1.GetSystemEventLogRequest, opts ...Option) (*Action, error) {
+func NewSystemEventLogAction(req interface{}, opts ...Option) (*Action, error) {
 	a := &Action{}
-	a.GetSystemEventLogRequest = req
-	for _, opt := range opts {
-		err := opt(a)
-		if err != nil {
-			return nil, err
-		}
+	switch r := req.(type) {
+	case *v1.SystemEventLogRequest:
+		a.SystemEventLogRequest = r
+	case *v1.SystemEventLogRawRequest:
+		a.SystemEventLogRawRequest = r
+	default:
+		return nil, fmt.Errorf("unsupported request type")
 	}
-	return a, nil
-}
 
-func NewSystemEventLogRawGetter(req *v1.GetSystemEventLogRawRequest, opts ...Option) (*Action, error) {
-	a := &Action{}
-	a.GetSystemEventLogRawRequest = req
 	for _, opt := range opts {
 		err := opt(a)
 		if err != nil {
 			return nil, err
 		}
 	}
+
 	return a, nil
 }
 
-func (m Action) GetSystemEventLog(ctx context.Context) (result bmc.SystemEventLogEntries, err error) {
+func (m Action) SystemEventLog(ctx context.Context) (entries bmc.SystemEventLogEntries, raw string, err error) {
 	labels := prometheus.Labels{
 		"service": "diagnostic",
-		"action":  "get_system_event_log",
+		"action":  m.ActionName,
 	}
 
 	timer := prometheus.NewTimer(metrics.ActionDuration.With(labels))
 	defer timer.ObserveDuration()
 
 	tracer := otel.Tracer("pbnj")
-	ctx, span := tracer.Start(ctx, "diagnostic.GetSystemEventLog", trace.WithAttributes(
-		attribute.String("bmc.device", m.GetSystemEventLogRequest.GetAuthn().GetDirectAuthn().GetHost().GetHost()),
+	ctx, span := tracer.Start(ctx, "diagnostic."+m.RPCName, trace.WithAttributes(
+		attribute.String("bmc.device", m.SystemEventLogRequest.GetAuthn().GetDirectAuthn().GetHost().GetHost()),
 	))
 	defer span.End()
 
-	if v := m.GetSystemEventLogRequest.GetVendor(); v != nil {
+	if v := m.SystemEventLogRequest.GetVendor(); v != nil {
 		span.SetAttributes(attribute.String("bmc.vendor", v.GetName()))
 	}
 
-	host, user, password, parseErr := m.ParseAuth(m.GetSystemEventLogRequest.GetAuthn())
+	host, user, password, parseErr := m.ParseAuth(m.SystemEventLogRequest.GetAuthn())
 	if parseErr != nil {
 		span.SetStatus(codes.Error, "error parsing credentials: "+parseErr.Error())
-		return result, parseErr
+		return entries, raw, parseErr
 	}
 
 	span.SetAttributes(attribute.String("bmc.host", host), attribute.String("bmc.username", user))
@@ -75,96 +72,25 @@ func (m Action) GetSystemEventLog(ctx context.Context) (result bmc.SystemEventLo
 	}
 
 	client := bmclib.NewClient(host, user, password, opts...)
-	client.Registry.Drivers = client.Registry.Supports(providers.FeatureGetSystemEventLog)
-
-	m.SendStatusMessage("connecting to BMC")
-	err = client.Open(ctx)
-	meta := client.GetMetadata()
-	span.SetAttributes(attribute.StringSlice("bmc.open.providersAttempted", meta.ProvidersAttempted),
-		attribute.StringSlice("bmc.open.successfulOpenConns", meta.SuccessfulOpenConns))
-	if err != nil {
-		span.SetStatus(codes.Error, err.Error())
-		return nil, &repository.Error{
-			Code:    v1.Code_value["PERMISSION_DENIED"],
-			Message: err.Error(),
-		}
-	}
-	log := m.Log.WithValues("host", host, "user", user)
-	defer func() {
-		client.Close(ctx)
-		log.Info("closed connections", logMetadata(client.GetMetadata())...)
-	}()
-	log.Info("connected to BMC", logMetadata(client.GetMetadata())...)
-	m.SendStatusMessage("connected to BMC")
-
-	// Get the system event log
-	m.SendStatusMessage("getting system event log")
-	sel, err := client.GetSystemEventLog(ctx)
-	log = m.Log.WithValues(logMetadata(client.GetMetadata())...)
-	meta = client.GetMetadata()
-	span.SetAttributes(attribute.StringSlice("bmc.get_system_event_log.providersAttempted", meta.ProvidersAttempted),
-		attribute.StringSlice("bmc.get_system_event_log.successfulOpenConns", meta.SuccessfulOpenConns))
-	if err != nil {
-		log.Error(err, "error getting system event log")
-		span.SetStatus(codes.Error, "error getting system event log: "+err.Error())
-		m.SendStatusMessage(fmt.Sprintf("failed to get system event log %v", host))
-
-		return nil, &repository.Error{
-			Code:    v1.Code_value["UNKNOWN"],
-			Message: err.Error(),
-		}
-	}
-
-	span.SetStatus(codes.Ok, "")
-	log.Info("got system event log", logMetadata(client.GetMetadata())...)
-	m.SendStatusMessage(fmt.Sprintf("got system event log on %v", host))
-
-	return sel, nil
-}
-
-func (m Action) GetSystemEventLogRaw(ctx context.Context) (result string, err error) {
-	labels := prometheus.Labels{
-		"service": "diagnostic",
-		"action":  "get_system_event_log_raw",
-	}
-
-	timer := prometheus.NewTimer(metrics.ActionDuration.With(labels))
-	defer timer.ObserveDuration()
-
-	tracer := otel.Tracer("pbnj")
-	ctx, span := tracer.Start(ctx, "diagnostic.GetSystemEventLogRaw", trace.WithAttributes(
-		attribute.String("bmc.device", m.GetSystemEventLogRawRequest.GetAuthn().GetDirectAuthn().GetHost().GetHost()),
-	))
-	defer span.End()
 
-	if v := m.GetSystemEventLogRawRequest.GetVendor(); v != nil {
-		span.SetAttributes(attribute.String("bmc.vendor", v.GetName()))
+	// Set the driver(s) to use based on the request type
+	switch {
+	case m.SystemEventLogRequest != nil:
+		client.Registry.Drivers = client.Registry.Supports(providers.FeatureGetSystemEventLog)
+	case m.SystemEventLogRawRequest != nil:
+		client.Registry.Drivers = client.Registry.Supports(providers.FeatureGetSystemEventLogRaw)
+	default:
+		return entries, raw, fmt.Errorf("unsupported request type")
 	}
 
-	host, user, password, parseErr := m.ParseAuth(m.GetSystemEventLogRawRequest.GetAuthn())
-	if parseErr != nil {
-		span.SetStatus(codes.Error, "error parsing credentials: "+parseErr.Error())
-		return result, parseErr
-	}
-
-	span.SetAttributes(attribute.String("bmc.host", host), attribute.String("bmc.username", user))
-
-	opts := []bmclib.Option{
-		bmclib.WithLogger(m.Log),
-		bmclib.WithPerProviderTimeout(common.BMCTimeoutFromCtx(ctx)),
-	}
-
-	client := bmclib.NewClient(host, user, password, opts...)
-	client.Registry.Drivers = client.Registry.Supports(providers.FeatureGetSystemEventLogRaw)
-
 	m.SendStatusMessage("connecting to BMC")
 	err = client.Open(ctx)
 	meta := client.GetMetadata()
 	span.SetAttributes(attribute.StringSlice("bmc.open.providersAttempted", meta.ProvidersAttempted),
 		attribute.StringSlice("bmc.open.successfulOpenConns", meta.SuccessfulOpenConns))
 	if err != nil {
 		span.SetStatus(codes.Error, err.Error())
-		return "", &repository.Error{
+		return entries, raw, &repository.Error{
 			Code:    v1.Code_value["PERMISSION_DENIED"],
 			Message: err.Error(),
 		}
@@ -177,27 +103,37 @@ func (m Action) GetSystemEventLogRaw(ctx context.Context) (result string, err er
 	log.Info("connected to BMC", logMetadata(client.GetMetadata())...)
 	m.SendStatusMessage("connected to BMC")
 
-	// Get the system event log
-	m.SendStatusMessage("getting system event log")
-	sel, err := client.GetSystemEventLogRaw(ctx)
+	m.SendStatusMessage("getting " + m.ActionName + " on " + host)
+
+	switch {
+	case m.SystemEventLogRequest != nil:
+		// Get the system event log
+		entries, err = client.GetSystemEventLog(ctx)
+	case m.SystemEventLogRawRequest != nil:
+		// Get the system event log
+		raw, err = client.GetSystemEventLogRaw(ctx)
+	default:
+		return entries, raw, fmt.Errorf("unsupported request type")
+	}
+
 	log = m.Log.WithValues(logMetadata(client.GetMetadata())...)
 	meta = client.GetMetadata()
-	span.SetAttributes(attribute.StringSlice("bmc.get_system_event_log_raw.providersAttempted", meta.ProvidersAttempted),
-		attribute.StringSlice("bmc.get_system_event_log_raw.successfulOpenConns", meta.SuccessfulOpenConns))
+	span.SetAttributes(attribute.StringSlice("bmc."+m.ActionName+".providersAttempted", meta.ProvidersAttempted),
+		attribute.StringSlice("bmc."+m.ActionName+".successfulOpenConns", meta.SuccessfulOpenConns))
 	if err != nil {
-		log.Error(err, "error getting raw system event log")
-		span.SetStatus(codes.Error, "error getting raw system event log: "+err.Error())
-		m.SendStatusMessage(fmt.Sprintf("failed to get raw system event log %v", host))
+		log.Error(err, "error getting "+m.ActionName)
+		span.SetStatus(codes.Error, "error getting "+m.ActionName+": "+err.Error())
+		m.SendStatusMessage(fmt.Sprintf("failed to get "+m.ActionName+" %v", host))
 
-		return "", &repository.Error{
+		return entries, raw, &repository.Error{
 			Code:    v1.Code_value["UNKNOWN"],
 			Message: err.Error(),
 		}
 	}
 
 	span.SetStatus(codes.Ok, "")
-	log.Info("got raw system event log", logMetadata(client.GetMetadata())...)
-	m.SendStatusMessage(fmt.Sprintf("got raw system event log on %v", host))
+	log.Info("got "+m.ActionName, logMetadata(client.GetMetadata())...)
+	m.SendStatusMessage(fmt.Sprintf("got "+m.ActionName+" on %v", host))
 
-	return sel, nil
+	return entries, raw, nil
 }