From ff51c6c44fe3d87cfd08e624c4613a8b9201cb53 Mon Sep 17 00:00:00 2001 From: Eric Weber Date: Mon, 24 Jul 2023 16:39:59 -0500 Subject: [PATCH] Enable identity validation for deprecated EngineBinary flow Signed-off-by: Eric Weber --- controller/engine_controller.go | 9 ++--- engineapi/backup_monitor.go | 2 +- engineapi/backups.go | 18 ++++++++-- engineapi/engine.go | 64 ++++++++++++++++++++++++--------- engineapi/engine_binary.go | 9 ++--- engineapi/enginesim.go | 3 +- engineapi/proxy_backup.go | 5 +-- engineapi/snapshot.go | 11 +++++- engineapi/types.go | 11 +++--- 9 files changed, 94 insertions(+), 38 deletions(-) diff --git a/controller/engine_controller.go b/controller/engine_controller.go index 895000b758..9146f592bb 100644 --- a/controller/engine_controller.go +++ b/controller/engine_controller.go @@ -1610,10 +1610,11 @@ func GetBinaryClientForEngine(e *longhorn.Engine, engines engineapi.EngineClient } client, err = engines.NewEngineClient(&engineapi.EngineClientRequest{ - VolumeName: e.Spec.VolumeName, - EngineImage: image, - IP: e.Status.IP, - Port: e.Status.Port, + VolumeName: e.Spec.VolumeName, + EngineImage: image, + IP: e.Status.IP, + Port: e.Status.Port, + InstanceName: e.Name, }) if err != nil { return nil, err diff --git a/engineapi/backup_monitor.go b/engineapi/backup_monitor.go index d576908603..74b97b9ce1 100644 --- a/engineapi/backup_monitor.go +++ b/engineapi/backup_monitor.go @@ -294,7 +294,7 @@ func (m *BackupMonitor) syncBackupStatusFromEngineReplica() (currentBackupStatus m.backupStatus.DeepCopyInto(¤tBackupStatus) m.backupStatusLock.RUnlock() - engineBackupStatus, err = m.engineClientProxy.SnapshotBackupStatus(m.engine, m.backupName, m.replicaAddress) + engineBackupStatus, err = m.engineClientProxy.SnapshotBackupStatus(m.engine, m.backupName, m.replicaAddress, "") if err != nil { return currentBackupStatus, err } diff --git a/engineapi/backups.go b/engineapi/backups.go index 07fba655d2..30b8bcafa2 100644 --- a/engineapi/backups.go +++ b/engineapi/backups.go @@ -318,10 +318,10 @@ func (e *EngineBinary) SnapshotBackup(engine *longhorn.Engine, snapName, backupN // TODO: update when replacing this function snap, err := e.SnapshotGet(nil, snapName) if err != nil { - return "", "", errors.Wrapf(err, "error getting snapshot '%s', volume '%s'", snapName, e.name) + return "", "", errors.Wrapf(err, "error getting snapshot '%s', volume '%s'", snapName, e.volumeName) } if snap == nil { - return "", "", errors.Errorf("could not find snapshot '%s' to backup, volume '%s'", snapName, e.name) + return "", "", errors.Errorf("could not find snapshot '%s' to backup, volume '%s'", snapName, e.volumeName) } version, err := e.VersionGet(nil, true) if err != nil { @@ -365,12 +365,24 @@ func (e *EngineBinary) SnapshotBackup(engine *longhorn.Engine, snapName, backupN // SnapshotBackupStatus calls engine binary // TODO: Deprecated, replaced by gRPC proxy -func (e *EngineBinary) SnapshotBackupStatus(engine *longhorn.Engine, backupName, replicaAddress string) (*longhorn.EngineBackupStatus, error) { +func (e *EngineBinary) SnapshotBackupStatus(engine *longhorn.Engine, backupName, replicaAddress, + replicaName string) (*longhorn.EngineBackupStatus, error) { args := []string{"backup", "status", backupName} if replicaAddress != "" { args = append(args, "--replica", replicaAddress) } + // For now, we likely don't know the replica name here. Don't bother checking the binary version if we don't. + if replicaName != "" { + version, err := e.VersionGet(engine, true) + if err != nil { + return nil, err + } + if version.ClientVersion.CLIAPIVersion >= 9 { + args = append(args, "--replica-instance-name", replicaName) + } + } + output, err := e.ExecuteEngineBinary(args...) if err != nil { return nil, err diff --git a/engineapi/engine.go b/engineapi/engine.go index 327b27ca91..8dbd12eb4a 100644 --- a/engineapi/engine.go +++ b/engineapi/engine.go @@ -29,11 +29,12 @@ const ( type EngineCollection struct{} type EngineBinary struct { - name string - image string - ip string - port int - cURL string + volumeName string + image string + ip string + port int + cURL string + instanceName string } func (c *EngineCollection) NewEngineClient(request *EngineClientRequest) (*EngineBinary, error) { @@ -46,16 +47,17 @@ func (c *EngineCollection) NewEngineClient(request *EngineClientRequest) (*Engin } return &EngineBinary{ - name: request.VolumeName, - image: request.EngineImage, - ip: request.IP, - port: request.Port, - cURL: imutil.GetURL(request.IP, request.Port), + volumeName: request.VolumeName, + image: request.EngineImage, + ip: request.IP, + port: request.Port, + cURL: imutil.GetURL(request.IP, request.Port), + instanceName: request.InstanceName, }, nil } func (e *EngineBinary) Name() string { - return e.name + return e.volumeName } func (e *EngineBinary) LonghornEngineBinary() string { @@ -63,17 +65,26 @@ func (e *EngineBinary) LonghornEngineBinary() string { } func (e *EngineBinary) ExecuteEngineBinary(args ...string) (string, error) { - args = append([]string{"--url", e.cURL}, args...) + args, err := e.addFlags(args) + if err != nil { + return "", err + } return util.Execute([]string{}, e.LonghornEngineBinary(), args...) } func (e *EngineBinary) ExecuteEngineBinaryWithTimeout(timeout time.Duration, args ...string) (string, error) { - args = append([]string{"--url", e.cURL}, args...) + args, err := e.addFlags(args) + if err != nil { + return "", err + } return util.ExecuteWithTimeout(timeout, []string{}, e.LonghornEngineBinary(), args...) } func (e *EngineBinary) ExecuteEngineBinaryWithoutTimeout(envs []string, args ...string) (string, error) { - args = append([]string{"--url", e.cURL}, args...) + args, err := e.addFlags(args) + if err != nil { + return "", err + } return util.ExecuteWithoutTimeout(envs, e.LonghornEngineBinary(), args...) } @@ -98,7 +109,7 @@ func parseReplica(s string) (*Replica, error) { func (e *EngineBinary) ReplicaList(*longhorn.Engine) (map[string]*Replica, error) { output, err := e.ExecuteEngineBinary("ls") if err != nil { - return nil, errors.Wrapf(err, "failed to list replicas from controller '%s'", e.name) + return nil, errors.Wrapf(err, "failed to list replicas from controller '%s'", e.volumeName) } replicas := make(map[string]*Replica) lines := strings.Split(output, "\n") @@ -148,8 +159,12 @@ func (e *EngineBinary) ReplicaAdd(engine *longhorn.Engine, replicaName, url stri } } + if version.ClientVersion.CLIAPIVersion >= 9 { + cmd = append(cmd, "--replica-instance-name", replicaName) + } + if _, err := e.ExecuteEngineBinaryWithoutTimeout([]string{}, cmd...); err != nil { - return errors.Wrapf(err, "failed to add replica address='%s' to controller '%s'", url, e.name) + return errors.Wrapf(err, "failed to add replica address='%s' to controller '%s'", url, e.volumeName) } return nil } @@ -161,7 +176,7 @@ func (e *EngineBinary) ReplicaRemove(engine *longhorn.Engine, url string) error return err } if _, err := e.ExecuteEngineBinary("rm", url); err != nil { - return errors.Wrapf(err, "failed to rm replica address='%s' from controller '%s'", url, e.name) + return errors.Wrapf(err, "failed to rm replica address='%s' from controller '%s'", url, e.volumeName) } return nil } @@ -304,3 +319,18 @@ func (e *EngineBinary) ReplicaModeUpdate(engine *longhorn.Engine, url, mode stri func (e *EngineBinary) MetricsGet(*longhorn.Engine) (*Metrics, error) { return nil, fmt.Errorf(ErrNotImplement) } + +// addFlags always adds required flags to args. In addition, if the engine version is high enough, it adds additional +// engine identity validation flags. +func (e *EngineBinary) addFlags(args []string) ([]string, error) { + version, err := e.VersionGet(nil, true) + if err != nil { + return args, errors.Wrap(err, "failed to get engine CLI version while adding identity flags") + } + + argsToAdd := []string{"--url", e.cURL} + if version.ClientVersion.CLIAPIVersion >= 9 { + argsToAdd = append(argsToAdd, "--volume-name", e.volumeName, "--engine-instance-name", e.instanceName) + } + return append(argsToAdd, args...), nil +} diff --git a/engineapi/engine_binary.go b/engineapi/engine_binary.go index 7ba3dff56f..be242a183b 100644 --- a/engineapi/engine_binary.go +++ b/engineapi/engine_binary.go @@ -43,9 +43,10 @@ func GetEngineBinaryClient(ds *datastore.DataStore, volumeName, nodeID string) ( engineCollection := &EngineCollection{} return engineCollection.NewEngineClient(&EngineClientRequest{ - VolumeName: e.Spec.VolumeName, - EngineImage: e.Status.CurrentImage, - IP: e.Status.IP, - Port: e.Status.Port, + VolumeName: e.Spec.VolumeName, + EngineImage: e.Status.CurrentImage, + IP: e.Status.IP, + Port: e.Status.Port, + InstanceName: e.Name, }) } diff --git a/engineapi/enginesim.go b/engineapi/enginesim.go index c9e63162a8..c108d009c4 100644 --- a/engineapi/enginesim.go +++ b/engineapi/enginesim.go @@ -196,7 +196,8 @@ func (e *EngineSimulator) SnapshotBackup(engine *longhorn.Engine, backupName, sn return "", "", fmt.Errorf(ErrNotImplement) } -func (e *EngineSimulator) SnapshotBackupStatus(engine *longhorn.Engine, backupName, replicaAddress string) (*longhorn.EngineBackupStatus, error) { +func (e *EngineSimulator) SnapshotBackupStatus(engine *longhorn.Engine, backupName, replicaAddress, + replicaName string) (*longhorn.EngineBackupStatus, error) { return nil, fmt.Errorf(ErrNotImplement) } diff --git a/engineapi/proxy_backup.go b/engineapi/proxy_backup.go index b82c6f0b68..a9cfad5915 100644 --- a/engineapi/proxy_backup.go +++ b/engineapi/proxy_backup.go @@ -48,9 +48,10 @@ func (p *Proxy) SnapshotBackup(e *longhorn.Engine, snapshotName, backupName, bac return backupID, replicaAddress, nil } -func (p *Proxy) SnapshotBackupStatus(e *longhorn.Engine, backupName, replicaAddress string) (status *longhorn.EngineBackupStatus, err error) { +func (p *Proxy) SnapshotBackupStatus(e *longhorn.Engine, backupName, replicaAddress, + replicaName string) (status *longhorn.EngineBackupStatus, err error) { recv, err := p.grpcClient.SnapshotBackupStatus(string(e.Spec.BackendStoreDriver), e.Name, e.Spec.VolumeName, - p.DirectToURL(e), backupName, replicaAddress, "") + p.DirectToURL(e), backupName, replicaAddress, replicaName) if err != nil { return nil, err } diff --git a/engineapi/snapshot.go b/engineapi/snapshot.go index b69becdf54..dfe6467d94 100644 --- a/engineapi/snapshot.go +++ b/engineapi/snapshot.go @@ -108,7 +108,16 @@ func (e *EngineBinary) SnapshotPurgeStatus(*longhorn.Engine) (map[string]*longho func (e *EngineBinary) SnapshotClone(engine *longhorn.Engine, snapshotName, fromEngineAddress, fromEngineName string, fileSyncHTTPClientTimeout int64) error { args := []string{"snapshot", "clone", "--snapshot-name", snapshotName, "--from-controller-address", - fromEngineAddress, "--from-controller-instance-name", fromEngineName} + fromEngineAddress} + + version, err := e.VersionGet(engine, true) + if err != nil { + return err + } + if version.ClientVersion.CLIAPIVersion >= 9 { + args = append(args, "--from-controller-instance-name", fromEngineName) + } + if _, err := e.ExecuteEngineBinaryWithoutTimeout([]string{}, args...); err != nil { return errors.Wrapf(err, "error starting snapshot clone") } diff --git a/engineapi/types.go b/engineapi/types.go index 422ced9b1c..df0c6c4cd7 100644 --- a/engineapi/types.go +++ b/engineapi/types.go @@ -94,7 +94,7 @@ type EngineClient interface { SnapshotPurge(engine *longhorn.Engine) error SnapshotPurgeStatus(engine *longhorn.Engine) (map[string]*longhorn.PurgeStatus, error) SnapshotBackup(engine *longhorn.Engine, backupName, snapName, backupTarget, backingImageName, backingImageChecksum, compressionMethod string, concurrentLimit int, storageClassName string, labels, credential map[string]string) (string, string, error) - SnapshotBackupStatus(engine *longhorn.Engine, backupName, replicaAddress string) (*longhorn.EngineBackupStatus, error) + SnapshotBackupStatus(engine *longhorn.Engine, backupName, replicaAddress, replicaName string) (*longhorn.EngineBackupStatus, error) SnapshotCloneStatus(engine *longhorn.Engine) (map[string]*longhorn.SnapshotCloneStatus, error) SnapshotClone(engine *longhorn.Engine, snapshotName, fromEngineAddress, fromEngineName string, fileSyncHTTPClientTimeout int64) error SnapshotHash(engine *longhorn.Engine, snapshotName string, rehash bool) error @@ -107,10 +107,11 @@ type EngineClient interface { } type EngineClientRequest struct { - VolumeName string - EngineImage string - IP string - Port int + VolumeName string + EngineImage string + IP string + Port int + InstanceName string } type EngineClientCollection interface {