Do not overwrite AWS config when sync fails #569
Conversation
|
Thanks for the PR @sosheskaz. I do agree with your proposed change. However, this change can be a configurable option for backward compatibility and for people who would like the previous flow. |
|
done |
|
Configuration is respected and controller per-repo, but:
This is probably more desireable than a global flag, as it opens the door to "fallback" registries and allows more flexible configuration. I am imagining two use-cases here: Use-case 1: Stable / Unstable registries.In this scenario, we have a registry that we do not want to ever be in a broken state, and we have others that we are okay with breaking. In that case, we can add the option to the "unstable" registries. This means that any failure for the unstable registry will result in those entries being removed from the config file. However, entries will not be removed from the "stable" registry due to a sync failure. e.g. Use-case 2: Fallback registriesIf registries are critical, we may choose to keep a mirror for high availability. In that case, we may have two registries, set up like: Which would effectively guarantee that viable configs are always present. And, assuming the mirror is up-to-date, also always gives the latest available, provided the backup remains reachable (although, it would stop overwriting if backup were ever down). |
|
Thankyou @sosheskaz and thankyou @Eddie023 for the review 🙌 🙌 |
* Do not overwrite AWS config when sync fails Fix issue fwdcloudsec#386 * Add WriteOnSyncFailure option to repo sync
|
FYI, per #600 it looks like this broke setups where It might be safer to open a temporary directory under the same path as the existing config file? |
Fix issue #386 by writing the generated AWS config to a temp file, and replacing current AWS config if and only if all registry syncs succeed.
What changed?
When
granted registry syncfails, do not alter ~/.aws/config.Why?
Overwriting ~/.aws/config on failure creates a dangerous situation. If Github (or whatever VCS is being used) is down, and the sync runs automatically, then users are be unable to use the service.
Also, on machines that may not be connected to the VCS 24/7 (e.g. VPN, scheduled network outage, not connected to internet), granted registry sync may run automatically, leading to an inconvenience for a user because
granted registry syncmust be re-run manually.How did you test it?
granted registry syncwc < ~/.aws/config95488granted registry syncwc < ~/.aws/config0dgrantedinstead ofgranted):95488Potential risks
This changes behavior which was previously intentional. However, I think that the downside of this is too high. Currently, we are effectively suppressing failures, and breaking the environment in the process. This will make the error louder, and more explicit.
There may be an argument that both failure modes should be supported. However, as a "Principle of Least Surprise" thing, I feel very strongly that this should be the default behavior.
Is patch release candidate?
Link to relevant docs PRs