Proof-of-concept draft for teleport-update binary implementation #46357
Conversation
| endpoint := fmt.Sprintf("https://%s/webapi/find", cfg.ProxyAddr) | ||
|
|
||
| if cfg.Group != "" { | ||
| endpoint = fmt.Sprintf("%s?group=%s", endpoint, cfg.Group) | ||
| } | ||
|
|
There was a problem hiding this comment.
The group content is not urlencoded and the query building is fragile. The url lib allows to do this safely.
There was a problem hiding this comment.
Sorry, totally agree, was just using this to test
| func (tv *TeleportVersion) Remove(version string) error { | ||
| versionPath := filepath.Join(tv.VersionsDir, version) | ||
| sumPath := filepath.Join(versionPath, checksumType) | ||
|
|
||
| // invalidate checksum first, to protect against partially-removed | ||
| // directory with valid checksum. | ||
| if err := os.Remove(sumPath); err != nil { | ||
| return trace.Wrap(err) | ||
| } | ||
| if err := os.RemoveAll(versionPath); err != nil { | ||
| return trace.Wrap(err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This function should not allow to delete the current active version.
| sum, err := hex.DecodeString(raw) | ||
| if err != nil { | ||
| plog.WarnContext(ctx, "corrupt checksum detected", "size", n, "checksum", raw) | ||
| return nil, nil |
There was a problem hiding this comment.
Returning nil while we failed to read the checksum is not consistent with the other error return paths of the function: e.g. we can't open the file. If the goal is to continue the execution, it would be clearer to return an error and handle it.
There was a problem hiding this comment.
Agree, will add a typed error in the separate PR for this function.
| } | ||
| raw := buf.String() | ||
| if n != 64 { | ||
| plog.WarnContext(ctx, "unexpected checksum size", "size", n, "checksum", raw) |
There was a problem hiding this comment.
An unexpected size should cause an immediate failure, there is no need to try to decode a buffer that has the wrong length.
There was a problem hiding this comment.
Will cover this with a typed error (same as below)
There was a problem hiding this comment.
nit: might be replaced with io.CopyN which actually does the same, and returns EOF if n is less than expected
| return nil, nil, trace.Errorf("size of download (%d bytes) exceeds available disk space (%d bytes)", resp.ContentLength, free) | ||
| } | ||
|
|
||
| // Calculate checksum concurrently with download. |
There was a problem hiding this comment.
What's the motivation behind checksuming as we download versus reading the file back from the fs?
There was a problem hiding this comment.
No downside to it, and it avoids needed to re-read the file from disk
Co-authored-by: Hugo Shaka <[email protected]>
Co-authored-by: Hugo Shaka <[email protected]>
Co-authored-by: Hugo Shaka <[email protected]>
sclevine
added
the
no-changelog
sclevine
changed the title
|
Completed by #47565 |
This PR contains an initial draft of the
teleport-updatebinary described in #40190.Not all functionality is implemented, only:
/var/lib/teleport/versions/X.Y.Z, with disk space checks, checksum verification, and protection against power loss scenarios.updates.yamlThis PR will not be merged as-is, and is only open for early review of the approach.
I plan to break this download into separate, tested PRs if this initial version looks good.