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

Add ADR for support for etcd s3 config secret #9364

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

brandond
Copy link
Contributor

@brandond brandond commented Feb 6, 2024

Proposed Changes

  • Add ADR for etcd s3 config secret
  • Add implementation

Types of Changes

new feature

Verification

See ADR

Testing

Linked Issues

User-Facing Change

Further Comments

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
@cwayne18
Copy link
Collaborator

@jakefhyde

Copy link
Contributor

@jakefhyde jakefhyde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this applies to both the standalone case and Rancher, but if the s3 credential is rotated and you restore using a snapshot with an outdated s3 credential, are we planning to do anything on that front? I'm mostly worried about this in the case where a restore is performed and everything looks fine, but snapshots aren't being uploaded correctly to s3 which isn't going to be immediately visible to a user until there is a problem (I'm expecting on the Rancher side we'll create some controller to sync the secret with the downstream cluster, so it may be a moot point, but just wanted to pick your brain on this). Stale credentials may get left over post-restore, but that's a much smaller concern IMO. Also curious on which form will have priority if both mechanisms are provided during the migratory period.

@brandond
Copy link
Contributor Author

brandond commented Feb 16, 2024

if the s3 credential is rotated and you restore using a snapshot with an outdated s3 credential, are we planning to do anything on that front?

I'm not sure that we have a good way to take care of that for the user. I think the user, or an external controller (ie rancher), would need to be responsible for updating anything that has drifted since the snapshot was taken - that would include auth secrets. Other things like ImagePullSecrets are probably affected by this as well.

which form will have priority if both mechanisms are provided during the migratory period.

in the ADR I suggested that cli/config should always take precedence over the Secret. I'll update the ADR to make this more clear, calling them "defaults" probably isn't the correct language.

  • The Secret will provide default values; if S3 configuration is passed via CLI flags or configuration file, ALL fields set by the Secret will be ignored. Secret and CLI/config values will NOT be merged.

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
@brandond
Copy link
Contributor Author

brandond commented Apr 2, 2024

ADR has been updated to clarify intent.

@brandond brandond requested a review from jakefhyde April 2, 2024 18:48
Copy link
Member

@Oats87 Oats87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a high level this seems fine -- I don't see any issues with trying to implement support for this with the planner; however, that is not to say that there might be implementation details we discover down the line after initial implementation.

If possible, if the K3s/RKE2 implementation could be shipped in a pre-release that we could test on the planner side to see how well it can integrate, I think that would be best to ensure we don't end up shipping a feature that we'll need to use workarounds in Rancher with.

@Oats87
Copy link
Member

Oats87 commented Apr 2, 2024

Still needs review by @jakefhyde

@brandond
Copy link
Contributor Author

brandond commented Apr 2, 2024

@snasovich I can't add you as a reviewer; I don't think you're in the correct groups, but would you mind taking a look as well?

@brandond brandond changed the title Add support for etcd s3 config secret Add ADR for support for etcd s3 config secret Jun 4, 2024
@brandond
Copy link
Contributor Author

brandond commented Jun 4, 2024

Will merge this with just ADR, and add implementation in a future release cycle.

@brandond brandond merged commit df5db28 into k3s-io:master Jun 4, 2024
2 checks passed
@brandond brandond deleted the etcd-s3-secret branch June 6, 2024 21:14
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.

6 participants