Implement rename org#4341
Conversation
danail-branekov
left a comment
There was a problem hiding this comment.
I did not have the time to look in the change throughly, but it is great that you managed to implement a webhook validation.
I have two major comments:
- Computing the labels signature requires a secret which might be awkward with regards to lifecycle management
- If we manage to propagate the webhook error to the cli (see comments), then repositories should not explicitly need to validate the metadata patch (that would make repositories even simpler)
| const ReservedLabelDomain = "korifi.cloudfoundry.org/" | ||
|
|
||
| func Sign(secret []byte, labels map[string]string) string { | ||
| mac := hmac.New(sha256.New, secret) |
There was a problem hiding this comment.
I wonder whether using hmac signature with a secret is an overkill. Would it not be sufficient to sha256sum the reserved keys map? Then we would not need a secret that is seeded on helm install
| } | ||
|
|
||
| if !signer.Verify(r.signingSecret, obj.GetLabels(), storedSig) { | ||
| return admission.Denied(fmt.Sprintf( |
There was a problem hiding this comment.
Provided that in korifi we have a concept of returning webhook errors to the cli, see this example, I wonder could we not do the same here? Then, if the error from the webhook is propagated to the cli, then we would not need to validate the metadata patch in the repositories. This would significantly reduce the amount of changes, and all the metadata patch validation tests in the api would become redundand
Summary
This PR completes the org rename fix and hardens metadata protection across Korifi objects described in this Issue
It includes three parts:
What changed
Why repository-side validation should co-exist with the webhook solution
Both solutions protect different layers and should both remain in place.
Repository-side validation is still needed because:
The webhook solution is also needed because:
In short:
Together they provide correct API semantics and cluster-level enforcement.
Testing