Engine identity validation

[ejweber]

longhorn/longhorn#5845

This PR makes use of the changes in longhorn-engine (longhorn/longhorn-engine#902) and longhorn-instance-manager (longhorn/longhorn-instance-manager#229) to ensure identity validation during normal Longhorn operation.

[mergify]

This pull request is now in conflicts. Could you fix it @ejweber? 🙏

[mergify]

This pull request is now in conflicts. Could you fix it @ejweber? 🙏

[mergify]

This pull request is now in conflicts. Could you fix it @ejweber? 🙏

[ejweber]

Core tests all pass except for live engine upgrade tests, which are failing because of longhorn/longhorn-engine#902 (comment).

https://ci.longhorn.io/job/private/job/longhorn-tests-regression/4521/

Upgrade tests are running.

https://ci.longhorn.io/job/private/job/longhorn-tests-regression/4524/

[mergify]

This pull request is now in conflicts. Could you fix it @ejweber? 🙏

[PhanLe1010]

NIT: This PR is not a draft anymore, can we edit the PR desc?

[ejweber]

Realized I missed the engine upgrade case while testing. This prevents us from using engine identity validation for old engines that are live upgraded.

longhorn-manager/engineapi/instance_manager.go

Lines 667 to 719 in ff51c6c

	func (c *InstanceManagerClient) engineInstanceUpgrade(req *EngineInstanceUpgradeRequest) (*longhorn.InstanceProcess, error) {
	if err := CheckInstanceManagerCompatibility(c.apiMinVersion, c.apiVersion); err != nil {
	return nil, err
	}
	frontend, err := GetEngineInstanceFrontend(req.Engine.Spec.BackendStoreDriver, req.VolumeFrontend)
	if err != nil {
	return nil, err
	}
	args := []string{"controller", req.Engine.Spec.VolumeName, "--frontend", frontend, "--upgrade"}
	for _, addr := range req.Engine.Spec.UpgradedReplicaAddressMap {
	args = append(args, "--replica", GetBackendReplicaURL(addr))
	}
	if req.EngineCLIAPIVersion >= 6 {
	args = append(args,
	"--size", strconv.FormatInt(req.Engine.Spec.VolumeSize, 10),
	"--current-size", strconv.FormatInt(req.Engine.Status.CurrentSize, 10))
	}
	if req.EngineCLIAPIVersion >= 7 {
	args = append(args,
	"--engine-replica-timeout", strconv.FormatInt(req.EngineReplicaTimeout, 10),
	"--file-sync-http-client-timeout", strconv.FormatInt(req.ReplicaFileSyncHTTPClientTimeout, 10))
	if req.DataLocality == longhorn.DataLocalityStrictLocal {
	args = append(args,
	"--data-server-protocol", "unix")
	}
	if req.Engine.Spec.UnmapMarkSnapChainRemovedEnabled {
	args = append(args, "--unmap-mark-snap-chain-removed")
	}
	}
	binary := filepath.Join(types.GetEngineBinaryDirectoryForEngineManagerContainer(req.Engine.Spec.EngineImage), types.EngineBinaryName)
	if c.GetAPIVersion() < 4 {
	process, err := c.processManagerGrpcClient.ProcessReplace(
	req.Engine.Name, binary, DefaultEnginePortCount, args, []string{DefaultPortArg}, DefaultTerminateSignal)
	if err != nil {
	return nil, err
	}
	return parseProcess(imapi.RPCToProcess(process)), nil
	}
	instance, err := c.instanceServiceGrpcClient.InstanceReplace(string(req.Engine.Spec.BackendStoreDriver), req.Engine.Name,
	string(longhorn.InstanceManagerTypeEngine), binary, DefaultEnginePortCount, args, []string{DefaultPortArg}, DefaultTerminateSignal)
	if err != nil {
	return nil, err
	}
	return parseInstance(instance), nil
	}

• Ensure validation flags are passed to live upgraded engines.

[ejweber]

There is a new commit that fixes cloning, which is not included in the core tests. My code mistakenly assumed that the volume name we cloned from and to would be the same. This caused us to try to communicate with a from replica using the to volume name (triggering an invalid gRPC error). Please rereview when you are able.

cc @PhanLe1010 @james-munson

[shuo-wu]

LGTM

[PhanLe1010]

I think we need to pump this version to 5 too

longhorn-manager/engineapi/instance_manager.go

Line 23 in 83abf0f

CurrentInstanceManagerAPIVersion = 4

[PhanLe1010]

LGTM

[ejweber]

@mergify backport v1.5.x

[mergify]

backport v1.5.x

✅ Backports have been created

• #2128 Engine identity validation (backport #1897) has been created for branch v1.5.x but encountered conflicts