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

feat: implement conditional secret management #2517

Merged
merged 9 commits into from
Sep 11, 2024

Conversation

fykaa
Copy link
Contributor

@fykaa fykaa commented Sep 9, 2024

Fixes: #2214

Key changes include:

  1. Configuration updates:

    • EnableSecretManagement flag
    • modified ClusterRole and ConfigMap to manage secrets based on this flag
    • updated values.yaml and config.go to incorporate the new setting
  2. API enhancements:

    • updated GetConfig endpoint to include SecretManagementEnabled status
  3. Endpoint adjustments:

    • ensure endpoints for managing secrets return connect.CodeUnimplemented when secret management is disabled

cc: @krancour @rbreeze

Signed-off-by: Faeka Ansari <faeka6@gmail.com>
…nimplemented

Signed-off-by: Faeka Ansari <faeka6@gmail.com>
@fykaa fykaa self-assigned this Sep 9, 2024
@fykaa fykaa requested a review from a team as a code owner September 9, 2024 19:20
Copy link

netlify bot commented Sep 9, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit 4225454
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/66e1c5350aa62d00084cc4bb
😎 Deploy Preview https://deploy-preview-2517.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: Faeka Ansari <faeka6@gmail.com>
charts/kargo/templates/api/configmap.yaml Show resolved Hide resolved
@@ -18,6 +18,9 @@ data:
TLS_CERT_PATH: /etc/kargo/tls.crt
TLS_KEY_PATH: /etc/kargo/tls.key
{{- end }}
{{- if .Values.api.enableSecretManagement }}
Copy link
Contributor

@hiddeco hiddeco Sep 9, 2024

Choose a reason for hiding this comment

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

Suggested change
{{- if .Values.api.enableSecretManagement }}
{{- if .Values.api.secretManagement.enabled }}

maybe? To match .api.oidc.enabled, .api.adminAccount.enabled, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

enableSecretManagement matches the enablePermissiveCORSPolicy imo; which should be good as long as we have not more than one field to be managed for secret management unlike oidc and adminAccount

wdyt? @hiddeco

Copy link
Contributor

Choose a reason for hiding this comment

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

I now notice there is a .api.rollouts.integrationEnabled, so I guess we have already lost this game.

I would prefer things to be uniform at some point, but will not let this block you now. So go for whatever you think is best.

Copy link
Member

Choose a reason for hiding this comment

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

Let's make an issue for improving the consistency pre-GA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@krancour
Copy link
Member

krancour commented Sep 9, 2024

It looks like a few unit tests that use the credential management endpoints may need an update. Their config needs secret management enabled.

Signed-off-by: Faeka Ansari <faeka6@gmail.com>
Copy link

codecov bot commented Sep 9, 2024

Codecov Report

Attention: Patch coverage is 8.69565% with 21 lines in your changes missing coverage. Please review.

Project coverage is 48.26%. Comparing base (a64eb0e) to head (4225454).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/api/delete_credentials_v1alpha1.go 0.00% 5 Missing ⚠️
internal/api/get_credentials_v1alpha1.go 0.00% 4 Missing and 1 partial ⚠️
internal/api/list_credentials_v1alpha1.go 0.00% 5 Missing ⚠️
internal/api/update_credentials_v1alpha1.go 0.00% 5 Missing ⚠️
internal/api/config/config.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2517      +/-   ##
==========================================
- Coverage   48.32%   48.26%   -0.06%     
==========================================
  Files         254      254              
  Lines       18133    18155      +22     
==========================================
+ Hits         8762     8763       +1     
- Misses       8889     8909      +20     
- Partials      482      483       +1     

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

Signed-off-by: Faeka Ansari <faeka6@gmail.com>
@krancour
Copy link
Member

@rbreeze any chance we can get you to amend this PR with the bits to hide credential-management functionality when the config endpoint says its not enabled?

Signed-off-by: Remington Breeze <remington@breeze.software>
@rbreeze
Copy link
Contributor

rbreeze commented Sep 10, 2024

@fykaa @krancour Sure thing, just pushed the update 👍🏻

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
@krancour
Copy link
Member

Added a tiny fix in 92f07be

charts/kargo/values.yaml Outdated Show resolved Hide resolved
Signed-off-by: Faeka Ansari <faeka6@gmail.com>
@krancour
Copy link
Member

Thank you @fykaa! This is great!

@krancour
Copy link
Member

And @rbreeze, thanks to you as well!

@fykaa fykaa requested a review from hiddeco September 10, 2024 22:39
Signed-off-by: Faeka Ansari <faeka6@gmail.com>
@krancour krancour added this pull request to the merge queue Sep 11, 2024
Merged via the queue into akuity:main with commit c561668 Sep 11, 2024
15 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: option for api server not to touch credentials (secrets) at all
4 participants