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: allow project scoped generic kubernetes secrets #2975

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

Conversation

Marvin9
Copy link
Contributor

@Marvin9 Marvin9 commented Nov 20, 2024

This PR now allows create generic k8s secrets

TODO:

  • API - Update secret
  • Tests

Screenshot 2024-11-20 at 6 51 09 PM (2)
Screenshot 2024-11-20 at 6 55 23 PM (2)
Screenshot 2024-11-20 at 6 55 56 PM (2)

Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for docs-kargo-io ready!

Name Link
🔨 Latest commit 67c2047
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/67630ca8968b200008aebb3c
😎 Deploy Preview https://deploy-preview-2975.docs.kargo.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.

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 56.25000% with 70 lines in your changes missing coverage. Please review.

Project coverage is 51.48%. Comparing base (b75ef48) to head (67c2047).

Files with missing lines Patch % Lines
internal/api/update_credentials_v1alpha1.go 25.80% 45 Missing and 1 partial ⚠️
internal/api/create_credentials_v1alpha1.go 75.51% 16 Missing and 8 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2975      +/-   ##
==========================================
+ Coverage   51.27%   51.48%   +0.20%     
==========================================
  Files         285      285              
  Lines       25706    25833     +127     
==========================================
+ Hits        13182    13301     +119     
+ Misses      11824    11814      -10     
- Partials      700      718      +18     

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

@Marvin9
Copy link
Contributor Author

Marvin9 commented Nov 20, 2024

@krancour generic secret creation and listing those is ready. Till I figure out what to do for updates could you start looking at the backend code?

Also do you think its better to re-use the create resource API instead of modifying the create credentials?

@Marvin9 Marvin9 self-assigned this Nov 20, 2024
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9 Marvin9 marked this pull request as ready for review November 21, 2024 19:43
@Marvin9 Marvin9 requested a review from a team as a code owner November 21, 2024 19:43
@Marvin9 Marvin9 requested a review from hiddeco November 21, 2024 19:43
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
Signed-off-by: Mayursinh Sarvaiya <marvinduff97@gmail.com>
@Marvin9
Copy link
Contributor Author

Marvin9 commented Dec 5, 2024

@krancour could you start looking.. I will bring code coverage to threshold

@krancour
Copy link
Member

Sorry this took me so long.

Overall, this looks fantastic. Test drive went really smoothly.

I have some small bits of feedback, that I think are easy to address.

Screenshot 2024-12-11 at 4 20 35 PM

In the screenshot above, I was initially thrown off by not seeing any kind of "Add Secret" button. It took me a while to realize that clicking "Add Credentials" would pop up a dialog that could be used for adding generic Secrets as well.

I think this could potentially be less confusing if each of the two sections of this page each got their own "Add Credentials" and "Add Secret" buttons.

In the same screen shot, upper right, the alt text for the tab says "Credentials". With Secrets being more broad/generic than credentials, I would suggest making the alt text there "Secrets." Credentials are a kind of Secret. (If it's not a problem, I think the URL of the page could also change to end in "/secrets".

Screenshot 2024-12-11 at 4 25 17 PM

Two things tripped me up in the screenshot above.

The title of the dialog is "Edit Credentials" even when I'm working on a generic Secret. Could it be at all practical to have two separate dialogs? One for creds and one for generic?

When editing an existing Secret, not seeing any ****** in the value fields left me wondering whether I was going to accidentally blank out those fields when I hit Update. I confirmed that this is not a problem, but seeing ****** in those fields would have stopped me from wondering.

Last -- a weird one. If I try to add a new key "foobar" above, the new line disappears the moment I hit the second "o" in "foobar". I assume there's some real time validation that keys aren't being duplicated and after hitting that second "o", it looks like I am duplicating "foo" although I wasn't done typing. If I type any other value not staring with the characters of an existing key, everything works fine.

I think/hope these are all relatively minor tweaks. As I said, overall, this worked great and did exactly what it's supposed to.

@Marvin9
Copy link
Contributor Author

Marvin9 commented Dec 17, 2024

Great suggestions! @krancour you might want to take a look now, except your last point (in progress) all other review points are addressed. Thanks

@Marvin9
Copy link
Contributor Author

Marvin9 commented Dec 19, 2024

@krancour it is ready to review again

Credentials: kubernetesSecret,
},
), nil
case kargoapi.CredentialTypeLabelValueGeneric:
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking a Secret that isn't labeled as a generic credential should just automatically be treated as such. i.e. Perhaps generic credentials is the default case?

The thing I think I want to avoid is Kargo insisting on a special label for Secret that aren't really special in any way. (The "specific" creds are special, so that doesn't bother me.)

Copy link
Member

Choose a reason for hiding this comment

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

Or... I bet that label was basically necessary to make the existing credential-management endpoints work for generic Secrets? This actually has got me thinking about whether separate endpoints may be in order for Secrets since credentials are special in many ways that generic Secrets aren't.

For generic Secrets, I think we need the following:

  • Get/list endpoints that redact all values and annotations. (Compare to creds, which exempts the values of certain keys from being redacted.)

    Under the hood, credential and generic secret endpoints can share a common redaction func and pass the names of exempt keys.

  • Update endpoint that updates/deletes individual keys. The generic update resource endpoint cannot be used for this purpose. Since we redact a lot on the get, the client doesn't have everything it needs to send back for a full update.

    This is another bit that seems it could be shared between cred update and generic secret update endpoints.

  • Delete endpoint.

  • Create can use the generic create resource endpoint since I don't see any special treatment being required.

Copy link
Contributor Author

@Marvin9 Marvin9 Jan 2, 2025

Choose a reason for hiding this comment

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

The thing I think I want to avoid is Kargo insisting on a special label for Secret that aren't really special in any way. (The "specific" creds are special, so that doesn't bother me.)

Shading some lights on previous comments which seems in favour of explicit labels? Lets agree on either of way and I can update APIs accordingly @krancour

@krancour
Copy link
Member

@Marvin9 overall, this is great. I had some suggestions about the back end parts of this. In a nutshell, the existing credential management endpoints do some very specific things that I don't think apply to generic Secrets.

I'm happy to help on the new endpoints I suggested if you want me to. Just lmk.

@krancour
Copy link
Member

Sorry... one last thing. Can we make this dialog say "Create Credentials" (but leave the other dialog as "Create Secrets")?

Screenshot 2024-12-23 at 4 22 27 PM

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.

4 participants