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.

Sign up for GitHub

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

Jump to bottom

Expose client tools auto update for find endpoint #46785

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Expose client tools auto update for find endpoint #46785

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61b2b93
Expose client tools auto update for find endpoint
vapopov Sep 19, 2024
31b8316
Group auto update settings in find response
vapopov Sep 20, 2024
e3c3bba
Add more test cases
vapopov Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/client/webclient/webclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,10 @@ type PingResponse struct {
ServerVersion string `json:"server_version"`
// MinClientVersion is the minimum client version required by the server.
MinClientVersion string `json:"min_client_version"`
// 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"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the auto update information all be contained within another struct instead of adding each individual field to the response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

grouping fields to another struct would be better, will change this one

// ClusterName contains the name of the Teleport cluster.
ClusterName string `json:"cluster_name"`

Expand Down
20 changes: 18 additions & 2 deletions lib/web/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1516,7 +1516,7 @@ func (h *Handler) find(w http.ResponseWriter, r *http.Request, p httprouter.Para
if err != nil {
return nil, trace.Wrap(err)
}
return webclient.PingResponse{
response := webclient.PingResponse{
Auth: webclient.AuthenticationSettings{
// Nodes need the signature algorithm suite when joining to generate
// keys with the correct algorithm.
Expand All @@ -1526,7 +1526,23 @@ func (h *Handler) find(w http.ResponseWriter, r *http.Request, p httprouter.Para
ServerVersion: teleport.Version,
MinClientVersion: teleport.MinClientVersion,
ClusterName: h.auth.clusterName,
}, nil
}

autoUpdateConfig, err := h.GetAccessPoint().GetAutoUpdateConfig(r.Context())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the auth preference above is retrieved using h.cfg.AccessPoint, let's pick one and be consistent

if err != nil && !trace.IsNotFound(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should failing to retrieve auto update information prevent the response entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added logging instead, but we might generate a lot of log messages with this error message in frequent requests to this endpoint

} else if err == nil {
response.ToolsVersion = autoUpdateVersion.GetSpec().GetToolsVersion()
}

return response, nil
}

func (h *Handler) pingWithConnector(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) {
Expand Down
Loading