Remove SessionTicketsDisabled code after Go 1.21 update#30220
Remove SessionTicketsDisabled code after Go 1.21 update#30220
Conversation
Removed lines of code in lib/auth/helpers.go and lib/utils/tls.go that disabled session tickets and managed client session cache. The behavior has changed in Go 1.21, and after the upgrade, the cache may hold old certificates that were invalidated. This change may have a performance impact and should not be backported before full performance testing that we do during the test plan.
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Curious to see what impact this may have.
LGTM as long as others are happy to have it land.
rosstimothy
left a comment
rosstimothy
left a comment
There was a problem hiding this comment.
I'm happy to merge this and see what the impacts are, but, I think we first need to define our success criteria. Will the consequences of this change be evident from our existing cases in the Test Plan? Should we add a scenario that explicitly covers this and test it on both v13 latest and v14? Without any historical numbers how will we know if we are better or worse?
public-teleport-github-review-bot
Bot
removed the request for review
from ibeckermayer
| config.SessionTicketsDisabled = false | ||
| config.ClientSessionCache = tls.NewLRUClientSessionCache(DefaultLRUCapacity) |
There was a problem hiding this comment.
Note the default for SessionTicketsDisabled is already false. This change will disable client session cache, but still support session tickets for anything using this config as a server. If the intent here is to disable session tickets altogether then we should put config.SessionTicketsDisabled = true
There was a problem hiding this comment.
If we decide to keep session tickets enabled for servers, then I think we should also keep the client session caching and just fix the tests to not clone the cache. That can be done in the test code by calling SetupTLSConfig after we clone the client, or by setting it to nil or something
|
@jakule @rosstimothy let's make a decision on this one so that it doesn't rot. Close or merge? |
+1 for closing |
5 participants
Removed lines of code in lib/auth/helpers.go and lib/utils/tls.go that disabled session tickets and managed client session cache. The behavior has changed in Go 1.21, and after the upgrade, the cache may hold old certificates that were invalidated.
This change may have a performance impact and should not be backported before full performance testing that we do during the test plan.
Related #30201