Convert TeleportProcess to use slog for logging #38551
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rosstimothy
added
the
no-changelog
rosstimothy
force-pushed
the
tross/process_slog
branch
from
39b1d0c to
3b02128
Compare
kimlisa
approved these changes
Mar 4, 2024
| @@ -139,7 +139,7 @@ func (process *TeleportProcess) authServerTooOld(resp *proto.PingResponse) error | |||
|
|
|||
| if serverVersion.Major < teleportVersion.Major { | |||
| if process.Config.SkipVersionCheck { | |||
| process.log.Warnf("Teleport instance is too new. This instance is running v%d. The auth server is running v%d and only supports instances on v%d or v%d.", teleportVersion.Major, serverVersion.Major, serverVersion.Major, serverVersion.Major-1) | |||
| process.logger.WarnContext(process.ExitContext(), "This instance is too new. Using a newer major version than the Auth server is unsupported and may impair functionality.", "version", teleportVersion.Major, "auth_version", serverVersion.Major, "supported_versions", []int64{serverVersion.Major, serverVersion.Major - 1}) | |||
There was a problem hiding this comment.
i wanted to understand how this looks when printed. I understand how the previous will look like in my head, but i'm having trouble with this one 😓
There was a problem hiding this comment.
This should look like the following
2024-03-05T00:19:15-05:00 WARN [PROC:1] This instance is too new. Using a newer major version than the Auth server is unsupported and may impair functionality. pid:46154.1 version:15.1.0 auth_version:14.2.0 supported_versions: [14, 13] service/connect.go:142
3b02128 to
f1f5a30
Compare
|
Friendly ping @tcsc |
|
@tcsc could you please review? |
rosstimothy
force-pushed
the
tross/process_slog
branch
from
f1f5a30 to
25dc99d
Compare
Begins the conversion away from logrus to slog for all messages emitted from TeleportProcess. The logrus logger should now only be in use for dependency injection to child components that haven't been converted yet. There are still instances of the logrus logger in the process configuration to emit messages, they will be converted in a follow up PR.
rosstimothy
force-pushed
the
tross/process_slog
branch
from
25dc99d to
90f78d0
Compare
tcsc
approved these changes
Mar 12, 2024
rosstimothy
added a commit
that referenced
this pull request
Mar 13, 2024
The change to migrate the TeleportProcess to use slog in #38551 was preventing the regular expressions used by the operator tests to determine when auth and proxy were ready. This lead to the tests hanging indefinitely until the go test framework timed out. To remediate, the regular expressions have been updated to match the output when using slog. For example, the test was expecting the output in the format of: ``` Auth service 15.0.0:v15.0.0-0-ge126e8cd is starting on 192.168.50.108:3025. pid:61933.1 service/service.go:2072 ``` #38551 moved the listen address to a field instead of being formatted in the message: ``` Auth service is starting. pid:54159.1 version:16.0.0-dev git_ref:api/v13.4.16-31-g2b9ae35e31 listen_address:127.0.0.1:63152 service/service.go:2107 ```
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 14, 2024
The change to migrate the TeleportProcess to use slog in #38551 was preventing the regular expressions used by the operator tests to determine when auth and proxy were ready. This lead to the tests hanging indefinitely until the go test framework timed out. To remediate, the regular expressions have been updated to match the output when using slog. For example, the test was expecting the output in the format of: ``` Auth service 15.0.0:v15.0.0-0-ge126e8cd is starting on 192.168.50.108:3025. pid:61933.1 service/service.go:2072 ``` #38551 moved the listen address to a field instead of being formatted in the message: ``` Auth service is starting. pid:54159.1 version:16.0.0-dev git_ref:api/v13.4.16-31-g2b9ae35e31 listen_address:127.0.0.1:63152 service/service.go:2107 ```
|
@rosstimothy I noticed that with this change my e2e test logs now print the teleport process identity private key and certs bytes. Look for this: Looks like the offending line is in lib/service/connect.go:158 where it does not use the client identity String method for some reason. |
|
|
The text handler does indeed do the right thing, but the json handler isn't calling String. |
rosstimothy
added a commit
that referenced
this pull request
Mar 18, 2024
Begins the conversion away from logrus to slog for all messages emitted from TeleportProcess. The logrus logger should now only be in use for dependency injection to child components that haven't been converted yet. There are still instances of the logrus logger in the process configuration to emit messages, they will be converted in a follow up PR.
rosstimothy
added a commit
that referenced
this pull request
Mar 28, 2024
Begins the conversion away from logrus to slog for all messages emitted from TeleportProcess. The logrus logger should now only be in use for dependency injection to child components that haven't been converted yet. There are still instances of the logrus logger in the process configuration to emit messages, they will be converted in a follow up PR.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 28, 2024
* Convert TeleportProcess to use slog for logging (#38551) Begins the conversion away from logrus to slog for all messages emitted from TeleportProcess. The logrus logger should now only be in use for dependency injection to child components that haven't been converted yet. There are still instances of the logrus logger in the process configuration to emit messages, they will be converted in a follow up PR. * Make slog attributes output the same content regardless of format (#39428) * Make slog attributes output the same content regardless of format There were a few instances of fmt.Stringers being passed to slog as attributes to delay their evaluation as per the performance considerations of slog: https://pkg.go.dev/log/slog#hdr-Performance_considerations. However, this resulted in the json handler emitting different content since they check to see if attributes are json.Marshalers and not fmt.Stringers. To rectify the situation, a new slog.LogValuer implementation was added that wraps a fmt.Stringer so that attributes can be lazily constructed while producing the same output no matter the log output format. * fix: wrap identity and correct typo * Fix integrations ssh service tests (#39543) Similar to #39315, this updates the integrations test suite to use a regular expression that will match output to know when the ssh_service is ready now that Teleport is using slog instead of logrus.
marcoandredinis
added a commit
to gravitational/teleport-plugins
that referenced
this pull request
Apr 2, 2024
This new version of teleport has the following relevant changes: * Teleport process logging uses `log/slog` For terraform tests we start a teleport binary and parse its output to understand when the Auth/Proxy service started and what ports are they listening on. We used a regex for that, but teleport migrated to `log/slog` and the regex no longer works. Migration PR: gravitational/teleport#38551 We had to fix the regex for integration tests in teleport: gravitational/teleport#39315 Terraform Tests also use that library, so after the regex changed, we must upgrade Teleport CI version to get the new log format. * Teleport API: `GetClusterNetworkingConfig` and `GetSessionRecordingConfig` never return a nil When developing the `ClusterMaitenanceConfig` we had to include a nil check, because if it was never configured, `GetClusterMaintenanceConfig` would return a nil object. This nil check was added to all SingleResource resources. For `ClusterNetworkingConfig` and `SessionRecordingConfig`, the `Get` operation never returns a nil resource and `staticcheck` linter was yelling about it. So, we had to create a new flag to ensure we only nil-checked the resources that can actually return a nil value. * Teleport Resource Metadata It is no longer recommended to use the `<Resource>.Metadata.ID` to check for cached responses. We are now using the revision field. During this change we also detected a miss-usage of an `error` variable and fixed that (could lead to a panic). LoginRules didn't have the `Revision` field, so we added it here: gravitational/teleport.e#3821 Unfortunately, that PR didn't merge in time for 15.2.0. However, that's ok because LoginRules are not cached. So, instead of waiting for a new release (15.2.1), we just removed the cache check.
marcoandredinis
added a commit
to gravitational/teleport-plugins
that referenced
this pull request
Apr 2, 2024
This new version of teleport has the following relevant changes: * Teleport process logging uses `log/slog` For terraform tests we start a teleport binary and parse its output to understand when the Auth/Proxy service started and what ports are they listening on. We used a regex for that, but teleport migrated to `log/slog` and the regex no longer works. Migration PR: gravitational/teleport#38551 We had to fix the regex for integration tests in teleport: gravitational/teleport#39315 Terraform Tests also use that library, so after the regex changed, we must upgrade Teleport CI version to get the new log format. * Teleport API: `GetClusterNetworkingConfig` and `GetSessionRecordingConfig` never return a nil When developing the `ClusterMaitenanceConfig` we had to include a nil check, because if it was never configured, `GetClusterMaintenanceConfig` would return a nil object. This nil check was added to all SingleResource resources. For `ClusterNetworkingConfig` and `SessionRecordingConfig`, the `Get` operation never returns a nil resource and `staticcheck` linter was yelling about it. So, we had to create a new flag to ensure we only nil-checked the resources that can actually return a nil value. * Teleport Resource Metadata It is no longer recommended to use the `<Resource>.Metadata.ID` to check for cached responses. We are now using the revision field. During this change we also detected a miss-usage of an `error` variable and fixed that (could lead to a panic). LoginRules didn't have the `Revision` field, so we added it here: gravitational/teleport.e#3821 Unfortunately, that PR didn't merge in time for 15.2.0. However, that's ok because LoginRules are not cached. So, instead of waiting for a new release (15.2.1), we just removed the cache check.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Begins the conversion away from logrus to slog for all messages emitted from TeleportProcess. The logrus logger should now only be in use for dependency injection to child components that haven't been converted yet. There are still instances of the logrus logger in the process configuration to emit messages, they will be converted in a follow up PR.