keda icon indicating copy to clipboard operation
keda copied to clipboard

ScaledJob fails to support repeated containerPort with different names in container template

Open zd9KgA opened this issue 1 year ago • 5 comments

Report

In the definition of a ScaledJob.spec.jobTargetRef.template.containers, I'd expect to be able to state something like this:

          ports:
          - containerPort: 8080
            name: metrics
            protocol: TCP
          - containerPort: 8080
            name: probes
            protocol: TCP

That is, to refer to the same physical port by different names (e.g., metrics and probes in the given example). This works for a Pod, and I did not find any restriction why the same PodSpec shouldn't work in the definition of a ScaledJob.

Unless I am missing something here (see: https://github.com/kedacore/keda/discussions/3658), I believe that this is a bug.

Expected Behavior

Accept a PodSpec with repeated containerPort configuration as long as the naming differs (as it works in Deployment).

Actual Behavior

scaledjobs.keda.sh "-scaledjob" was not valid:

  • spec.jobTargetRef.template.spec.containers[0].ports[1]: Duplicate value: map[string]interface {}{"containerPort":8080, "protocol":"TCP"}

Steps to Reproduce the Problem

  1. Deploy a ScaledJob into a namespace
  2. Use kubectl edit to add the above ports section
  3. Try to save and exit

Logs from KEDA operator

No specific message related to the update attempt (as described above).

KEDA Version

2.8.1

Kubernetes Version

1.23

Platform

Other

Scaler Details

No response

Anything else?

No response

zd9KgA avatar Sep 19 '22 13:09 zd9KgA

Hi, Does this work on a regular job? I'm checking and we use batchv1.JobSpec, so basically we are using the same definition as k8s with jobs, if something is available there, should be available with ScaledJobs

JorTurFer avatar Sep 19 '22 18:09 JorTurFer

Hi, batchv1.JobSpec.template is a PodTemplateSpec [core/v1], and it's spec entry is a PodSpec [core/v1]. The containers entailed are of type Container [core/v1].

I didn't have a Job at hand, but quickly checked with a CronJob. Adding a repeated port as described above does work.

Many thanks for looking into this!

zd9KgA avatar Sep 19 '22 19:09 zd9KgA

okey, Thanks for the verification, seems totally legit that KEDA should support it if it works with other manifests that embedded podSpecs. Are you willing to contribute with this?

JorTurFer avatar Sep 19 '22 19:09 JorTurFer

Hi @JorTurFer, Willingness is not the limitation. I lack sufficient acquaintance with golang to be of good help. But I certainly have a set-up using which I could assist testing based on Docker images - provided this is of any benefit. My suspicion is that the problem is caused by an incomplete consideration of input during validation. Seemingly the optional field name is not used for disambiguation.

zd9KgA avatar Sep 19 '22 19:09 zd9KgA

I took a brief look at this when I saw the discussion thread, but couldn't quite figure out what the issue was either.

The validation is just a schema validation that's happening here https://github.com/kedacore/keda/blob/fc06165031509bb3f25506fbd97d307ca18eea79/config/crd/bases/keda.sh_scaledjobs.yaml#L1819-L1822

However, that schema is autogenerated from this type https://github.com/kubernetes/api/blob/052d63f042d1667beeaf166a6f7ae426aa1f293a/core/v1/types.go

and should be the same as L4405 to L4411 in this swagger (it's a bit too large to open in github, but you can download it to see io.k8s.api.core.v1.Container definition.)

 4405           "x-kubernetes-list-map-keys": [
 4406             "containerPort",
 4407             "protocol"
 4408           ],
 4409           "x-kubernetes-list-type": "map",
 4410           "x-kubernetes-patch-merge-key": "containerPort",
 4411           "x-kubernetes-patch-strategy": "merge"

though you'd notice our CRD is missing x-kubernetes-patch-merge-key and x-kubernetes-patch-strategy which are not generated for external CRDs (and ignored by kubernetes on apply)

Patching KEDA's CRDs to remove those 2 properties fixes this, though I'm not sure what the root cause is.

ahmelsayed avatar Sep 21 '22 00:09 ahmelsayed