Append extra_hosts from all tasks

[lgfa29]

#10823 changed how /etc/hosts to be mounted from the alloc dir instead of the task dir so that all tasks share the same configuration.

The problem is that allocations with multiple tasks would only use the values of extra_hosts from the task that starts first, ignoring all others. This prevents Consul Connect tasks from using extra_hosts because the sidecar proxy doesn't define any extra_hosts, so the main task configuration is ignored.

This fix appends any extra_hosts values if the hosts file already exists in the alloc dir.

Closes #11056

[schmichael]

I didn't notice it until now, but I'm afraid #10823 introduced a textbook time-of-check to time-of-use (TOCTOU) bug.

The key piece of prerequisite knowledge is that TaskRunners, and therefore Drivers, run concurrently within a single allocation.

Prior to #10823 hosts files had a single writer: their TaskRunner+Driver.

#10823 changed the file location so that it was shared between all tasks in an allocation: thus introducing a problem where the single file has multiple writers. Two tasks may do the Stat(...) call and both receive DoesNotExist errors, and then proceed to race to do the write.

Since the hosts file is usually quite small, the reads and writes probably all end being atomic, so the resulting file is probably always valid (no half-of-one write, half-of-another). However I think it's possible for tasks to still clobber each other's hosts files.

Sadly I'm not sure there's a simple fix. I think extra_hosts should be *on the group network stanza so it can be written once by the AllocRunner and included in a MountConfig passed to each TaskRunner+Driver. That's a pretty significant change though.

In the mean time we could document on extra_hosts is not safe to use for task groups with >1 non-lifecycle task.

[lgfa29]

Ahh that's a great point @schmichael.

It sounds like a more comprehensive refactoring of extra_hosts would be needed to pull it up to the group level, since it's only supported by Docker. This would probably help with #10768 as well.

I think I will close this for now then until we have a proper fix.

[schmichael]

There really needs to be a :sad-thumbs-up: reaction emoji to express my feelings about your plan. Sounds good though.

[lgfa29]

Closed with :sad-thumbs-up:

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.