common-fate / granted

The easiest way to access your cloud.
https://granted.dev
MIT License
957 stars 90 forks source link

Do not overwrite AWS config when sync fails #569

Closed sosheskaz closed 6 months ago

sosheskaz commented 7 months ago

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 sync fails, 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 sync must be re-run manually.

How did you test it?

  1. granted registry sync
  2. wc < ~/.aws/config
    1. 95488
  3. Disconnect network
  4. granted registry sync
  5. wc < ~/.aws/config
    1. With current version: 0
    2. With this version (i.e. using dgranted instead of granted): 95488

Potential 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

Eddie023 commented 7 months ago

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.

sosheskaz commented 7 months ago

done

sosheskaz commented 7 months ago

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.

dgranted registry add --name=stable --url https://github.com/my-org/my-registry
dgranted registry add --name=unstable --url https://github.com/my-org/my-other-registry --wosf

Use-case 2: Fallback registries

If registries are critical, we may choose to keep a mirror for high availability. In that case, we may have two registries, set up like:

dgranted registry add --name=main --url https://github.com/my-org/my-registry --wosf
dgranted registry add --name=backup --url https://gitea.my-org.com/my-org/my-registry --pdp

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).

chrnorm commented 6 months ago

Thankyou @sosheskaz and thankyou @Eddie023 for the review πŸ™Œ πŸ™Œ

gautamg795 commented 2 months ago

FYI, per #600 it looks like this broke setups where /tmp is on a different device than /home (or wherever the user's AWS config might be stored)

It might be safer to open a temporary directory under the same path as the existing config file?