Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Support content-type annotation #511

Merged
merged 1 commit into from
Mar 28, 2023
Merged

Support content-type annotation #511

merged 1 commit into from
Mar 28, 2023

Conversation

rpothier
Copy link
Contributor

@rpothier rpothier commented Mar 23, 2023 •
edited
Loading

Desired Outcome

Update push to file to support content-type annotation to decode base 64.

Implemented Changes

Added the new content-type annotations.

Connected Issue/Story

CyberArk internal issue ID: CNJR-813

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: [insert issue ID]
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

@rpothier rpothier self-assigned this Mar 23, 2023
@rpothier rpothier force-pushed the update-content-type-for-ptf branch 4 times, most recently from 3f3bec0 to 8dae52f Compare March 27, 2023 18:46
@rpothier rpothier marked this pull request as ready for review March 27, 2023 18:47
@rpothier rpothier requested a review from a team as a code owner March 27, 2023 18:47
@rpothier rpothier force-pushed the update-content-type-for-ptf branch from 8dae52f to b1eed30 Compare March 27, 2023 19:27
gl-johnson
gl-johnson reviewed Mar 27, 2023
View reviewed changes
pkg/secrets/pushtofile/retrieve_secrets.go Outdated
sValue, err = base64.StdEncoding.DecodeString(string(sValue))
if err != nil {
err = fmt.Errorf("failed to decode secret. Error: %v", err)
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

In the K8s secrets implementation we are logging the decoding error as a warning here, but then still providing the original secret value. I think this makes it a bit more resilient if there is a change in the secret value to plaintext where it won't immediately break here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will change.

pkg/secrets/pushtofile/secret_spec.go
t.ContentType = v
} else {
count = count + 1
if count > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think any unit tests trigger this condition (too many mapping entries)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test TestNewSecretSpecs/secret_spec_map_with_multiple_keys triggers this condition.

pkg/secrets/pushtofile/secret_spec.go Outdated
if content == "text" || content == "base64" {
return nil
} else {
return fmt.Errorf("secret group %s: the content-type of %s is invalid",
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhat similar to the other comment - in the K8s secrets implementation, if a user provides an invalid content-type we log a warning and assume it should be treated as text so that it doesn't fail

Happy to discuss which outcome is more intuitive for the user, but either way we should aim to be consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it should be consistent. I'll change it to a warning and leave in the base code and UT so we can
switch back in the future if we want to.

@rpothier rpothier force-pushed the update-content-type-for-ptf branch from b1eed30 to 3f6f4f5 Compare March 28, 2023 18:58
@rpothier rpothier force-pushed the update-content-type-for-ptf branch from 3f6f4f5 to bf6d37c Compare March 28, 2023 19:53
@codeclimate
Copy link

codeclimate bot commented Mar 28, 2023

Code Climate has analyzed commit bf6d37c and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 89.3% (50% is the threshold).

This pull request will bring the total coverage in the repository to 89.2% (0.0% change).

View more on Code Climate.

gl-johnson
gl-johnson approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@gl-johnson gl-johnson left a comment

Choose a reason for hiding this comment

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

LGTM

@rpothier rpothier merged commit c478db9 into main Mar 28, 2023
@rpothier rpothier deleted the update-content-type-for-ptf branch March 28, 2023 21:04
@john-odonnell john-odonnell mentioned this pull request Apr 21, 2023
Merged
13 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@gl-johnson gl-johnson gl-johnson approved these changes

Assignees

@rpothier rpothier

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rpothier @gl-johnson