[SM-1147] Switch to try_init with pyo3_log #676

coltonhurst · 2024-03-21T19:46:42Z

Type of change

Objective

When running the ansible-playbook command, we needed to export the following variable: export OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES. This suppresses fork safety warnings on Mac OS. With this set, you can safely query for individual secrets. However, if you query for secrets within an Ansible loop or with a Jinja2 template, we receive an error that is caused by setting up a logger more than once. (This error comes from the pyo3_log crate.)

If we switch to using the pyo3_log::try_init function, we ignore the case where a logger is already set up if the attempt fails. This happens because the pyo3_log::init function can panic. This also introduces panic safety.

Code changes

client.rs: Switch to try_init

Before you submit

Please add unit tests where it makes sense to do so

…t occurs.

codecov · 2024-03-21T19:58:31Z

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 60.59%. Comparing base (67e743f) to head (4c5b926).

Files	Patch %	Lines
crates/bitwarden-py/src/client.rs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #676      +/-   ##
==========================================
- Coverage   60.61%   60.59%   -0.02%     
==========================================
  Files         170      170              
  Lines       10387    10390       +3     
==========================================
  Hits         6296     6296              
- Misses       4091     4094       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dani-garcia

LGTM, we're already doing the same thing for the c and napi bindings, seems like we forgot this one.

https://github.com/bitwarden/sdk/blob/67e743f595e73df9807b6f17f557e327ac0fbc5a/crates/bitwarden-c/src/c.rs#L27

https://github.com/bitwarden/sdk/blob/67e743f595e73df9807b6f17f557e327ac0fbc5a/crates/bitwarden-napi/src/client.rs#L34-L36

Thomas-Avery

LGTM

coltonhurst · 2024-03-22T15:56:12Z

Added a comment and let for consistency across crates in 4c5b926.

mittkkoo · 2024-04-23T17:32:48Z

I'm sorry for the dumb question, but how to implement this fix on my side? I'm using Bitwarden Secrets with the ansible lookup plugin. I did cargo build in crates/bws as in the newest state of the main branch and then cargo install --path ., but what next? The lookup plugin still fails with a panic error as before.

jacobfelknor · 2024-04-24T15:26:05Z

@mittkkoo, I'm facing the same issue as you with the Ansible integration.

If you followed the directions for setup in the Bitwarden Documentation, I believe it doesn't use the bws cli you installed/built with cargo (maintainer, please correct me if I'm wrong), but instead only uses the bitwarden-sdk PyPI package.

I think we must wait until a new version of bitwarden-sdk is published to PyPI

jacobfelknor · 2024-06-05T22:35:42Z

@coltonhurst

Tagging you because you're the PR author and I'm not sure who else to ask, but feel free to delegate. This still appears to be unresolved for users following the official documentation I linked in my above reply for the same reason - no new PyPI package pushed.

When can the community expect this to be resolved (or how can I help us get there)?

coltonhurst · 2024-06-06T00:36:16Z

@mittkkoo & @jacobfelknor thanks for asking & reaching out, sorry for the delay on this. We have an internal ticket on deck to fix the Python issue, we will be investigating soon!

jacobfelknor · 2024-06-06T02:46:21Z

Great, thank you!

betermieux · 2024-07-08T07:46:17Z

@mittkkoo & @jacobfelknor thanks for asking & reaching out, sorry for the delay on this. We have an internal ticket on deck to fix the Python issue, we will be investigating soon!

Any news on this? The ansible plugin is not usable if you use more then one secret lookup…

felixklauke · 2024-08-04T15:05:46Z

@coltonhurst Would be great of you could give us an ETA for a new release.

@betermieux In the meantime, I created a dirty little fix for this:

- name: Deploy and configure Prometheus
  hosts: service_APP001_Prometheus
  become: true
  pre_tasks:
    - set_fact:
        netbox_api_url: "{{ lookup('bitwarden.secrets.lookup', 'xxx' )}}"
    - set_fact:
        netbox_api_token: "{{ lookup('bitwarden.secrets.lookup', 'xxx' )}}"
  roles:
    - prometheus.prometheus.prometheus

TL;DR: Put all lookups in it's own (!) set_fact task for now, until we have a release.

betermieux · 2024-08-04T18:29:38Z

@betermieux In the meantime, I created a dirty little fix for this…

Sorry for not replying earlier, I commented my findings at bitwardens forum:
Bitwarden lookup plugin for Ansible causes panick errors and “A worker was found in a dead state”

It basically works if you build a current version of this SDK and replace the packages in your ansible installation. Now someone just needs to publish a current version to the pip repository.

## 🎟️ Tracking  https://bitwarden.atlassian.net/browse/SM-1384 ## 📔 Objective  When creating multiple WASM clients, a panic occurs `failed to initialize logger: SetLoggerError())`. Looks to be the same thing we fixed: #181 #676 ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines  - 👍 (`:+1:`) or similar for great changes - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info - ❓ (`:question:`) for questions - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (`:art:`) for suggestions / improvements - ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or concerns needing attention - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt - ⛏ (`:pick:`) for minor or nitpick changes

SM-1147: Try to init logger and fail safely rather than the panic tha…

c766cea

…t occurs.

coltonhurst self-assigned this Mar 21, 2024

coltonhurst requested review from Hinton, dani-garcia and a team March 21, 2024 19:51

dani-garcia previously approved these changes Mar 22, 2024

View reviewed changes

Thomas-Avery previously approved these changes Mar 22, 2024

View reviewed changes

SM-1147: Add comment for consistency across crates

4c5b926

coltonhurst dismissed stale reviews from Thomas-Avery and dani-garcia via 4c5b926 March 22, 2024 15:55

coltonhurst requested review from dani-garcia and Thomas-Avery March 22, 2024 15:55

dani-garcia approved these changes Mar 22, 2024

View reviewed changes

tangowithfoxtrot approved these changes Mar 22, 2024

View reviewed changes

Thomas-Avery approved these changes Mar 22, 2024

View reviewed changes

Hinton mentioned this pull request Apr 9, 2024

PanicExecption in python sdk #697

Closed

1 task

djsmith85 linked an issue Apr 9, 2024 that may be closed by this pull request

PanicExecption in python sdk #697

Closed

1 task

coltonhurst merged commit b4205cc into main Apr 11, 2024
67 checks passed

coltonhurst deleted the sm/sm-1147 branch April 11, 2024 13:59

coltonhurst changed the title ~~SM-1147: Switch to try_init with pyo3_log~~ [SM-1147] Switch to try_init with pyo3_log Apr 11, 2024

Thomas-Avery mentioned this pull request Aug 2, 2024

[SM-1384] Fix panic on re-registering logger | WASM #935

Merged

dani-garcia mentioned this pull request Aug 20, 2024

ansible jinja2 template with more than one secret crashes #985

Closed

1 task

Hinton mentioned this pull request Aug 26, 2024

Python problem when creating multiple BitwardenClients #994

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SM-1147] Switch to try_init with pyo3_log #676

[SM-1147] Switch to try_init with pyo3_log #676

coltonhurst commented Mar 21, 2024

codecov bot commented Mar 21, 2024 •

edited

Loading

dani-garcia left a comment

Thomas-Avery left a comment

coltonhurst commented Mar 22, 2024

mittkkoo commented Apr 23, 2024 •

edited

Loading

jacobfelknor commented Apr 24, 2024 •

edited

Loading

jacobfelknor commented Jun 5, 2024 •

edited

Loading

coltonhurst commented Jun 6, 2024

jacobfelknor commented Jun 6, 2024

betermieux commented Jul 8, 2024

felixklauke commented Aug 4, 2024 •

edited

Loading

betermieux commented Aug 4, 2024

[SM-1147] Switch to try_init with pyo3_log #676

[SM-1147] Switch to try_init with pyo3_log #676

Conversation

coltonhurst commented Mar 21, 2024

Type of change

Objective

Code changes

Before you submit

codecov bot commented Mar 21, 2024 • edited Loading

Codecov Report

dani-garcia left a comment

Choose a reason for hiding this comment

Thomas-Avery left a comment

Choose a reason for hiding this comment

coltonhurst commented Mar 22, 2024

mittkkoo commented Apr 23, 2024 • edited Loading

jacobfelknor commented Apr 24, 2024 • edited Loading

jacobfelknor commented Jun 5, 2024 • edited Loading

coltonhurst commented Jun 6, 2024

jacobfelknor commented Jun 6, 2024

betermieux commented Jul 8, 2024

felixklauke commented Aug 4, 2024 • edited Loading

betermieux commented Aug 4, 2024

codecov bot commented Mar 21, 2024 •

edited

Loading

mittkkoo commented Apr 23, 2024 •

edited

Loading

jacobfelknor commented Apr 24, 2024 •

edited

Loading

jacobfelknor commented Jun 5, 2024 •

edited

Loading

felixklauke commented Aug 4, 2024 •

edited

Loading