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

Allow users to reference own public key secrets #18

Merged
merged 6 commits into from
Sep 4, 2023
Merged

Allow users to reference own public key secrets #18

merged 6 commits into from
Sep 4, 2023

Conversation

puffitos
Copy link
Collaborator

@puffitos puffitos commented Sep 1, 2023

Motivation

This should solve #17

Changes

I've extended the secret parsing function to check for the referenced secret (as was the original intention, when reading the docs). The default secret still works as a fallback.

To achieve this, I moved the client to the webhook server itself, to avoid uneccesary creation of multiple k8s clientsets. One is enough to handle the requests we get.

Additionally, I've cleaned up the code base a bit, removed the vendors folder (if there is some reason for it, we can leave it in) and spruced up the documentation.

Tests done

System tests for the following use cases:

  1. Wrong public key present in secret -> no deployment
  2. Wrongly encoded key present in secret -> no deployment
  3. No referenced secret present at all, no env variables, no default namespaced secret -> deployment
  4. Referenced secret key is empty -> deployment (if all other checks pass)

Also added some unit tests to get the ball rolling :)

@puffitos
Copy link
Collaborator Author

puffitos commented Sep 1, 2023

image

OMG Github will hate me for this 🗡️

@mamofejo
Copy link

mamofejo commented Sep 2, 2023

would it be possible to use an annotation instead of an environment variable for the name of the secret? This would not pollute the container environment.

@puffitos
Copy link
Collaborator Author

puffitos commented Sep 2, 2023

This is a interesting suggestion, but it doesn't really fit the scope of this PR. We're correcting the expected behavior here: one should be able to reference an own secret and not only the default one. The feature is suggested by the repo's Readme but isn't actually there. 😉

Still, I do encourage you to open an issue so we may discuss a possible implementation.

@eumel8 eumel8 merged commit 4017bc2 into eumel8:main Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants