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

[Feature] Add validation for etcd resources #217

Closed
stoyanr opened this issue Aug 5, 2021 · 4 comments · Fixed by #230
Closed

[Feature] Add validation for etcd resources #217

stoyanr opened this issue Aug 5, 2021 · 4 comments · Fixed by #230
Assignees
Labels
kind/enhancement Enhancement, improvement, extension

Comments

@stoyanr
Copy link
Contributor

stoyanr commented Aug 5, 2021

Feature (What you would like to be added):

Please add validation code for etcd resources, similarly to the validation code that already exists for other Gardener extension resources, even though this is technically still dead code.

We are currently working on a new validating webhook in seed-admission-controller for such extension resources, see gardener/gardener#4293, I think we could include the validation of etcd resources there as well. Alternatively, etcd-druid could introduce its own validating webhook if for whatever reason the above option is not good enough.

Motivation (Why is this needed?):

We recently had a rather severe issue that could have been prevented if we had such validation in place, see gardener/gardener-extension-provider-azure#328 (comment). In this particular case, gardenlet was generating an etcd resource with a spec.backup.store.prefix set to -- due to a data race. With validation in place, we could have detected -- as an invalid spec.backup.store.prefix and prevented the reconciliation from continuing. This particular issue is already fixed in gardenlet (see gardener/gardener#4459 and gardener/gardener#4454), but similar issues may occur in the future.

Approach/Hint to the implement solution (optional):

@stoyanr stoyanr added the kind/enhancement Enhancement, improvement, extension label Aug 5, 2021
@stoyanr
Copy link
Contributor Author

stoyanr commented Aug 5, 2021

/cc @amshuman-kr @plkokanov @vanjiii

@amshuman-kr
Copy link
Collaborator

Thanks for creating the issue @stoyanr! @shreyas-s-rao created another issue #213 for the same purpose. But the approach is slightly different that it is suggesting to make the backup configuration fields immutable which will have much of the same effect as the validation proposed here. Should we keep and address both issues? WDYT?

@stoyanr
Copy link
Contributor Author

stoyanr commented Aug 6, 2021

Sorry @amshuman-kr I didn't see that issue yesterday. I think both approaches are valid and should be considered. Validation of etcd resources is needed for both of them, so the 2 issues are somewhat duplicates. Feel free to close one of them if you prefer.

@vanjiii
Copy link
Contributor

vanjiii commented Sep 9, 2021

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Enhancement, improvement, extension
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants