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

Add option from datakey to be provided from existing secret #183

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dan-transmit
Copy link

@dan-transmit dan-transmit commented Jul 11, 2023

Desired Outcome

The secret doesn't show in the helm values

Implemented Changes

Allow to use existing secrets in the cluster

Connected Issue/Story

Resolves #[relevant GitHub issue(s), e.g. 76]

CyberArk internal issue ID: [insert issue ID]

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

@dan-transmit dan-transmit requested a review from a team as a code owner July 11, 2023 17:26
Copy link
Contributor

@szh szh left a comment

Choose a reason for hiding this comment

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

Hi @dan-transmit,

Thank you so much for your PR! I took an initial review pass and it looks great overall. I left a couple of comments for you to address, and we'll try to get this fully reviewed in the next few days.

Thank you!

@@ -7,7 +7,7 @@
# rather than setting these in a custom values YAML file. This avoids the
# risk of leaving around residual values files containing this sensitive
# information.

dataKeySecretRef:
Copy link
Contributor

Choose a reason for hiding this comment

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

The linter is failing here. I think this needs to default to an empty string instead of null.

Comment on lines 3 to 5
"required": [
"dataKey"
],
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to require exactly one of [dataKey, dataKeySecretRef]. This can be accomplished with syntax like this:

"oneOf": [
{
  "required": [
    "dataKey"
  ]
},
{
  "required": [
    "dataKeySecretRef"
   ]
},

See the JSON Schema reference for more details.

@szh szh self-assigned this Jul 11, 2023
@@ -7,6 +7,7 @@
# rather than setting these in a custom values YAML file. This avoids the
# risk of leaving around residual values files containing this sensitive
# information.
dataKeySecretRef: "conjur-data-key"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should default to an empty string. That way if it's not explicitly provided it will fail validation. With this default, I think it would succeed initially and then fail when it couldn't find this secret, which would lead to a confusing error message.

Copy link
Author

Choose a reason for hiding this comment

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

Currently, it fails because two of the values are provided, so I'll just remove it altogether

Copy link
Contributor

@szh szh left a comment

Choose a reason for hiding this comment

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

This is looking amazing. Do you time to add a test case to test.sh?

@dan-transmit
Copy link
Author

Writing a test would mean to create a whole new setup (it's not like the current script has a reusable interface).
If you think that dataKeySecretRef should be the preferred usage, then I can replace dataKey from the tests with it.

@szh
Copy link
Contributor

szh commented Jul 12, 2023

That's true. Ideally we'd refactor the tests to make it easier to have multiple test cases. If this isn't something you're interested in doing I totally understand, we just can't merge it without tests. I'm fine to put this on hold for a bit until we can do that internally if that's ok with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants