-
Notifications
You must be signed in to change notification settings - Fork 126
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
SNOW-1553756 use forked version 99design/keyring
without kwallet/secretservice to avoid dbus issue
#1296
SNOW-1553756 use forked version 99design/keyring
without kwallet/secretservice to avoid dbus issue
#1296
Conversation
* use forked keyring from 99designs/keyring#103 (comment) which eliminates Linux kwallet/secretservice, which we don't use anyways but causes headache with dbus upon keyring init()
99design/keyring
without kwallet/secretservice to avoid dbus issue
@@ -2,6 +2,8 @@ module github.com/snowflakedb/gosnowflake | |||
|
|||
go 1.21 | |||
|
|||
replace github.com/99designs/keyring => github.com/Jeffail/keyring v1.2.3 |
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 using replace? It was required in hashicorp, because it was a transitive dependency for them (their direct dependency was gosnowflake). For us it is a direct dependency, so we can use it directly.
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.
hmm did not consider it but you're right, lets fix 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.
as we discussed: since the fork declares itself as 99design/keyring, cannot import it directly due to
go: github.com/snowflakedb/gosnowflake imports
github.com/99designs/keyring: github.com/Jeffail/[email protected]: parsing go.mod:
module declares its path as: github.com/99designs/keyring
but was required as: github.com/Jeffail/keyring
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.
do we have Jeffail/keyring library security approval?
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.
it's exactly the same as 99designs/keyring , just two modules completely deleted from the original. No other changes.
If you suspect we'll still require security approval for using an already approved module with less functionality, Slack me please with the procedure
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.
Risk Assessment PR created: 6390
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.
Is this not going to be a problem -
i've checked what differs in between those library versions @sfc-gh-pfus
99designs/keyring@master...Jeffail:keyring:master
and it says in the readme:
"Unfortunately, any modules that import your module will also need to add a replace directive of their own."
so not sure if this change won't be a BCR for GO driver users
cc @sfc-gh-pfus @sfc-gh-dszmolka
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1296 +/- ##
=======================================
Coverage 82.20% 82.20%
=======================================
Files 55 55
Lines 13636 13636
=======================================
Hits 11209 11209
Misses 2427 2427 ☔ View full report in Codecov by Sentry. |
internal jira updates with manual test details. marking this ready for review. |
during RA review , turns out this intermediary workaround doesn't help if a package depends on gosnowflake and doesn't do the replace by itself, which somewhat beats the purpose of the PR. |
#1183 for the full story
Description
99designs/keyring
has a long outstanding bug (99designs/keyring#103) which on certain Linux distros, results in runaway dbus-daemon processes. It seems to originate fromkwallet/secretservice
modules of the said library, which we in gosnowflake do not even use; we use a JSON file for temporal credential storage on Linux.Yet, suffering from the issue originating from the bug, because the modules are of course initialized (= bug happens) even when not used.
A long term and nice solution will be entirely replacing
99design/keyring
with a different implementation, but given the priorities so far and the foreseeable near future, it's not realistic to expect it will happen very soon.Thus: workaround; use a fork of
keyring
as many other people to, which simply nukeskwallet
andsecretservice
where the bug is coming from. 👌Checklist