credhub
credhub copied to clipboard
cloudfoundry
Fetch metadata in batches
This fixes an issue with Postgres and instances with >= 32768 certificates available to a user.
CertificateDataService.findAllValidMetadata(names) puts all cert names in a single IN (...) SQL query, which JdbcTemplate turns into a query with a bind variable for each cert name (IN (?, ?, ?, ...)). This exhausts Postgres's per-query bind variable limit.
Fixes https://github.com/cloudfoundry/credhub/issues/340
The PR also includes
- Local DB automation
- Fixes I needed to make existing tests pass with Postgres and MySQL
- A commit adding a couple of
@DirtiesContextannotations, helping with resource consumption
We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.
The labels on this github issue will be updated when the story is started.
Thank you @andreasf, we have performed an initial review of the issue and this PR. It makes a lot of sense. We do notice that the following commits are not essential to the fix of the issue described & might require further validation on our part:
- https://github.com/cloudfoundry/credhub/pull/346/commits/7714b81edc6a54464214cdb3be1895b940b0f775: This commit is solving a test run issue that we do not observe in our testing environment. Further, we should consider having these datasource params be configurable. We need to validate this change further.
- https://github.com/cloudfoundry/credhub/pull/346/commits/8b4e58c0f40ad6b6673c3fbcbc8810fc9ba59dda: Currently, CredHub is only run with Java 8 in credhub-release. We will likely need to bump to Java 11 at some point, which will require a more systematic effort. For now, making CredHub run on Java 9 is not a prioritized effort & not essential to the fix.
- https://github.com/cloudfoundry/credhub/pull/346/commits/9ec4d7faf4ce8ee139fceffaab7adb3cd6b96475: Could you describe the specific issues that this change would solve? Does it make the tests less flaky, faster, more resource-saving? We also need to look into this further.
To expedite delivering the fix to the issue, could we ask you to extract these 3 commits as 1 or more separate PRs (these 3 commits are helpful but will require more time to validate)? So that this PR only contains the commits essential to the fix (the other 2 commits look great, and probably can merge soon)?
@hsinn0 and I
We cherry-picked part of the commits to create separate pull requests as follows:
- https://github.com/cloudfoundry/credhub/pull/396
- https://github.com/cloudfoundry/credhub/pull/399
