k6-operator icon indicating copy to clipboard operation
k6-operator copied to clipboard

fix: Enforce ReadOnly mode at VolumeMount level instead of Volume level

Open The-Flash-Routine opened this issue 6 months ago • 7 comments

  • Moves readOnly flag from Volume to VolumeMount in Script struct to ensure proper per-mount access control
  • Maintains backward compatibility while fixing PVC write access
  • ConfigMap mounts remain forced read-only (security best practice)

Why:

  • Volume-level readOnly affected all mounts globally (undesirable for multi-container pods)
  • VolumeMount-level is the standard Kubernetes pattern for granular control

Testing:

  • Tested by setting up a minikube cluster on local and creating a new operator image & using TestRun with a sample script
  • Attaching a 5 minute video showing existing functionality + building & deploying new operator image + updated functionality
  • Google Drive Video Link: https://photos.app.goo.gl/qF2KiBHnUH51Z4cW7

The-Flash-Routine avatar May 18 '25 19:05 The-Flash-Routine

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 18 '25 19:05 CLAassistant

Hi @yorugac can you please enable workflows to run on my PR

The-Flash-Routine avatar May 20 '25 20:05 The-Flash-Routine

Hi @The-Flash-Routine, thanks for the PR but there is no issue for this one. Could you please clarify the use case you're trying to fix or improve? By the PR, it seems like you wish to change the behaviour of .spec.script.volumeClaim.readOnly: this might be a breaking change for some. Some elaboration on the use case would be nice :slightly_smiling_face:

yorugac avatar May 26 '25 09:05 yorugac

Hi @yorugac Thanks for having a look at my PR. Yes, no issue exists for this one. Whilst I was working with k6-operator, I identified this improvement.

Context: I have a use case of initContainer as part of TestRun resource, which is needed as part of Runner job.

UseCase: The current method puts the "readOnly=false" flag at "volume" level. When I mount the same Volume/PVC to initContainer as part of TestRun resource, the initContainer also can write in the PVC. I want:

  1. the Runner's main container can write to Volume/PVC
  2. The initContainer should not be able to write

This PR change: Introduces more granular controls to set "readOnly=false" at Container/VolumeMount level, instead of Volume/PVC level.

Regarding the discussion about breaking-change: I doubt it would be breaking change for people, as the change will affect new Jobs which are created when TestRun object is created on Kubernetes cluster. So any existing running jobs (if they are still running while the k6-operator is being updated) will not get affected. Also the jobs are marked as completed at the end of test.

The-Flash-Routine avatar Jun 01 '25 18:06 The-Flash-Routine

Also @yorugac there is this short 5 minute demo video attached within this PR https://photos.app.goo.gl/qF2KiBHnUH51Z4cW7

This would help to see the change live working on my local

The-Flash-Routine avatar Jun 01 '25 18:06 The-Flash-Routine

@yorugac I see that one of the workflow stages failed due to some intermittent Github connectivity issue. Can you please trigger those again

The-Flash-Routine avatar Jun 01 '25 18:06 The-Flash-Routine

@The-Flash-Routine, thanks for the description. I've restarted the CI checks: they seem to be fine.

The current method puts the "readOnly=false" flag at "volume" level. When I mount the same Volume/PVC to initContainer as part of TestRun resource, the initContainer also can write in the PVC.

the Runner's main container can write to Volume/PVC The initContainer should not be able to write

That makes sense. OTOH, why shouldn't initContainer write to the volume? I.e., is the reason here to enhance the security stance (close everything that is not strictly necessary) or is there an additional factor? Or IOW, why it's a problem?

I doubt it would be breaking change for people, as the change will affect new Jobs which are created when TestRun object is created on Kubernetes cluster. So any existing running jobs (if they are still running while the k6-operator is being updated) will not get affected.

Breaking change is not regarding specific, ephemeral, k6 Jobs, but regarding any workflows that use existing logic. For example, PrivateLoadZone tests use initContainer to write to the volume, even though it's not a volumeClaim. But I doubt it's the only case. ATM, it looks like this change would require changes in some definitions of the TestRun: that's what I meant by breaking change.

I'll need to investigate and test this change in setting in greater detail. By timeline, that can happen in the next couple of weeks approx., given my current load. Apologies for the delays. Meanwhile, I'd appreciate any additional details about the use case. (like my question above or what is your initContainer doing in the first place :smile:)

yorugac avatar Jun 11 '25 07:06 yorugac

Hi @yorugac Would you be up for a Zoom call to discuss the changes so that we can get them going ?

The-Flash-Routine avatar Jul 16 '25 19:07 The-Flash-Routine

Hi @yorugac

Glad you liked the video :)

Good spot. Thanks ! I've updated the PR to mention that default block is for LocalFile.

Thanks for running tests on my behalf ! It could be an idea for my next PR to add automated tests for volume

The-Flash-Routine avatar Jul 21 '25 19:07 The-Flash-Routine

Thanks a lot @yorugac for your support. I'll definitely start looking at e2e folder to address the volume testing.

The-Flash-Routine avatar Aug 01 '25 19:08 The-Flash-Routine