Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not overwrite AWS config when sync fails #569

Merged
merged 2 commits into from Dec 14, 2023

Conversation

sosheskaz
Copy link
Contributor

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
  3. 95488
  4. Disconnect network
  5. granted registry sync
  6. wc < ~/.aws/config
  7. With current version: 0
  8. 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
Copy link
Collaborator

Eddie023 commented Dec 8, 2023

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
Copy link
Contributor Author

done

@sosheskaz
Copy link
Contributor Author

Configuration is respected and controller per-repo, but:

  • Preserving the atomic overwrite behavior in all cases, which is a good idea regardless.
  • If any repositories with the new default behavior fail, they prevent overwriting the config for all others.

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
Copy link
Contributor

chrnorm commented Dec 14, 2023

Thankyou @sosheskaz and thankyou @Eddie023 for the review 🙌 🙌

@chrnorm chrnorm merged commit dfa6ad4 into common-fate:main Dec 14, 2023
sosheskaz added a commit to sosheskaz/granted that referenced this pull request Dec 19, 2023
* Do not overwrite AWS config when sync fails

Fix issue common-fate#386

* Add WriteOnSyncFailure option to repo sync
@gautamg795
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants