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(snap): make secretstore connect hook idempotent #3815

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

farshidtz
Copy link
Member

Fixes #3814 by copying the tokens instead of moving them away from the origin.

In addition, it improves logging:

  • The list of directories are now fully printed (not just the first line)
  • Failed connect operation returns an error (when token not present because edgex-go doesn't provide it or someone has remove it)

Signed-off-by: Farshid Tavakolizadeh [email protected]

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-go/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

  • I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
  • I am not introducing a new dependency (add notes below if you are)
  • I have added unit tests for the new feature or bug fix (if not, why?)
  • I have fully tested (add details below) this the new feature or bug fix (if not, why?)
  • I have opened a PR for the related docs change (if not, why?)

Testing Instructions

See #3814 to check how it used to work.

Using a locally-build edgexfoundry, so the plug doesn't connect automatically.

# install device-camera
$ sudo snap install edgex-device-camera --edge
edgex-device-camera (edge) 2.0.2-dev.4 from Canonical✓ installed
# connect the plug
$ sudo snap connect edgexfoundry:edgex-secretstore-token edgex-device-camera:edgex-secretstore-token
# check that token is copied
$ sudo ls /var/snap/edgex-device-camera/current/device-camera/secrets-token.json
/var/snap/edgex-device-camera/current/device-camera/secrets-token.json
# remove device-camera
$ sudo snap remove edgex-device-camera
edgex-device-camera removed

# install device-camera again
$ sudo snap install edgex-device-camera --edge
edgex-device-camera (edge) 2.0.2-dev.4 from Canonical✓ installed
# connect the plug again
$ sudo snap connect edgexfoundry:edgex-secretstore-token edgex-device-camera:edgex-secretstore-token
# check that token is copied
$ sudo ls /var/snap/edgex-device-camera/current/device-camera/secrets-token.json
/var/snap/edgex-device-camera/current/device-camera/secrets-token.json

Test error handling:

# disconnect the plug
$ sudo snap disconnect edgexfoundry:edgex-secretstore-token edgex-device-camera:edgex-secretstore-token
# remove the token manually
$ sudo rm -v /var/snap/edgexfoundry/current/secrets/device-camera/secrets-token.json
removed '/var/snap/edgexfoundry/current/secrets/device-camera/secrets-token.json'
# connect the plug
$ sudo snap connect edgexfoundry:edgex-secretstore-token edgex-device-camera:edgex-secretstore-token
error: cannot perform the following tasks:
- Run hook prepare-plug-edgex-secretstore-token of snap "edgexfoundry" (run hook "prepare-plug-edgex-secretstore-token": <13>Nov 12 19:54:21 root: edgex-secretstore-token: could not find token for device-camera)

Look at the journals:

$ journalctl -n 100 -o cat | grep edgex-secretstore-token:
edgex-secretstore-token: prepare SLOTDIRS=[ "$SNAP_DATA/device-camera" ]
edgex-secretstore-token: prepare device-camera
edgex-secretstore-token: copying /var/snap/edgexfoundry/x1/secrets/device-camera/secrets-token.json to /tmp/device-camera-secrets-token.json
edgex-secretstore-token: connect SLOTDIRS=[ "$SNAP_DATA/device-camera" ]
edgex-secretstore-token: connect device-camera
edgex-secretstore-token: moving /tmp/device-camera-secrets-token.json to /var/snap/edgexfoundry/x1/secrets/device-camera/secrets-token.json
...
edgex-secretstore-token: prepare SLOTDIRS=[ "$SNAP_DATA/device-camera" ]
edgex-secretstore-token: prepare device-camera
edgex-secretstore-token: could not find token for device-camera

New Dependency Instructions (If applicable)

@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2021

Codecov Report

Merging #3815 (3a8ccbc) into main (31a468f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3815   +/-   ##
=======================================
  Coverage   45.77%   45.77%           
=======================================
  Files         112      112           
  Lines        9640     9640           
=======================================
  Hits         4413     4413           
  Misses       4844     4844           
  Partials      383      383           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31a468f...3a8ccbc. Read the comment docs.

Copy link
Member

@tonyespy tonyespy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a "Fixes: #3814" to the body of your commit message?

@farshidtz farshidtz force-pushed the snap-token-hook-idempotent branch from 32e516a to 4930e0c Compare November 13, 2021 12:06
@farshidtz farshidtz force-pushed the snap-token-hook-idempotent branch from 4930e0c to 99d04a8 Compare November 13, 2021 12:07
@farshidtz farshidtz requested a review from tonyespy November 13, 2021 12:07
@farshidtz
Copy link
Member Author

Can you please add a "Fixes: #3814" to the body of your commit message?

Done. I amended the commit message.

@siggiskulason siggiskulason self-requested a review November 16, 2021 09:04
Copy link

@siggiskulason siggiskulason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Testing:

  • Followed your test instructions successfully
  • Ran TAF tests: OK: 17 FAIL/585 PASS, which is the current benchmark

Code:

  • looks fine. I like using "--stderr" with logger to get the output to stderr as well

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@siggiskulason siggiskulason dismissed tonyespy’s stale review November 16, 2021 09:56

Requested change has been implemented by Farshid

@siggiskulason siggiskulason merged commit 387e8ab into edgexfoundry:main Nov 16, 2021
@farshidtz farshidtz deleted the snap-token-hook-idempotent branch November 16, 2021 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snap connect edgex-secretstore-token not idempotent
4 participants