fix: race on warned flag in versioncheck pkg, make consistent use of client/server terms

This commit is contained in:
Pasha Sviderski
2026-04-08 20:42:27 +10:00
parent 883f184e37
commit ab644f6a47
8 changed files with 122 additions and 138 deletions
@@ -1,12 +1,14 @@
package versioncheck
package grpcversion
import (
"context"
"fmt"
"io"
"os"
"sync/atomic"
"github.com/Masterminds/semver"
internalVersion "github.com/psviderski/uncloud/internal/version"
"github.com/psviderski/uncloud/internal/version"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
@@ -14,39 +16,43 @@ import (
)
const (
MetadataKeyCLIVersion = "uncloud-client-version"
MetadataKeyMinDaemonVersion = "uncloud-min-server-version"
MetadataKeyDaemonVersion = "uncloud-server-version"
MetadataKeyClientVersion = "uncloud-client-version"
MetadataKeyMinServerVersion = "uncloud-min-server-version"
MetadataKeyServerVersion = "uncloud-server-version"
// MinCLIVersion is the minimum client version the daemon accepts. The daemon
// MinClientVersion is the minimum client version the daemon accepts. The daemon
// rejects requests from older clients, forcing them to upgrade. This provides
// a clean cut-off for dropping support for old clients.
//
// MinDaemonVersion is the minimum daemon version the client requires. The client
// MinServerVersion is the minimum daemon version the client requires. The client
// sends this with each request so the daemon can immediately reject if it's too old,
// avoiding the need for a preflight request. This is useful when a new client feature
// requires daemon capabilities that didn't exist in older versions.
//
// The two minimums are independent: a client might require a newer daemon for new
// features, while that same daemon could still handle requests from older clients.
MinCLIVersion = "0.0.0"
MinDaemonVersion = "0.0.0"
MinClientVersion = "0.0.0"
MinServerVersion = "0.0.0"
ReleaseURL = "https://github.com/psviderski/uncloud/releases/latest"
)
var (
// currentVersion is the version of this binary (CLI or daemon)
currentVersion = semver.MustParse(internalVersion.String())
// zeroVersion is used when no version is specified (treated as 0.0.0)
// currentVersion is the version of this binary (CLI or daemon).
currentVersion = semver.MustParse(version.String())
// zeroVersion is used when no version is specified (treated as 0.0.0).
zeroVersion = semver.MustParse("0.0.0")
// Pre-parsed minimum versions for comparison
minCLIVersion = semver.MustParse(MinCLIVersion)
minDaemonVersion = semver.MustParse(MinDaemonVersion)
// Pre-parsed minimum versions for comparison.
minClientVersion = semver.MustParse(MinClientVersion)
minServerVersion = semver.MustParse(MinServerVersion)
// warned tracks if we've already printed the daemon version warning
// TODO: remove when checkDaemonVersionInResponse is no longer needed (see below)
warned bool
// warned tracks if we've already printed the daemon version warning.
// TODO: Remove when checkServerVersionInResponse is no longer needed (see below).
warned atomic.Bool
// WarnWriter is the writer used for version mismatch warnings. Defaults to os.Stderr.
// Tests can override this to capture warning output.
WarnWriter io.Writer = os.Stderr
)
func extractVersion(md metadata.MD, key string) *semver.Version {
@@ -67,18 +73,18 @@ func extractVersion(md metadata.MD, key string) *semver.Version {
func checkClientVersionHeaders(ctx context.Context) error {
md, _ := metadata.FromIncomingContext(ctx)
actualCLIVersion := extractVersion(md, MetadataKeyCLIVersion)
if actualCLIVersion.LessThan(minCLIVersion) {
actualClientVersion := extractVersion(md, MetadataKeyClientVersion)
if actualClientVersion.LessThan(minClientVersion) {
return status.Errorf(codes.FailedPrecondition,
"version check failed: client version is below minimum %s. Please upgrade: %s",
minCLIVersion, ReleaseURL)
minClientVersion, ReleaseURL)
}
requiredMinDaemon := extractVersion(md, MetadataKeyMinDaemonVersion)
if currentVersion.LessThan(requiredMinDaemon) {
requiredMinServer := extractVersion(md, MetadataKeyMinServerVersion)
if currentVersion.LessThan(requiredMinServer) {
return status.Errorf(codes.FailedPrecondition,
"version check failed: daemon version %s is below client's minimum required version %s. Please upgrade the daemon: %s",
currentVersion, requiredMinDaemon, ReleaseURL)
currentVersion, requiredMinServer, ReleaseURL)
}
return nil
@@ -88,7 +94,7 @@ func ServerUnaryInterceptor(ctx context.Context, req any, info *grpc.UnaryServer
if err := checkClientVersionHeaders(ctx); err != nil {
return nil, err
}
if err := grpc.SetHeader(ctx, metadata.Pairs(MetadataKeyDaemonVersion, currentVersion.String())); err != nil {
if err := grpc.SetHeader(ctx, metadata.Pairs(MetadataKeyServerVersion, currentVersion.String())); err != nil {
return nil, err
}
return handler(ctx, req)
@@ -98,7 +104,7 @@ func ServerStreamInterceptor(srv any, ss grpc.ServerStream, info *grpc.StreamSer
if err := checkClientVersionHeaders(ss.Context()); err != nil {
return err
}
if err := ss.SetHeader(metadata.Pairs(MetadataKeyDaemonVersion, currentVersion.String())); err != nil {
if err := ss.SetHeader(metadata.Pairs(MetadataKeyServerVersion, currentVersion.String())); err != nil {
return err
}
return handler(srv, ss)
@@ -106,11 +112,11 @@ func ServerStreamInterceptor(srv any, ss grpc.ServerStream, info *grpc.StreamSer
func ClientUnaryInterceptor(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
ctx = metadata.AppendToOutgoingContext(ctx,
MetadataKeyCLIVersion, currentVersion.String(),
MetadataKeyMinDaemonVersion, MinDaemonVersion,
MetadataKeyClientVersion, currentVersion.String(),
MetadataKeyMinServerVersion, MinServerVersion,
)
// TODO: remove when checkDaemonVersionInResponse is no longer needed,
// TODO: Remove when checkServerVersionInResponse is no longer needed,
// as we'll no longer need to extract headers from the response here.
var respMD metadata.MD
opts = append(opts, grpc.Header(&respMD))
@@ -120,34 +126,33 @@ func ClientUnaryInterceptor(ctx context.Context, method string, req, reply any,
return err
}
// TODO: Remove eventually (see note on method below)
checkDaemonVersionInResponse(respMD)
// TODO: Remove eventually (see note on method below).
checkServerVersionInResponse(respMD)
return nil
}
// This is just needed as a warning during the transition to version checking
// releases. It warns the user when they just communicated with a daemon that did
// not check the version requirements.
// checkServerVersionInResponse warns the user when they communicated with a daemon that
// did not check the version requirements. This is only needed during the transition to
// version-checking releases.
// TODO: Remove this in some later release, after users have upgraded.
func checkDaemonVersionInResponse(md metadata.MD) {
daemonVersion := extractVersion(md, MetadataKeyDaemonVersion)
if daemonVersion.LessThan(minDaemonVersion) {
if warned {
func checkServerVersionInResponse(md metadata.MD) {
serverVersion := extractVersion(md, MetadataKeyServerVersion)
if serverVersion.LessThan(minServerVersion) {
if warned.Swap(true) {
return
}
warned = true
msg := fmt.Sprintf("daemon version is below minimum required version %s. The daemon did not verify this CLI's minimum version requirement, so the operation may not have behaved as intended. Please upgrade the daemon: %s",
minDaemonVersion, ReleaseURL)
fmt.Fprintf(os.Stderr, "WARNING: %s\n", msg)
minServerVersion, ReleaseURL)
fmt.Fprintf(WarnWriter, "WARNING: %s\n", msg)
}
}
func ClientStreamInterceptor(ctx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, method string, streamer grpc.Streamer, opts ...grpc.CallOption) (grpc.ClientStream, error) {
ctx = metadata.AppendToOutgoingContext(ctx,
MetadataKeyCLIVersion, currentVersion.String(),
MetadataKeyMinDaemonVersion, MinDaemonVersion,
MetadataKeyClientVersion, currentVersion.String(),
MetadataKeyMinServerVersion, MinServerVersion,
)
stream, err := streamer(ctx, desc, cc, method, opts...)
@@ -157,23 +162,23 @@ func ClientStreamInterceptor(ctx context.Context, desc *grpc.StreamDesc, cc *grp
// TODO: Wrapping the stream in versionedClientStream will no longer
// be necessary when we are ready to remove the temporary, transition
// safety check checkDaemonVersionInResponse (see note on method above)
// safety check checkServerVersionInResponse (see note on method above).
return &versionedClientStream{ClientStream: stream}, nil
}
// TODO: remove when checkDaemonVersionInResponse is no longer needed
// TODO: Remove when checkServerVersionInResponse is no longer needed.
type versionedClientStream struct {
grpc.ClientStream
}
// TODO: remove when checkDaemonVersionInResponse is no longer needed
// TODO: Remove when checkServerVersionInResponse is no longer needed.
func (s *versionedClientStream) Header() (metadata.MD, error) {
md, err := s.ClientStream.Header()
if err != nil {
return nil, err
}
checkDaemonVersionInResponse(md)
checkServerVersionInResponse(md)
return md, nil
}
@@ -1,11 +1,8 @@
package versioncheck
package grpcversion
import (
"bytes"
"context"
"io"
"os"
"strings"
"testing"
"github.com/stretchr/testify/assert"
@@ -25,37 +22,37 @@ func TestExtractVersion(t *testing.T) {
{
name: "nil metadata",
md: nil,
key: MetadataKeyCLIVersion,
key: MetadataKeyClientVersion,
expected: "0.0.0",
},
{
name: "missing key",
md: metadata.MD{},
key: MetadataKeyCLIVersion,
key: MetadataKeyClientVersion,
expected: "0.0.0",
},
{
name: "empty value",
md: metadata.Pairs(MetadataKeyCLIVersion, ""),
key: MetadataKeyCLIVersion,
md: metadata.Pairs(MetadataKeyClientVersion, ""),
key: MetadataKeyClientVersion,
expected: "0.0.0",
},
{
name: "invalid version",
md: metadata.Pairs(MetadataKeyCLIVersion, "not-a-version"),
key: MetadataKeyCLIVersion,
md: metadata.Pairs(MetadataKeyClientVersion, "not-a-version"),
key: MetadataKeyClientVersion,
expected: "0.0.0",
},
{
name: "valid version",
md: metadata.Pairs(MetadataKeyCLIVersion, "1.2.3"),
key: MetadataKeyCLIVersion,
md: metadata.Pairs(MetadataKeyClientVersion, "1.2.3"),
key: MetadataKeyClientVersion,
expected: "1.2.3",
},
{
name: "version with prerelease",
md: metadata.Pairs(MetadataKeyCLIVersion, "0.0.0-dev"),
key: MetadataKeyCLIVersion,
md: metadata.Pairs(MetadataKeyClientVersion, "0.0.0-dev"),
key: MetadataKeyClientVersion,
expected: "0.0.0-dev",
},
}
@@ -79,7 +76,7 @@ func TestCheckClientVersionHeaders(t *testing.T) {
{
name: "cli version below minimum",
md: metadata.Pairs(
MetadataKeyCLIVersion, "0.0.0-dev",
MetadataKeyClientVersion, "0.0.0-dev",
),
wantErr: true,
errCode: codes.FailedPrecondition,
@@ -88,15 +85,15 @@ func TestCheckClientVersionHeaders(t *testing.T) {
{
name: "cli version above minimum",
md: metadata.Pairs(
MetadataKeyCLIVersion, "999.0.0",
MetadataKeyClientVersion, "999.0.0",
),
wantErr: false,
},
{
name: "min daemon version above current daemon",
md: metadata.Pairs(
MetadataKeyCLIVersion, "999.0.0",
MetadataKeyMinDaemonVersion, "999.0.0",
MetadataKeyClientVersion, "999.0.0",
MetadataKeyMinServerVersion, "999.0.0",
),
wantErr: true,
errCode: codes.FailedPrecondition,
@@ -105,8 +102,8 @@ func TestCheckClientVersionHeaders(t *testing.T) {
{
name: "min daemon version below current daemon",
md: metadata.Pairs(
MetadataKeyCLIVersion, "999.0.0",
MetadataKeyMinDaemonVersion, "0.0.1",
MetadataKeyClientVersion, "999.0.0",
MetadataKeyMinServerVersion, "0.0.1",
),
wantErr: false,
},
@@ -126,8 +123,7 @@ func TestCheckClientVersionHeaders(t *testing.T) {
st, ok := status.FromError(err)
require.True(t, ok, "expected gRPC status error, got %T", err)
assert.Equal(t, tt.errCode, st.Code())
assert.True(t, strings.Contains(st.Message(), tt.errContain),
"error message = %q, want to contain %q", st.Message(), tt.errContain)
assert.Contains(t, st.Message(), tt.errContain)
} else {
assert.NoError(t, err)
}
@@ -135,7 +131,17 @@ func TestCheckClientVersionHeaders(t *testing.T) {
}
}
func TestCheckDaemonVersionInResponse(t *testing.T) {
func captureWarnings(t *testing.T, fn func()) string {
t.Helper()
var buf bytes.Buffer
old := WarnWriter
WarnWriter = &buf
t.Cleanup(func() { WarnWriter = old })
fn()
return buf.String()
}
func TestCheckServerVersionInResponse(t *testing.T) {
tests := []struct {
name string
md metadata.MD
@@ -143,74 +149,47 @@ func TestCheckDaemonVersionInResponse(t *testing.T) {
}{
{
name: "daemon version below minimum",
md: metadata.Pairs(MetadataKeyDaemonVersion, "0.0.0-dev"),
md: metadata.Pairs(MetadataKeyServerVersion, "0.0.0-dev"),
wantWarning: true,
},
{
name: "daemon version above minimum",
md: metadata.Pairs(MetadataKeyDaemonVersion, "999.0.0"),
md: metadata.Pairs(MetadataKeyServerVersion, "999.0.0"),
wantWarning: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Reset warned flag for each test
warned = false
warned.Store(false)
// Capture stderr
old := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w
checkDaemonVersionInResponse(tt.md)
w.Close()
var buf bytes.Buffer
io.Copy(&buf, r)
os.Stderr = old
output := buf.String()
output := captureWarnings(t, func() {
checkServerVersionInResponse(tt.md)
})
if tt.wantWarning {
assert.True(t, strings.Contains(output, "WARNING"), "expected warning output, got none")
assert.Contains(t, output, "WARNING")
} else {
assert.Equal(t, "", output)
assert.Empty(t, output)
}
})
}
}
func TestCheckDaemonVersionInResponse_WarnOnce(t *testing.T) {
// Reset warned flag
warned = false
func TestCheckServerVersionInResponse_WarnOnce(t *testing.T) {
warned.Store(false)
md := metadata.Pairs(MetadataKeyDaemonVersion, "0.0.0-dev")
md := metadata.Pairs(MetadataKeyServerVersion, "0.0.0-dev")
// First call - should warn
old := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w
// First call should warn.
output1 := captureWarnings(t, func() {
checkServerVersionInResponse(md)
})
assert.Contains(t, output1, "WARNING", "first call should warn")
checkDaemonVersionInResponse(md)
w.Close()
var buf bytes.Buffer
io.Copy(&buf, r)
os.Stderr = old
assert.True(t, strings.Contains(buf.String(), "WARNING"), "first call should warn")
// Second call - should NOT warn (warned flag is now true)
r2, w2, _ := os.Pipe()
os.Stderr = w2
checkDaemonVersionInResponse(md)
w2.Close()
var buf2 bytes.Buffer
io.Copy(&buf2, r2)
os.Stderr = old
assert.Equal(t, "", buf2.String(), "second call should not warn")
// Second call should not warn (warned flag is now true).
output2 := captureWarnings(t, func() {
checkServerVersionInResponse(md)
})
assert.Empty(t, output2, "second call should not warn")
}
+5 -5
View File
@@ -22,6 +22,7 @@ import (
"github.com/psviderski/uncloud/internal/corrosion"
"github.com/psviderski/uncloud/internal/docker"
"github.com/psviderski/uncloud/internal/fs"
"github.com/psviderski/uncloud/internal/grpcversion"
"github.com/psviderski/uncloud/internal/journal"
"github.com/psviderski/uncloud/internal/machine/api/pb"
apiproxy "github.com/psviderski/uncloud/internal/machine/api/proxy"
@@ -34,7 +35,6 @@ import (
"github.com/psviderski/uncloud/internal/machine/network"
"github.com/psviderski/uncloud/internal/machine/store"
"github.com/psviderski/uncloud/pkg/api"
versionpkg "github.com/psviderski/uncloud/pkg/versioncheck"
"github.com/psviderski/unregistry"
"github.com/siderolabs/grpc-proxy/proxy"
"golang.org/x/sync/errgroup"
@@ -273,8 +273,8 @@ func NewMachine(config *Config) (*Machine, error) {
proxyDirector := apiproxy.NewDirector(config.MachineSockPath, constants.MachineAPIPort)
localProxyServer := grpc.NewServer(
grpc.ForceServerCodecV2(proxy.Codec()),
grpc.UnaryInterceptor(versionpkg.ServerUnaryInterceptor),
grpc.StreamInterceptor(versionpkg.ServerStreamInterceptor),
grpc.UnaryInterceptor(grpcversion.ServerUnaryInterceptor),
grpc.StreamInterceptor(grpcversion.ServerStreamInterceptor),
grpc.UnknownServiceHandler(
proxy.TransparentHandler(proxyDirector.Director),
),
@@ -428,8 +428,8 @@ func (m *Machine) Run(ctx context.Context) error {
m.proxyDirector.UpdateLocalAddress(m.state.Network.ManagementIP.String())
proxyServer := grpc.NewServer(
grpc.ForceServerCodecV2(proxy.Codec()),
grpc.UnaryInterceptor(versionpkg.ServerUnaryInterceptor),
grpc.StreamInterceptor(versionpkg.ServerStreamInterceptor),
grpc.UnaryInterceptor(grpcversion.ServerUnaryInterceptor),
grpc.StreamInterceptor(grpcversion.ServerStreamInterceptor),
grpc.UnknownServiceHandler(
proxy.TransparentHandler(m.proxyDirector.Director),
),
+3 -3
View File
@@ -7,9 +7,9 @@ import (
"net"
"strings"
"github.com/psviderski/uncloud/internal/grpcversion"
"github.com/psviderski/uncloud/internal/machine"
"github.com/psviderski/uncloud/internal/sshexec"
"github.com/psviderski/uncloud/pkg/versioncheck"
"golang.org/x/crypto/ssh"
"golang.org/x/net/proxy"
"google.golang.org/grpc"
@@ -75,8 +75,8 @@ func (c *SSHConnector) Connect(ctx context.Context) (*grpc.ClientConn, error) {
"unix://"+sockPath,
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithDefaultServiceConfig(defaultServiceConfig),
grpc.WithUnaryInterceptor(versioncheck.ClientUnaryInterceptor),
grpc.WithStreamInterceptor(versioncheck.ClientStreamInterceptor),
grpc.WithUnaryInterceptor(grpcversion.ClientUnaryInterceptor),
grpc.WithStreamInterceptor(grpcversion.ClientStreamInterceptor),
grpc.WithContextDialer(
func(ctx context.Context, addr string) (net.Conn, error) {
addr = strings.TrimPrefix(addr, "unix://")
+3 -3
View File
@@ -11,7 +11,7 @@ import (
"strings"
"github.com/docker/cli/cli/connhelper/commandconn"
"github.com/psviderski/uncloud/pkg/versioncheck"
"github.com/psviderski/uncloud/internal/grpcversion"
"golang.org/x/net/proxy"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
@@ -78,8 +78,8 @@ func (c *SSHCLIConnector) Connect(ctx context.Context) (*grpc.ClientConn, error)
"passthrough:///", // Dummy target since we're using a custom dialer.
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithDefaultServiceConfig(defaultServiceConfig),
grpc.WithUnaryInterceptor(versioncheck.ClientUnaryInterceptor),
grpc.WithStreamInterceptor(versioncheck.ClientStreamInterceptor),
grpc.WithUnaryInterceptor(grpcversion.ClientUnaryInterceptor),
grpc.WithStreamInterceptor(grpcversion.ClientStreamInterceptor),
grpc.WithContextDialer(func(ctx context.Context, _ string) (net.Conn, error) {
dialArgs := append(c.buildSSHArgs(), "uncloudd", "dial-stdio")
if c.config.SockPath != "" {
+3 -3
View File
@@ -5,7 +5,7 @@ import (
"fmt"
"net/netip"
"github.com/psviderski/uncloud/pkg/versioncheck"
"github.com/psviderski/uncloud/internal/grpcversion"
"golang.org/x/net/proxy"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
@@ -25,8 +25,8 @@ func (c *TCPConnector) Connect(_ context.Context) (*grpc.ClientConn, error) {
c.apiAddr.String(),
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithDefaultServiceConfig(defaultServiceConfig),
grpc.WithUnaryInterceptor(versioncheck.ClientUnaryInterceptor),
grpc.WithStreamInterceptor(versioncheck.ClientStreamInterceptor),
grpc.WithUnaryInterceptor(grpcversion.ClientUnaryInterceptor),
grpc.WithStreamInterceptor(grpcversion.ClientStreamInterceptor),
)
if err != nil {
return nil, fmt.Errorf("create machine API client: %w", err)
+3 -3
View File
@@ -4,7 +4,7 @@ import (
"context"
"fmt"
"github.com/psviderski/uncloud/pkg/versioncheck"
"github.com/psviderski/uncloud/internal/grpcversion"
"golang.org/x/net/proxy"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
@@ -27,8 +27,8 @@ func (c *UnixConnector) Connect(_ context.Context) (*grpc.ClientConn, error) {
target,
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithDefaultServiceConfig(defaultServiceConfig),
grpc.WithUnaryInterceptor(versioncheck.ClientUnaryInterceptor),
grpc.WithStreamInterceptor(versioncheck.ClientStreamInterceptor),
grpc.WithUnaryInterceptor(grpcversion.ClientUnaryInterceptor),
grpc.WithStreamInterceptor(grpcversion.ClientStreamInterceptor),
)
if err != nil {
return nil, fmt.Errorf("create machine API client: %w", err)
+3 -3
View File
@@ -8,11 +8,11 @@ import (
"strconv"
"github.com/psviderski/uncloud/internal/cli/config"
"github.com/psviderski/uncloud/internal/grpcversion"
"github.com/psviderski/uncloud/internal/machine/constants"
"github.com/psviderski/uncloud/internal/machine/network"
"github.com/psviderski/uncloud/internal/machine/network/tunnel"
"github.com/psviderski/uncloud/pkg/client"
"github.com/psviderski/uncloud/pkg/versioncheck"
"golang.org/x/net/proxy"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
@@ -71,8 +71,8 @@ func (c *WireGuardConnector) Connect(ctx context.Context) (*grpc.ClientConn, err
grpc.WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) {
return c.tun.DialContext(ctx, "tcp", addr)
}),
grpc.WithUnaryInterceptor(versioncheck.ClientUnaryInterceptor),
grpc.WithStreamInterceptor(versioncheck.ClientStreamInterceptor),
grpc.WithUnaryInterceptor(grpcversion.ClientUnaryInterceptor),
grpc.WithStreamInterceptor(grpcversion.ClientStreamInterceptor),
)
if err != nil {
return nil, fmt.Errorf("connect to machine API through WireGuard tunnel: %w", err)