-
Notifications
You must be signed in to change notification settings - Fork 729
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 PodTemplate semantic validation for Elasticsearch #3020
Conversation
The validation fails with the following error on GKE 1.14: I think it means that the validation proposed here should only be a "best effort" validation: as long as the error is not explicitly a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
// Mutate Elasticsearch with an invalid PodTemplate | ||
b.UpgradeTestSteps(k), | ||
).WithStep( | ||
test.Step{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation feels a bit off here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed but this file is formatted with go fmt
🤷♂️
) error { | ||
var statusErr *k8serrors.StatusError | ||
if !errors.As(err, &statusErr) { | ||
// Not a Kubernetes API error (e.g. timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we could always reach a timeout in some situations (eg. particular proxy configuration). Hard to know for sure, so I'm fine moving on with this.
Should we add some debug logs here just in case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if we reach a timeout here it is either:
- a transient error, in this case it is fair to try again in the next reconciliation loop
- a permanent error, in which case the StatefulSet update/creation that should happen right after will also fail
The error is already reported in the controller log, I'm not sure we should duplicate it.
To explain a little bit more the issue with the admission webhook: as explained in the documentation:
In order for an admission webhook to state if it supports dry-run its
apiVersion: v1
items:
- apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
name: pod-identity-webhook
webhooks:
- admissionReviewVersions:
- v1beta1
failurePolicy: Ignore
matchPolicy: Exact
name: iam-for-pods.amazonaws.com
....
sideEffects: Unknown Unfortunately on EKS it seems that the Regarding GKE it seems that built-in admission webhooks are compatible since 1.15. I will update the e2e test to ensure that it is only run on GKE for K8S > 1.15. TL;DR: even if dry run requests have been promoted to beta in K8S 1.13 the admission webhooks are not all compatible with that feature 😢 |
This PR adds a semantic validation for the content of the
podTemplate
field of the Elasticsearch resources.It is achieved by performing a dry-run request which attempts to create a Pod reusing the content of the
podTemplate
. The validation is only performed when aStatefulSet
is created or updated.Fixes #2266