-
Notifications
You must be signed in to change notification settings - Fork 12
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: Default false
for sslUseDefaultTrustStore
#906
base: main
Are you sure you want to change the base?
Conversation
false
for sslUseDefaultTrustStore
false
for sslUseDefaultTrustStore
hana/package.json
Outdated
@@ -43,7 +43,8 @@ | |||
"kinds": { | |||
"sql": { | |||
"[production]": { | |||
"kind": "hana" | |||
"kind": "hana", | |||
"sslUseDefaultTrustStore": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need that?:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure that this property would be required to be part of the credentials
object. As that is what being send to the database driver. Also this seems to only be a problem with hdb
it shouldn't be a problem with @sap/hana-client
. Which would mean that it should probably go into:
cds-dbs/hana/lib/drivers/hdb.js
Lines 23 to 41 in 732a2f3
constructor(creds) { | |
creds = { | |
fetchSize: 1 << 16, // V8 default memory page size | |
...creds, | |
} | |
// Retain hana credential mappings to hdb / node credential mapping | |
for (const m of credentialMappings) { | |
if (m.old in creds && !(m.new in creds)) creds[m.new] = creds[m.old] | |
} | |
super(creds) | |
this._native = hdb.createClient(creds) | |
if (dt_sdk_is_present()) this._native = wrap_client(this._native, creds, creds.tenant) | |
this._native.setAutoCommit(false) | |
this._native.on('close', () => this.destroy?.()) | |
this.connected = false | |
} |
As the change was made in hdb
to resolve a long running issue. I am hesitant to have our implementation overwrite the default. The main problem with this change is when the HANAService
has a large connection pool or a lot of tenants. As out default max
pool size is 10
. It would require ~1000 tenants to reach the reported state in the issue.
Additionally it is possible to use the previous hdb
version which does not have this behavior to achieve the same state as this PR is trying to achieve. Which only becomes a problem when root issue shows up as it would require the hdb
upgrade, but then having it disabled by @cap-js/hana
would not allow CAP applications to connect to their HANA system at all anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that this is the better place. I moved it to hdb.js
. We could also say we change the default for multitenancy only for now, as there's a higher probability stakeholders will run into issues.
For that the internal PR for cds-mtxs
would be enough, so we don't have to change cds-dbs
at all – keeping this PR in draft state until the DB sync on Monday.
is that already fixed by hdb 0.19.11? |
|
No, with hdb 0.19.11 does not fix the issue, just change certificates that are mixed in... |
Possible workaround for SAP/node-hdb#240