Address code review feedback

- Use Server.ExternalWebUrl from config if --server flag not provided
- Remove cookie from authorization request (Authorization header is sufficient)
- Use structured response output instead of raw JSON
- Replace custom trimSuffix with strings.TrimSuffix from stdlib
- Rename scope section to "Logging Management Scopes"
- Hook ResetGlobalManager into server_utils.ResetTestState()
- Update LogLevelManager to accept context and errgroup for clean shutdown

Co-authored-by: bbockelm <[email protected]>

Author: Copilot

cmd/logging_set_level.go

@@ -27,12 +27,15 @@ import (
 	"net/url"
 	"path"
 	"strconv"
+	"strings"
+	"time"
 
 	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 
 	"github.com/pelicanplatform/pelican/config"
+	"github.com/pelicanplatform/pelican/param"
 )
 
 var (
@@ -75,8 +78,16 @@ func setLogLevel(cmd *cobra.Command, args []string) error {
 		return errors.New("Duration must be a positive integer (seconds)")
 	}
 
-	// Construct API URL
-	targetURL, err := constructLoggingApiURL(loggingServerURLStr)
+	// Construct API URL - use config if server URL not provided
+	serverURL := loggingServerURLStr
+	if serverURL == "" {
+		serverURL = param.Server_ExternalWebUrl.GetString()
+		if serverURL == "" {
+			return errors.New("Server URL must be provided via --server flag or Server.ExternalWebUrl config")
+		}
+	}
+	
+	targetURL, err := constructLoggingApiURL(serverURL)
 	if err != nil {
 		return err
 	}
@@ -92,8 +103,13 @@ func setLogLevel(cmd *cobra.Command, args []string) error {
 		return errors.Wrap(err, "Failed to marshal request payload")
 	}
 
-	// Get admin token
-	tok, err := fetchOrGenerateWebAPIAdminToken(loggingServerURLStr, loggingTokenLocation)
+	// Get admin token - use config for server URL if not provided
+	serverURL := loggingServerURLStr
+	if serverURL == "" {
+		serverURL = param.Server_ExternalWebUrl.GetString()
+	}
+	
+	tok, err := fetchOrGenerateWebAPIAdminToken(serverURL, loggingTokenLocation)
 	if err != nil {
 		return err
 	}
@@ -105,7 +121,6 @@ func setLogLevel(cmd *cobra.Command, args []string) error {
 	}
 	req.Header.Set("Content-Type", "application/json")
 	req.Header.Set("Authorization", "Bearer "+tok)
-	req.AddCookie(&http.Cookie{Name: "login", Value: tok})
 	req.Header.Set("Accept", "application/json")
 	req.Header.Set("User-Agent", "pelican-client/"+config.GetVersion())
 
@@ -122,22 +137,29 @@ func setLogLevel(cmd *cobra.Command, args []string) error {
 	}
 
 	// Parse response
-	var response map[string]interface{}
+	type LogLevelChangeResponse struct {
+		ChangeID   string    `json:"changeId"`
+		Level      string    `json:"level"`
+		EndTime    time.Time `json:"endTime"`
+		Remaining  int       `json:"remainingSeconds"`
+	}
+	
+	var response LogLevelChangeResponse
 	if err := json.Unmarshal(bodyBytes, &response); err != nil {
 		return errors.Wrap(err, "Failed to parse server response")
 	}
 
-	fmt.Println("Log level change successful:")
-	prettyJSON, _ := json.MarshalIndent(response, "", "  ")
-	fmt.Println(string(prettyJSON))
+	fmt.Printf("Log level successfully changed to '%s' for %d seconds\n", response.Level, response.Remaining)
+	fmt.Printf("Change ID: %s\n", response.ChangeID)
+	fmt.Printf("Will revert at: %s\n", response.EndTime.Format(time.RFC3339))
 	return nil
 }
 
 func constructLoggingApiURL(serverURLStr string) (*url.URL, error) {
 	if serverURLStr == "" {
 		return nil, errors.New("The --server flag providing the server's web URL is required")
 	}
-	serverURLStr = trimSuffix(serverURLStr, "/") // Normalize URL
+	serverURLStr = strings.TrimSuffix(serverURLStr, "/") // Normalize URL
 	baseURL, err := url.Parse(serverURLStr)
 	if err != nil {
 		return nil, errors.Wrapf(err, "Invalid server URL format: %s", serverURLStr)
@@ -157,9 +179,4 @@ func constructLoggingApiURL(serverURLStr string) (*url.URL, error) {
 	return targetURL, nil
 }
 
-func trimSuffix(s, suffix string) string {
-	if len(s) >= len(suffix) && s[len(s)-len(suffix):] == suffix {
-		return s[:len(s)-len(suffix)]
-	}
-	return s
-}
+

config/config.go

@@ -891,7 +891,7 @@ func setLoggingInternal() error {
 	SetLogging(level)
 
 	// Initialize the log level manager with our custom functions after setting up logging
-	logging.InitLogLevelManager(SetLogging, GetEffectiveLogLevel)
+	logging.InitLogLevelManager(nil, nil, SetLogging, GetEffectiveLogLevel)
 
 	return nil
 }

docs/scopes.yaml

@@ -82,7 +82,7 @@ issuedBy: ["*"]
 acceptedBy: ["*"]
 ---
 ############################
-#      Pelican Scopes      #
+# Logging Management Scopes#
 ############################
 name: pelican.logging_modify
 description: >-

logging/level_manager.go

@@ -24,6 +24,7 @@ import (
 	"time"
 
 	log "github.com/sirupsen/logrus"
+	"golang.org/x/sync/errgroup"
 )
 
 type (
@@ -41,7 +42,7 @@ type (
 		activeChanges   map[string]*LogLevelChange // key is ChangeID
 		baseGlobalLevel log.Level
 		ctx             context.Context
-		cancel          context.CancelFunc
+		egrp            *errgroup.Group
 		// Callback function to actually set the log level
 		// This allows us to avoid import cycles with the config package
 		setLevelFunc func(log.Level)
@@ -68,34 +69,48 @@ func ResetGlobalManager() {
 
 // InitLogLevelManager initializes the global log level manager with custom functions
 // This is called by the config package to inject the proper functions without creating an import cycle
-func InitLogLevelManager(setFunc func(log.Level), getFunc func() log.Level) {
+// If egrp is nil, a new errgroup will be created for the background goroutine
+func InitLogLevelManager(ctx context.Context, egrp *errgroup.Group, setFunc func(log.Level), getFunc func() log.Level) {
 	globalManagerOnce.Do(func() {
-		ctx, cancel := context.WithCancel(context.Background())
+		if ctx == nil {
+			ctx = context.Background()
+		}
+		if egrp == nil {
+			egrp = &errgroup.Group{}
+		}
+		
 		globalManager = &LogLevelManager{
 			activeChanges:   make(map[string]*LogLevelChange),
 			baseGlobalLevel: getFunc(),
 			ctx:             ctx,
-			cancel:          cancel,
+			egrp:            egrp,
 			setLevelFunc:    setFunc,
 			getLevelFunc:    getFunc,
 		}
-		go globalManager.manageLogLevels()
+		egrp.Go(func() error {
+			globalManager.manageLogLevels()
+			return nil
+		})
 	})
 }
 
 // GetLogLevelManager returns the global log level manager instance
 func GetLogLevelManager() *LogLevelManager {
 	globalManagerOnce.Do(func() {
-		ctx, cancel := context.WithCancel(context.Background())
+		ctx := context.Background()
+		egrp := &errgroup.Group{}
 		globalManager = &LogLevelManager{
 			activeChanges:   make(map[string]*LogLevelChange),
 			baseGlobalLevel: defaultGetLevelFunc(),
 			ctx:             ctx,
-			cancel:          cancel,
+			egrp:            egrp,
 			setLevelFunc:    defaultSetLevelFunc,
 			getLevelFunc:    defaultGetLevelFunc,
 		}
-		go globalManager.manageLogLevels()
+		egrp.Go(func() error {
+			globalManager.manageLogLevels()
+			return nil
+		})
 	})
 	return globalManager
 }
@@ -219,9 +234,8 @@ func (m *LogLevelManager) checkExpiredChanges() {
 
 // Shutdown stops the log level manager
 func (m *LogLevelManager) Shutdown() {
-	if m.cancel != nil {
-		m.cancel()
-	}
+	// Context cancellation is handled by the caller
+	// This method is kept for compatibility
 }
 
 // SetBaseLevel updates the base log level (called when configuration changes)

server_utils/server_utils.go

@@ -314,6 +314,7 @@ func ResetTestState() {
 	config.ResetConfig()
 	ResetOriginExports()
 	logging.ResetLogFlush()
+	logging.ResetGlobalManager()
 	baseAdOnce = sync.Once{}
 	baseAd = server_structs.ServerBaseAd{}
 	baseAdErr = nil