Skip to content
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

fix(db) iterate through all SNIs #3722

Merged
merged 4 commits into from Aug 23, 2018

Conversation

Projects
None yet
3 participants
@hishamhm
Copy link
Member

hishamhm commented Aug 22, 2018

This PR fixes the behavior of Kong master by making sure it always iterates over all SNIs.

The source of the problem was that the :for_* functions were being used at times as a "select" function, i.e., ignoring the fact that it returns a page at a time and not the full results. My approach was then to begin with a refactor, renaming these functions to :page_* (i.e. routes:page_for_service etc.), so that this distinction becomes harder to miss in the future.

Then, I added an iterator variant, :each_for_* (the page-for method was renamed aiming to get some symmetry with the each-for) — the implementation of :each_for_* reuses the logic of :each (which was refactored to be done at the generic-DAO level, because the implementation in both strategies was essentially identical: a row-iterator over strategy:page).

Finally, once all of this is in place, the actual fix is replacing two instances of :page_for_certificate into :each_for_certificate, making sure all items are iterated. This includes a regression test for /certificates in the Admin API. (The behavior as implemented HERE is that all SNIs are returned alongside the certificate; eventually these should probably be paginated as well, but we don't have a mechanism for two-level pagination yet, so I figured this behavior is at least an improvement over the current buggy behavior, and could be further improved in the future.)

Once this is accepted into master, there are similar fixes to be done in next in its uses of the :for_* methods for the newly-ported Upstreams and Targets.

Fixes #3718.

hishamhm added some commits Aug 22, 2018

refactor(db) rename for_* methods to page_for_*
Be more explicit about the fact that the for_* methods of the new DAO objects
return a page only.
fix(db) iterate through all snis
Fixes the iteration of SNIs, so that all SNIs of a certificate
are iterated through, and not only the first page.

Includes a regression test.
refactor(db) implement :each in terms of :page
Removes the strategy-specific implementations of :each, since both of them are
identically implemented in terms of :page -- :each becomes implemented in the
generic DAO layer only, using the :page method of each strategy.

We also prepare the ground for adding more iterators in the generic DAO by
making the internal row and page iterators reusable.
feat(db) introduce :each_for_* iterators
Introduces iterator versions of the :page_for_* methods,
reusing the internal iteration logic of :each.
@hishamhm

This comment has been minimized.

Copy link
Member Author

hishamhm commented Aug 22, 2018

As usual, Github is listing my commits out-of-order, but the logical sequence goes:

  1. refactor(db) rename for_* methods to page_for_* (0db6b43)
  2. refactor(db) implement :each in terms of :page (04ccf8a)
  3. feat(db) introduce :each_for_* iterators (0b4c871)
  4. fix(db) iterate through all snis (c8709d9)
@thibaultcha
Copy link
Member

thibaultcha left a comment

This is great! Looking ready for merge!

@thibaultcha thibaultcha merged commit be50425 into master Aug 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@thibaultcha thibaultcha deleted the fix/explicit-page branch Aug 23, 2018

@ionosphere80

This comment has been minimized.

Copy link

ionosphere80 commented Aug 23, 2018

Thanks for the quick fix!

@ionosphere80

This comment has been minimized.

Copy link

ionosphere80 commented Sep 28, 2018

Can you tell me when this fix will appear in a release?

@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Sep 28, 2018

@ionosphere80 This is available in 1.0.0rc1 (a release candidate, you can read more about it here: https://discuss.konghq.com/t/kong-1-0-rc1-available-for-download/1898). We are planning on a 0.15 and 1.0 release in a month from now, roughly. We do not have plans for a 0.14.2 release at this time.

@ionosphere80

This comment has been minimized.

Copy link

ionosphere80 commented Sep 28, 2018

Thanks! Can you point me to a potential change log for these future releases? I'm particularly curious about DNS fixes/improvements, specifically involving resolution of SRV records and recovery from temporary DNS errors.

@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Sep 28, 2018

@ionosphere80 We do not have a public facing changelog of upcoming releases at this time. From the top of my head, I am not aware of any DNS fixes to be made; can you elaborate? Has this issue been reported elsewhere? Or a PR merged somewhere?

@ionosphere80

This comment has been minimized.

Copy link

ionosphere80 commented Sep 28, 2018

I can't find an existing issue (open or closed) that completely describes my current DNS problems, so I'll open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.