Do not destroy SNIs when updating unrelated certificate fields #386
Conversation
|
cert_test.txt shows some example syncs with the changes against testcerts.tar.gz Note that there was one small change after the test run: originally, I only copied the current SNIs onto the new certificate, so this would spuriously report that it was adding SNIs even if they were unchanged (this was only a cosmetic issue with the diff--the PUT had effect it should). I later amended the commit to copy it onto both, since the current certificate in the update is only used in the diff, and doing so resolves that diff issue. |
rainest
force-pushed
the
fix/preserve-cert-sni
branch
from
3834fb4
to
6de9ee2
Compare
Codecov Report
@@ Coverage Diff @@
## main #386 +/- ##
==========================================
- Coverage 53.59% 53.52% -0.08%
==========================================
Files 62 62
Lines 5034 5069 +35
==========================================
+ Hits 2698 2713 +15
- Misses 2037 2057 +20
Partials 299 299
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Ultimately my read of this patch based on the description is that this will take care of it, I have no blockers (though please do see my comments below) however, due to the lack of testing for this code path I would appreciate if we could please write a small report about our manual testing of this patch before we merge this.
|
In the spirit of atomic and smaller PRs, please send a different PR for the typo and remove that commit from this PR. |
| @@ -85,6 +85,18 @@ func (sc *Syncer) createUpdateCertificate( | |||
|
|
|||
| // found, check if update needed | |||
| if !currentCertificate.EqualWithOpts(certificateCopy, false, true) { | |||
| currentSNIs, err := sc.currentState.SNIs.GetAllByCertID(*currentCertificate.ID) | |||
There was a problem hiding this comment.
This is a fairly special case. Do you mind copying the git commit body in here as a code comment to help future maintainers?
|
Please only merge the relevant patch in this PR. The typo fix should be its own PR. |
When decK must update a certificate, retrieve the current certificate's set of SNIs, convert them to strings, and set these on the updated certificate. Certificate and SNI objects have a special relationship. A PUT request (which we use for updates) with a certificate that contains no SNI children will in fact delete any existing SNI objects associated with that certificate, rather than leaving them as-is. Because decK considers SNIs separate objects and strips SNI child objects from certificate objects, updates to other certificate fields will PUT a certificate with no SNIs and inadvertently delete existing SNIs. Not stripping SNIs from certificate objects in general presents its own issues, as decK will attempt to operate on both objects and generate conflicts. To work around these issues, this change sets SNIs on certificates ONLY during update requests using the current certificate's SNI list. If there are changes to the SNIs, subsequent actions on the SNI objects will handle those. Fix #356
rainest
force-pushed
the
fix/preserve-cert-sni
branch
from
368a6f8
to
c010cdb
Compare
When decK must update a certificate, retrieve the current certificate's set of SNIs, convert them to strings, and set these on the updated certificate. Certificate and SNI objects have a special relationship. A PUT request (which we use for updates) with a certificate that contains no SNI children will in fact delete any existing SNI objects associated with that certificate, rather than leaving them as-is. Because decK considers SNIs separate objects and strips SNI child objects from certificate objects, updates to other certificate fields will PUT a certificate with no SNIs and inadvertently delete existing SNIs. Not stripping SNIs from certificate objects in general presents its own issues, as decK will attempt to operate on both objects and generate conflicts. To work around these issues, this change sets SNIs on certificates ONLY during update requests using the current certificate's SNI list. If there are changes to the SNIs, subsequent actions on the SNI objects will handle those. Fix #356
The first commit here is a noisy chore. Review 6de9ee2 for the meat of this change.
#356 occurs when updating a certificate without modifying its SNIs. We strip SNIs from certificate objects to handle SNIs separately, but the Kong admin API behavior for certificate PUTs handles an empty SNI set on certificates as an instruction to delete those SNIs (probably because SNIs can't exist independent of certificates and because there's a strict one-to-one certificate-to-SNI relationship).
To work around this, this change copies the existing certificate's SNIs out of state during updates and sets them on the updated certificate object before sending it. This preserves the SNI list if it is unchanged while still updating SNIs if they do change (subsequent diffs on the SNIs themselves handle that).
Fix #356