Check example-checksum label for consistency in ConfigMap webhook. #1366
Conversation
We've seen users try to edit our configuration and falling into the trap of editing the '_example' block a lot. This attempts at guiding the users to do "the right thing" by checking the ConfigMap's '_example' value (if present) against a precomputed hash of the same value (if present). The idea is that we precompute this has using the tool herein in code generation and thus allow us to easily change the example block automatically while making it hard to change it on ppurpose. If the hashes don't match on an upgrade, the webhook will return an error synchronously, guiding the user to the correct behavior.
knative-prow-robot
added
the
size/XXL
| exampleHash != configmap.ExampleHash(exampleData) { | ||
| return fmt.Errorf( | ||
| "%q block edited, you likely wanted to create an unindented configuration", | ||
| configmap.ExampleKey) |
There was a problem hiding this comment.
I wonder if we should include the diff 🤔
There was a problem hiding this comment.
Diff of the update or diff of the hash?
configmap/example_test.go
Outdated
| import "testing" | ||
|
|
||
| func TestExampleHash(t *testing.T) { | ||
| if got, want := ExampleHash(""), "e3b0c4429"; got != want { |
There was a problem hiding this comment.
It feels as if this deserves to be a constant :)
There was a problem hiding this comment.
Which one exactly?
There was a problem hiding this comment.
The empty string. Since this is what we'll have if example section is deleted. But doesn't matter anyway.
| } | ||
| content := doc.Content[0] | ||
|
|
||
| example := traverse(content, "data", configmap.ExampleKey) |
There was a problem hiding this comment.
If we cared to constant-ize Example key, why not "data" then as well?
There was a problem hiding this comment.
Cause it's the default key for a ConfigMap. The example key was also a constant before so 🤷♀️. We'd have to go create constants for "metadata" and "labels" as well. Felt like overkill.
| labels := traverse(content, "metadata", "labels") | ||
| if labels == nil { | ||
| // TODO(markusthoemmes): Potentially handle missing metadata and labels? | ||
| return nil, errors.New("'metadata.labels' not found") |
There was a problem hiding this comment.
See comment. I can do that but it didn't feel worth it given all ConfigMaps we have already have the relevant fields and working with these Nodes is not super straightforward. I can add them if you feel like we should.
configmap/hash-gen/main.go
Outdated
| } | ||
|
|
||
| buffer := &bytes.Buffer{} | ||
| encoder := yaml.NewEncoder(buffer) |
There was a problem hiding this comment.
We probably don't care much here, but we can just pass in the bufio.Writer wrapping around the file, but this is not a service, so 🤷
There was a problem hiding this comment.
I thought about the same, but opening a file for writing and making sure the stars all align felt like more ceremony than it was worth it so I just kept it simple here. Doesn't matter performancewise anyway.
|
@antoineco and @vagababov thanks for the thorough reviews (and sorry for the somewhat subpar implementation). I think this is ready for another look. I'm not sure I feel strongly about the checksum algorithm. Just sticking to SHA256 sounded fair to me, given git uses it in the same way (which is why I picked it too) and given performance doesn't matter. |
markusthoemmes
changed the title
|
I don't feel strongly about the algorithm either, I just figured SHA was often used when it is not desired that clashes can be introduced, while the use case here doesn't have such constraint. Feel free to discard my comment. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markusthoemmes, mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
The following is the coverage report on the affected files.
|
We've seen users try to edit our configuration and falling into the trap of editing the '_example' block a lot. This attempts at guiding the users to do "the right thing" by checking the ConfigMap's '_example' value (if present) against a precomputed hash of the same value (if present). The idea is that we precompute this has using the tool herein in code generation and thus allow us to easily change the example block automatically while making it hard to change it on ppurpose. If the hashes don't match on an upgrade, the webhook will return an error synchronously, guiding the user to the correct behavior.
Downstream PRs:
knative/eventing#3231
knative/serving#8123
/assign @mattmoor