agent: return error when failing to apply network key

[stevvooe]

Signed-off-by: Stephen J Day stephen.day@docker.com

Closes #2592

[codecov]

Codecov Report

Merging #2593 into master will increase coverage by 0.5%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #2593     +/-   ##
=========================================
+ Coverage   61.51%   62.01%   +0.5%     
=========================================
  Files         134      134             
  Lines       21761    21761             
=========================================
+ Hits        13386    13495    +109     
+ Misses       6933     6815    -118     
- Partials     1442     1451      +9

[cyli]

LGTM to return an error rather than a panic.

[cyli]

Unrelated to this fix, but this logic seems a little odd:

	for _, key := range message.NetworkBootstrapKeys {
		same := false
		for _, agentKey := range a.keys {
			if agentKey.LamportTime == key.LamportTime {
				same = true
			}
		}
		if !same {
			a.keys = message.NetworkBootstrapKeys
			if err := a.config.Executor.SetNetworkBootstrapKeys(a.keys); err != nil {
				return errors.Wrap(err, "configuring network key failed")
			}
		}
	}

It looks like we are looping over each bootstrap key, and for each key that is different, we set the agent keys to the bootstrap keys - so if multiple keys are different, we'd set the same set of keys multiple times.

This seems like an idempotent action, so it's probably not an issue, but it does give the executor more changes to fail.

Happy to change this in a different PR, though.

[cyli]

Is setting the network bootstrap keys an expensive or prone-to-failure operation? Can we just set it every time without checking for whether it's different or the same? cc @abhi

[cyli]

From slack conversation - we're not sure whether it's expensive.

[thaJeztah]

LGTM

ping @dperny @anshulpundir PTAL (should be an easy merge)

[dperny]

LGTM. the end result of this is just that instead of panicking, an error will be logged.