Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post launch Log level control for the Server #4880

Merged
merged 20 commits into from
Mar 11, 2024
Merged

Conversation

edwbuck
Copy link
Contributor

@edwbuck edwbuck commented Feb 13, 2024

fixes #4785

Code for server log level adjustment is complete and functional. Unit tests are being added. Core solution is ready for initial review.

Affected functionality
After the spire server is running, one can request its logger caracteristics with the command

spire-server logger get

Which will output the current logger details

Logger Level  : info
Logger Default: info

Where Logger Level indicates the current logger's level and Logger Default indicates the logger's level according to the configuration at server launch time.

To adjust the log level, on can use the command

spire-server logger set -level debug

Which will adjust the logger's log level and return the updated logger details

Logger Level  : debug
Logger Default: info

Supported logger set values include (panic, fatal, error, warn, info, debug, trace, and default). default will set the Logger Level to the value held in Logger Default

Logger adjustments for the spire-agent will be added soon. Currently, I'm updating the unit testing to obtain more code coverage. Functionality has only been manually tested.

spire-server logger set

Is a no-op, as no directive on what to set (or what value to set it to) is provided.

Description of change
The spire-server executable has two new cli commands logger get and logger set which access the dynamically configurable logger.

logger get builds a grpc call (loggerv1) logger.GetLogger in the new spire-api-sdk logger (v1) api, as detailed in PR spiffe/spire-api-sdk#55

logger set may, if -level <value> is passed, build a grpc call (loggerv1) logger.SetLogLevel, detailed in the same PR spiffe/spire-api-sdk#55

The spire server then processes the request, adjusts the logrus logger level, and returns the system's Logger details, which indicates both the logger's current level and the logger's default level.

Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @edwbuck. Here's some preliminary feedback.

pkg/common/api/middleware/names.go Outdated Show resolved Hide resolved
cmd/spire-server/cli/run/run.go Outdated Show resolved Hide resolved
pkg/server/endpoints/config.go Outdated Show resolved Hide resolved
pkg/server/endpoints/config.go Outdated Show resolved Hide resolved
pkg/server/endpoints/endpoints.go Outdated Show resolved Hide resolved
pkg/server/endpoints/endpoints.go Outdated Show resolved Hide resolved
pkg/server/api/logger/v1/service.go Show resolved Hide resolved
pkg/server/api/logger/v1/service.go Outdated Show resolved Hide resolved
@azdagron azdagron added this to the 1.9.1 milestone Mar 5, 2024
@edwbuck edwbuck changed the title Post launch Log level control Post launch Log level control for the Server Mar 6, 2024
@edwbuck
Copy link
Contributor Author

edwbuck commented Mar 6, 2024

@azdagron All checks passing except the docker ones which are failing on the UID:GID processing not properly populating (an issue being looked at in #4903)

azdagron
azdagron previously approved these changes Mar 6, 2024
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for the contribution, @edwbuck.

}

// mock implementation for GetLogger
func (s *mockLoggerServer) GetLogger(_ context.Context, _ *loggerv1.GetLoggerRequest) (*types.Logger, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (s *mockLoggerServer) GetLogger(_ context.Context, _ *loggerv1.GetLoggerRequest) (*types.Logger, error) {
func (s *mockLoggerServer) GetLogger(context.Context, *loggerv1.GetLoggerRequest) (*types.Logger, error) {

return s.returnLogger, s.returnErr
}

func (s *mockLoggerServer) ResetLogLevel(_ context.Context, _ *loggerv1.ResetLogLevelRequest) (*types.Logger, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (s *mockLoggerServer) ResetLogLevel(_ context.Context, _ *loggerv1.ResetLogLevelRequest) (*types.Logger, error) {
func (s *mockLoggerServer) ResetLogLevel(context.Context, *loggerv1.ResetLogLevelRequest) (*types.Logger, error) {

// The routine that executes the command
func (c *setCommand) Run(ctx context.Context, _ *commoncli.Env, serverClient util.ServerClient) error {
if c.newLevel == "" {
return fmt.Errorf("a value (-level) must be set")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return fmt.Errorf("a value (-level) must be set")
return errors.New("a value (-level) must be set")

}
apiLevel, found := serverlogger.APILevel[logrusLevel]
if !found {
return fmt.Errorf("the logrus level %d could not be transformed into an api log level", logrusLevel)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm decimal has not too much value for a user, what do you think about returning level that is string instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look carefully, this is an error message for returning a log level that doesn't have an associated string

level := strings.ToLower(c.newLevel)
var logger *apitype.Logger
var err error
if level == "launch" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is confusing to me to have launch to reset as part of level,
may we add a reset option instead?

--resetLevel 

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in case we want to keep using launch, I think we must add more documentation about what that launch means

if req.NewLevel == apitype.LogLevel_UNSPECIFIED {
return nil, fmt.Errorf("Invalid request, NewLevel value cannot be LogLevel_UNSPECIFIED")
}
service.log.WithFields(logrus.Fields{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as previous function can you get logger from context and use that to add logs?
the service.Log must be used only to change levels
you can get it using:

	ctxLog := rpccontext.Logger(ctx)

}

func (service *Service) SetLogLevel(_ context.Context, req *loggerv1.SetLogLevelRequest) (*apitype.Logger, error) {
if req.NewLevel == apitype.LogLevel_UNSPECIFIED {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add:

	rpccontext.AddRPCAuditFields(ctx, logrus.Fields{telemetry.LogLevel: req.NewLevel})

this is to add get parammeters to audit logs

CurrentLevel: APILevel[service.log.GetLevel()],
LaunchLevel: APILevel[service.launchLevel],
}
return logger, nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add? (it is to execute audit log)

	rpccontext.AuditRPC(ctx)

}

func (service *Service) ResetLogLevel(_ context.Context, _ *loggerv1.ResetLogLevelRequest) (*apitype.Logger, error) {
service.log.Info("ResetLogLevel Called")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use logger from context?

CurrentLevel: APILevel[service.log.GetLevel()],
LaunchLevel: APILevel[service.launchLevel],
}
return logger, nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add? (it is to execute audit log)

	rpccontext.AuditRPC(ctx)

Add list command to cli.

Add some of the receiving services in the server "run" instance.

Add the "registry" of loggers (currently a btree)

Beginning unit testing on naming ordering.

Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
JSON now outputs log levels as their logrus lower-case strings.

Some unit testing on logger.get cli commands.

Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Complete unit testing for spire-server cli.

Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
logrus.Logger.

Signed-off-by: Edwin Buck <edwbuck@gmail.com>
The Logger service fields are now private, the service config fields
remain public.

The Logger variable / field names are now log (or Log) to match
conventions.

The logger package now adds a Logger type which extends the logrus.Logger
interface with set log level and get log level funcs.

Signed-off-by: Edwin Buck <edwbuck@gmail.com>
This fixes the inability of this unit test to intialize the new GRPC
logger service, as it requires a Logger type that also includes the
ability to change log levels.

Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
endpoints_test.go

Signed-off-by: Edwin Buck <edwbuck@gmail.com>
is fixed.

Removed allow_admin from policy_data.json until determination can be
made to make this allow_admin

Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
@azdagron azdagron merged commit a8d547c into spiffe:main Mar 11, 2024
32 checks passed
rushi47 pushed a commit to rushi47/spire that referenced this pull request Apr 11, 2024
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support retuning of log level on spire-{server,agent}
3 participants