Expose client tools auto update for find endpoint #46785
Conversation
vapopov
added
backport/branch/v14
no-changelog
vapopov
force-pushed
the
vapopov/add-webapi-find-autoupdate-fields
branch
from
21709ac
to
61b2b93
Compare
lib/web/apiserver.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| autoUpdateConfig, err := h.GetAccessPoint().GetAutoUpdateConfig(r.Context()) |
There was a problem hiding this comment.
nit: the auth preference above is retrieved using h.cfg.AccessPoint, let's pick one and be consistent
api/client/webclient/webclient.go
Outdated
| // ToolsVersion defines the version of {tsh, tctl} for client auto update. | ||
| ToolsVersion string `json:"tools_version"` | ||
| // ToolsAutoupdate enables client auto update feature. | ||
| ToolsAutoupdate bool `json:"tools_autoupdate"` |
There was a problem hiding this comment.
Should the auto update information all be contained within another struct instead of adding each individual field to the response?
There was a problem hiding this comment.
grouping fields to another struct would be better, will change this one
lib/web/apiserver.go
Outdated
| } | ||
|
|
||
| autoUpdateConfig, err := h.GetAccessPoint().GetAutoUpdateConfig(r.Context()) | ||
| if err != nil && !trace.IsNotFound(err) { |
There was a problem hiding this comment.
I think it may also be possible to get a NotImplemented error back if the Proxy is newer than Auth. Even though this should be reading from the cache most of the time, a cluster admin can disable the cache, in which case this would make an RPC request to the old auth instance that doesn't implement the autoupdate service.
lib/web/apiserver.go
Outdated
| return nil, trace.Wrap(err) | ||
| } else if err == nil { | ||
| response.ToolsAutoupdate = autoUpdateConfig.GetSpec().GetToolsAutoupdate() | ||
| } | ||
|
|
||
| autoUpdateVersion, err := h.GetAccessPoint().GetAutoUpdateVersion(r.Context()) | ||
| if err != nil && !trace.IsNotFound(err) { | ||
| return nil, trace.Wrap(err) |
There was a problem hiding this comment.
Should failing to retrieve auto update information prevent the response entirely?
There was a problem hiding this comment.
I added logging instead, but we might generate a lot of log messages with this error message in frequent requests to this endpoint
Log error instead returning error Add tests auto update settings in find endpoint Add check for not implemented error
api/client/webclient/webclient.go
Outdated
| // ToolsVersion defines the version of {tsh, tctl} for client auto update. | ||
| ToolsVersion string `json:"tools_version"` | ||
| // ToolsAutoupdate enables client auto update feature. | ||
| ToolsAutoupdate bool `json:"tools_autoupdate"` |
There was a problem hiding this comment.
The json tag for the AutoUpdate field in the PingResponse is auto_update - we should be consistent with the tag here.
| ToolsAutoupdate bool `json:"tools_autoupdate"` | |
| ToolsAutoUpdate bool `json:"tools_auto_update"` |
lib/web/apiserver.go
Outdated
| } | ||
|
|
||
| autoUpdateConfig, err := h.cfg.AccessPoint.GetAutoUpdateConfig(r.Context()) | ||
| // TODO(vapopov) DELETE IN v18.0.0 |
There was a problem hiding this comment.
Suggestion: make the deletion note clearer that its only the not found and not implemented checks that should be removed in v18
lib/web/apiserver.go
Outdated
| } | ||
|
|
||
| autoUpdateVersion, err := h.cfg.AccessPoint.GetAutoUpdateVersion(r.Context()) | ||
| // TODO(vapopov) DELETE IN v18.0.0 |
There was a problem hiding this comment.
Suggestion: make the deletion note clearer that its only the not found and not implemented checks that should be removed in v18
lib/web/apiserver_ping_test.go
Outdated
| assert.True(t, pr.AutoUpdate.ToolsAutoupdate, "tools_autoupdate must be enabled") | ||
| assert.Equal(t, teleport.Version, pr.AutoUpdate.ToolsVersion, "tools_version must be equal to current version") |
There was a problem hiding this comment.
Suggestion: decouple the json tags from the message
| assert.True(t, pr.AutoUpdate.ToolsAutoupdate, "tools_autoupdate must be enabled") | |
| assert.Equal(t, teleport.Version, pr.AutoUpdate.ToolsVersion, "tools_version must be equal to current version") | |
| assert.True(t, pr.AutoUpdate.ToolsAutoupdate, "tools auto update must be enabled") | |
| assert.Equal(t, teleport.Version, pr.AutoUpdate.ToolsVersion, "tools version must be equal to current version") |
| _, err = env.server.Auth().UpsertAutoUpdateVersion(ctx, version) | ||
| require.NoError(t, err) | ||
|
|
||
| req, err := http.NewRequest(http.MethodGet, proxy.newClient(t).Endpoint("webapi", "find"), nil) |
There was a problem hiding this comment.
Suggestion: add a test case that covers the default values when none of the auto update configurations are populated or there is an error retrieving the configurations?
In this PR added exposing client auto update fields (is enabled, and required client tools version for update)
Related: https://github.com/gravitational/teleport/pull/39805/files#diff-8d3c0408dd826f27e9f13cd15f09f4d7e89202862f0b13ce131a2e3072c2a40dR49-R53