Add validation webhook configurations for all resource types #2781
Conversation
Add webhook config for all types Add webhooks for all resources Add APM server webhook tests Fix webhook manifest + add Kibana and Ents Add v1beta1 Add checks to controller Rebase master
There was a problem hiding this comment.
LGTM, your implementation looks very clean. I like the test abstraction you found for the webhook testing.
The version validation for Elasticsearch is a bit more complex as we don't support 7.0.x but I think I would be OK to merge if that's the only thing keeping this from being included in 1.1.
| // supported Stack versions. See https://www.elastic.co/support/matrix#matrix_compatibility | ||
| var ( | ||
| SupportedAPMServerVersions = MinMaxVersion{Min: From(6, 2, 0), Max: From(8, 99, 99)} | ||
| SupportedElasticsearchVersions = MinMaxVersion{Min: From(6, 8, 0), Max: From(8, 99, 99)} |
There was a problem hiding this comment.
We don't support 7.0 but only > 7.1 because this is the version that made TLS available under the basic license. So because we also support 6.8 this needs to be a non-contiguous range (not sure if that's the right word)
There was a problem hiding this comment.
Good catch! This is a leftover line that I forgot to remove. I didn't touch the Elasticsearch validation code in this PR because I noticed this problem as well. It's a refactoring that I wanted to do in a subsequent PR but I forgot to mention it. The logic for Elasticsearch is unchanged.
| return commonv1.CheckSupportedStackVersion(as.Spec.Version, version.SupportedAPMServerVersions) | ||
| } | ||
|
|
||
| func checkNoDowngrade(prev, curr *ApmServer) field.ErrorList { |
There was a problem hiding this comment.
Can we downgrade APM? I'd have thought that we could, but I couldn't find an answer in the docs.
There was a problem hiding this comment.
Maybe @simitt or @graphaelli can advise? But my hunch would be that downgrades are not advisable as I think there is some version specific bootstrapping happening in Elasticsearch with index templates etc.
There was a problem hiding this comment.
As you say, it's not documented; so I am not sure either. I was following the example of Elasticsearch where even a patch version downgrade is not allowed by the validation check.
I do want to address that issue in another PR later. In the mean time, my feeling is that it's OK to keep the somewhat rigid check in place for now. People who are confident that a downgrade is not going to affect them can disable the webhook and still go through with it if they want to. WDYT?
There was a problem hiding this comment.
That's an interesting question, I honestly never tried to downgrade an installation, and I have never seen a request for it. As long as there is no real good reason I would not actively support downgrading.
| Operation: admissionv1beta1.Create, | ||
| Object: func(t *testing.T, uid string) []byte { | ||
| apm := mkApmServer(uid) | ||
| return serialize(t, apm) |
There was a problem hiding this comment.
Does it maybe make sense to offload the serialization calls to RunValidationWebhookTests, and change the ValidationWebhookTestCase.Object fields to a generic object? As it is there's a lot of boiler plate in TestCase.Object. That said, even if it does make sense I'm still okay with merging this in today as is and leaving that for a future refactoring.
|
jenkins test this please |
|
cla/check |
Adds validation webhook configurations for APM Server, Kibana, and Enterprise Search resources. This PR sets up the framework and basic checks (unknown fields, long names, invalid versions, and downgrades).
Fixes #2584