Address review comments from @anshulpundir

Signed-off-by: Sune Keller <absukl@almbrand.dk>

agent/secrets/secrets.go

@@ -71,10 +71,14 @@ func (sp *taskRestrictedSecretsProvider) Get(secretID string) (*api.Secret, erro
 		return nil, fmt.Errorf("task not authorized to access secret %s", secretID)
 	}
 
+	// First check if the secret is available with the task specific ID, which is the XOR of the original secret ID and the task ID.
+	// That is the case when a secret driver has returned DoNotReuse == true for a secret value.
 	secret, err := sp.secrets.Get(identity.XorIDs(secretID, sp.taskID))
 	if err != nil {
+		// Otherwise, which is the default case, the secret is retrieved by its original ID.
 		return sp.secrets.Get(secretID)
 	}
+	// For all intents and purposes, the rest of the flow should deal with the original secret ID.
 	secret.ID = secretID
 	return secret, err
 }

identity/randomid.go

@@ -59,7 +59,7 @@ func NewID() string {
 // Note that XorIDs is not guaranteed to produce an ID of the same length as
 // NewID, or with the same properties.
 func XorIDs(id1, id2 string) string {
-	// first, we convert the ids back to big.Int, so we can do math on them
+	// First, we convert the ids back to big.Int, so we can do math on them
 	id1int, success := (&big.Int{}).SetString(id1, randomIDBase)
 	if !success {
 		panic(fmt.Errorf("Failed to convert first ID to big.Int{}"))
@@ -69,13 +69,14 @@ func XorIDs(id1, id2 string) string {
 		panic(fmt.Errorf("Failed to convert second ID to big.Int{}"))
 	}
 
-	// now, we xor the two numbers together.
+	// Now, we xor the two numbers together.
 	xorint := (&big.Int{}).Xor(id1int, id2int)
-	// if there are leading 0s in the result, they might get omitted, but we
-	// want to keep them affirmatively. thus, if the resulting string is less
+	// If there are leading 0s in the result, they might get omitted, but we
+	// want to keep them affirmatively. Thus, if the resulting string is less
 	// than maxRandomIDLength, then we should leading pad with 0s to make it
+	// the same length as other IDs.
 	xorstr := xorint.Text(randomIDBase)
-	for len(xorstr) < maxRandomIDLength {
+	for i := len(xorstr); i < maxRandomIDLength; i++ {
 		xorstr = "0" + xorstr
 	}
 	return xorstr

manager/dispatcher/assignments.go

@@ -48,7 +48,7 @@ func assignSecret(a *assignmentSet, readTx store.ReadTx, mapKey typeAndID, t *ap
 		}).Debug("failed to fetch secret")
 		return
 	}
-	// If a driver is used, give a unique ID per task to allow different values for different tasks
+	// If the secret should not be reused for other tasks, give it a unique ID for the task to allow different values for different tasks.
 	if doNotReuse {
 		// Give the secret a new ID and mark it as internal
 		originalSecretID := secret.ID
@@ -306,7 +306,9 @@ func (a *assignmentSet) message() api.AssignmentsMessage {
 }
 
 // secret populates the secret value from raft store. For external secrets, the value is populated
-// from the secret driver.
+// from the secret driver. The function returns: a secret object; an indication of whether the value
+// is to be reused across tasks; and an error if the secret is not found in the store, if the secret
+// driver responds with one or if the payload does not pass validation.
 func (a *assignmentSet) secret(readTx store.ReadTx, task *api.Task, secretID string) (*api.Secret, bool, error) {
 	secret := store.GetSecret(readTx, secretID)
 	if secret == nil {

manager/drivers/secrets.go

@@ -25,7 +25,10 @@ func NewSecretDriver(plugin plugingetter.CompatPlugin) *SecretDriver {
 	return &SecretDriver{plugin: plugin}
 }
 
-// Get gets a secret from the secret provider
+// Get gets a secret from the secret provider. The function returns: the secret value;
+// a bool indicating whether the value should be reused across different tasks (defaults to false);
+// and an error if either the spec or task are nil, if calling the driver returns an error, or if
+// the driver returns an error in the payload.
 func (d *SecretDriver) Get(spec *api.SecretSpec, task *api.Task) ([]byte, bool, error) {
 	if spec == nil {
 		return nil, false, fmt.Errorf("secret spec is nil")