-
Notifications
You must be signed in to change notification settings - Fork 11
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
Delete secrets files when retrieval fails #447
Conversation
b519ffe
to
fc3dc3d
Compare
75cf39e
to
436b1ac
Compare
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.
Changes look good! Thanks for all of the gratuitous, style-related cleanup, and for adding in the Namespace resource dumps on E2E test failure.
deploy/test/test_in_docker.sh
Outdated
@@ -5,6 +5,9 @@ set -xeuo pipefail | |||
|
|||
# Clean up when script completes and fails | |||
finish() { | |||
dump_conjur_namespace_upon_error |
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.
Thank you for putting this in!!!!
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 ended up removing it because I was getting "kubectl: command not found" errors... Not sure what that's about but I don't want it to hold this up.
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.
Ah, yeah, the 'kubectl' commands would need to be run via a test client container.
a1f59e3
to
2a890a2
Compare
578f0cd
to
3a0e82d
Compare
3a0e82d
to
231ac94
Compare
Code Climate has analyzed commit 231ac94 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 93.9% (50% is the threshold). This pull request will bring the total coverage in the repository to 90.0% (-3.7% change). View more on Code Climate. |
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.
LGTM!!!
Desired Outcome
Delete secret file if Conjur returns a 403 or 404 status code.
Implemented Changes
When secrets rotation is enabled, if a batch fetch call to Conjur fails during a refresh due to a deleted secret or revoked access, delete the secrets files from the shared volume to avoid leaving unauthorized secrets on the file system. Currently there's no support for specifying which secret(s) failed, so we need to delete all secrets included in the fetch, which includes all secret groups. A future effort will improve this.
This feature can be disabled using an annotation,
conjur.org/remove-deleted-secrets-enabled: "false"
.Connected Issue/Story
CyberArk internal issue link: ONYX-17938
Definition of Done
At least 1 todo must be completed in the sections below for the PR to be
merged.
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
README
s) were updated in this PRBehavior
Security