Repository navigation
Detect cloud-init failure at startup to prevent silent cluster miscon… - #4988
Conversation
d151b94 to
eea8c52
Compare
eea8c52 to
48f01ca
Compare
48f01ca to
c7e91f3
Compare
|
|
||
| userData, err := ec2Client.GetUserData() | ||
| if err != nil { | ||
| seelog.Infof("cloud-init used DataSourceNone; unable to verify if userdata exists on IMDS, skipping check: %v", err) |
There was a problem hiding this comment.
Q: thinking more on this now, why do we not want to throw a terminal error if we're unable to get the userdata? is this for the edge case where IMDS is not available on the host or something else?
There was a problem hiding this comment.
yeah, along similar lines, I want to understand.
what happens for no userdata case? will it be an error or empty userdata?
what happens for IMDS not reachable case? agent should always be able to reach to IMDS else why start on broken host for unreliable failures/config down the line?
There was a problem hiding this comment.
My intention for this check is to catch the narrow case where cloud-init failed to fetch userdata but it is available now. That is why this check skips when it cannot establish if userdata is available now. Also it is possible to have Agent run with IMDS disabled (by providing everything that Agent needs from IMDS via env vars instead), so failing here would block such use cases. It is okay if this check has some leaks but I don't want it to break any legitimate setups.
There was a problem hiding this comment.
We can remove this check altogether. It only exists to allow Agent to run in cases when there was no userdata to begin with thus making cloud-init failure a no-op from ECS config POV.
There was a problem hiding this comment.
what happens for no userdata case? will it be an error or empty userdata?
In that case err is non-nil due to a 404 response from IMDS and we don't block Agent.
what happens for IMDS not reachable case? agent should always be able to reach to IMDS else why start on broken host for unreliable failures/config down the line?
It is possible to configure Agent to run without IMDS access by providing overrides for region, credentials etc. using environment variables or a custom credentials file (/rotatingcreds/credentials), so failing here can break such setups. If Agent is configured to require IMDS then it will fail regardless of whether we fail in this branch or not.
There was a problem hiding this comment.
We can remove this check altogether. It only exists to allow Agent to run in cases when there was no userdata to begin with thus making cloud-init failure a no-op from ECS config POV.
This is a legitimate case though, no? Wouldn't that mean if no userdata is specified and IMDS is transiently unreachable during instance boot (but then possibly reachable later), that ECS Agent would fail to start up? I expect that customers who don't specify userdata wouldn't want ECS Agent to fail to start up in this case.
There was a problem hiding this comment.
Also it is possible to have Agent run with IMDS disabled (by providing everything that Agent needs from IMDS via env vars instead), so failing here would block such use cases.
Ah gotcha, mainly for external/ECS Anywhere scenarios iiuc. Although, there isn't anything blocking anyone from doing it on EC2 hosts as well. Thanks for clarifying.
There was a problem hiding this comment.
Ah gotcha, mainly for external/ECS Anywhere scenarios iiuc.
Yes but for external mode this check is not even invoked.
There was a problem hiding this comment.
This is a legitimate case though, no? Wouldn't that mean if no userdata is specified and IMDS is transiently unreachable during instance boot (but then possibly reachable later), that ECS Agent would fail to start up? I expect that customers who don't specify userdata wouldn't want ECS Agent to fail to start up in this case.
Yep.
75ac162 to
e8c15c9
Compare
…figuration When IMDS is transiently unreachable during instance boot, cloud-init falls back to DataSourceNone and skips userdata execution. The agent then starts with default config and silently registers to the wrong cluster. Add a best-effort check at agent startup: if cloud-init's result.json reports DataSourceNone and userdata exists on IMDS, the agent terminally exits (code 5) so ASG replaces the instance. The check is fail-open — any error reading or parsing the file causes it to be skipped. Changes: - ecs-init: bind mount /var/lib/cloud/data/result.json into agent container - agent: add CheckCloudInitFailure() in config package (Linux only) - agent: call the check in doStart() before Docker version verification
e8c15c9 to
07efcae
Compare
Summary
When IMDS is transiently unreachable during instance boot (e.g., ENI initialization failure), cloud-init falls back to DataSourceNone and skips userdata execution. The ECS agent then starts with default config which is undesirable.
This adds a best-effort check at agent startup that detects this condition and terminally exits (code 5) so Agent does not run with default configuration.
Implementation details
Detection logic:
/var/lib/cloud/data/result.json(bind-mounted by ecs-init)DataSourceNoneAND IMDS/latest/user-datareturns non-empty content → cloud-init failed to execute userdataFail-open design: Any failure to read or parse the file causes the check to be skipped. The check can never incorrectly prevent the agent from starting.
Changes:
ecs-init/docker/docker_config.go: Optional read-only bind mount of/var/lib/cloud/data/result.json(skipped in External mode, logs warning if file missing)agent/config/cloudinit_check_linux.go:CheckCloudInitFailure()function with fail-open semanticsagent/config/cloudinit_check_unsupported.go: No-op stub for non-Linuxagent/app/agent.go: Call the check indoStart()before Docker version verification (skipped in External mode)Testing
New tests cover the changes: yes
Description for the changelog
Feature - Detect cloud-init failure at startup to prevent silent cluster misconfiguration when IMDS is transiently unreachable during boot
Additional Information
Does this PR include breaking model changes? No
Does this PR include the addition of new environment variables in the README? No
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.