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

Create a mutating webhook to mount secrets #857

Merged
merged 16 commits into from
Mar 6, 2023

Conversation

sseidman
Copy link
Contributor

@sseidman sseidman commented Feb 7, 2023

What this PR does:
Creates a webhook that, as a default, creates volumes and volume mounts on created/updated pods for secrets that are specified via a specific annotation.

For example, a pod with the annotation k8ssandra.io/inject-secret=[{"secretName": "cassandraSuperUser", "path": "/etc/credentials/cassandra"}] will create a volume for the cassandraSuperuser secret and mount the secret to the path /etc/credentials/cassandra on each container for the pod. This will mount secrets in the default Kubernetes style, which will create a single file for each key/value pair in the Secret (e.g. there will be username file and password file for credential sets).

Which issue(s) this PR fixes:
Fixes #605

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CHANGELOG.md updated (not required for documentation PRs)
  • CLA Signed: DataStax CLA

Copy link
Member

@Miles-Garnsey Miles-Garnsey left a comment

Choose a reason for hiding this comment

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

I've left a couple of comments already. I have done a manual test and the webhook certainly appears to behave as expected.

There are a couple of additional areas I think we need to think about:

  1. It would be good to have a CLI flag to disable the webhook for users who don't want it turned on.
  2. There are test failures. I think I may have figured out the failures in the integration tests. I am still looking at the failures in the e2e tests. These are all multi-cluster, so I suspect that maybe the webhook isn't being installed on data planes or something, but I haven't explored that path yet.

I'll comment further tomorrow.

controllers/secrets-webhook/secretswebhook.go Outdated Show resolved Hide resolved
controllers/secrets-webhook/secretswebhook.go Show resolved Hide resolved
// mutatePods injects the secret mounting configuration into the pod
func (p *podSecretsInjector) mutatePods(ctx context.Context, pod *corev1.Pod, logger logr.Logger) error {
if pod.Annotations == nil {
logger.Info("no annotations exist")
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to log here (and I think this logging might be a bit too chatty), can we include the pod name and namespace so that any unexpected behaviour can be traced?

// get secret name from injection annotation
secretName := secret.SecretName
mountPath := secret.Path
logger.Info("creating volume and volume mount for secret",
Copy link
Member

Choose a reason for hiding this comment

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

Again, it would be good to include the pod name here.

@Miles-Garnsey
Copy link
Member

Miles-Garnsey commented Feb 22, 2023

@sseidman, nice work tracking down the e2e failures! I've completed my review now. This broadly looks really good, there are only one or two naming, unit testing and refactoring related things I'm going to suggest.

(NB: take note of my comments on why the envtests are failing in case that helps, I think it is an easy fix.)

@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Merging #857 (8ca9d37) into main (9dfb3d9) will increase coverage by 0.66%.
The diff coverage is 65.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #857      +/-   ##
==========================================
+ Coverage   56.41%   57.08%   +0.66%     
==========================================
  Files          96       96              
  Lines        9538     9335     -203     
==========================================
- Hits         5381     5329      -52     
+ Misses       3672     3545     -127     
+ Partials      485      461      -24     
Impacted Files Coverage Δ
apis/k8ssandra/v1alpha1/k8ssandracluster_types.go 36.23% <ø> (ø)
apis/medusa/v1alpha1/medusarestorejob_types.go 100.00% <ø> (ø)
apis/medusa/v1alpha1/medusaschedule_types.go 100.00% <ø> (ø)
...ollers/k8ssandra/cassandra_telemetry_reconciler.go 65.00% <ø> (ø)
...trollers/medusa/medusabackupschedule_controller.go 73.23% <ø> (ø)
controllers/reaper/vector.go 56.32% <0.00%> (+12.95%) ⬆️
controllers/stargate/vector.go 56.32% <0.00%> (+12.95%) ⬆️
controllers/stargate/stargate_controller.go 47.70% <42.85%> (+1.82%) ⬆️
controllers/control/k8ssandratask_controller.go 63.11% <50.00%> (-2.97%) ⬇️
controllers/k8ssandra/medusa_reconciler.go 80.32% <66.66%> (+2.01%) ⬆️
... and 21 more

@sseidman sseidman marked this pull request as ready for review February 23, 2023 18:45
@sseidman sseidman requested a review from a team as a code owner February 23, 2023 18:45
@sseidman sseidman changed the title Sseidman/controller webhook Create a mutating webhook to mount secrets Feb 23, 2023
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
3.7% 3.7% Duplication

@Miles-Garnsey
Copy link
Member

@sseidman, apologies, I totally missed that you'd been pushing some commits over here. I'll follow up with a review tomorrow.

Copy link
Member

@Miles-Garnsey Miles-Garnsey left a comment

Choose a reason for hiding this comment

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

Looks perfect, thanks for addressing all my comments from the previous iterations!

@Miles-Garnsey Miles-Garnsey merged commit 632ede9 into k8ssandra:main Mar 6, 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.

K8SSAND-1623 ⁃ Create a mutating webhook for the internal secrets provider
2 participants