argo-rollouts icon indicating copy to clipboard operation
argo-rollouts copied to clipboard

Unable to use multiples ports with the same port number

Open Byh0ki opened this issue 1 year ago • 11 comments

Checklist:

  • [x] I've included steps to reproduce the bug.
  • [x] I've included the version of argo rollouts.

Describe the bug

As stated in the documentation, Rollouts spec.template field should be 100% compatible with the classic Deployment spec.template field. When I try to use multiples ports that share the same port number but not the same name, I get:

The Rollout "bug-demo" is invalid: spec.template.spec.containers[0].ports[1]: Duplicate value: map[string]interface {}{"containerPort":8080, "protocol":"TCP"}

This is working fine if I convert the Rollout to a Deployment.

Related issue(s):

  • https://github.com/argoproj/argo-rollouts/issues/2271

To Reproduce

Manifest:

apiVersion: argoproj.io/v1alpha1
kind: Rollout
metadata:
  name: bug-demo
spec:
  replicas: 1
  selector:
    matchLabels:
      app: bug-demo
  template:
    metadata:
      labels:
        app: bug-demo
    spec:
      containers:
      - name: bug-demo
        image: argoproj/rollouts-demo:blue
        ports:
        - name: http
          containerPort: 8080
          protocol: TCP
        - name: metrics
          containerPort: 8080
          protocol: TCP

Expected behavior

Be able to use multiple ports using the same port number.

Version

Helm chart version: 2.26.0 Controller version: v1.4.1

Logs

No logs in the controller.


Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.

Byh0ki avatar Apr 27 '23 13:04 Byh0ki

Hi, I am new to this project and would like to contribute. Is this a good issue for me to start with?

aksingla33 avatar Apr 30 '23 10:04 aksingla33

@aksingla33 yea I think this is a good first issue

zachaller avatar May 01 '23 04:05 zachaller

Hi, If this has not already been addressed, I'd like to implement it and want to contribute! (I have confirmed that the latest version reproduces the problem)

I also have a few questions about the implementation.

I think the part that needs to be fixed is the process after apply, so I think it is correct to fix CustomResourceDefinition in the same way as Deployment(containerPort), is that correct?

But this did not work.

  1. where containerPort is defined, add the following to install.yaml
x-kubernetes-list-type: map
x-kubernetes-patch-strategy: merge <- add
x-kubernetes-patch-merge-key: containerPort <- add

e.g. https://github.com/argoproj/argo-rollouts/blob/master/manifests/install.yaml#L1581

  1. Tested with manifest with duplicate containerPort

I don't have enough knowledge of kubenetis, so if you could give me some advice, that would be great!

shimatar0 avatar Jun 28 '23 17:06 shimatar0

So I took a really quick look at this I am not sure it is a good first issue because we just bring in the spec from k8s and so it should actually just work. I would need to do a lot of digging to see what is going on with it, but it is probably something pretty deep within k8s schema stuff.

zachaller avatar Jul 06 '23 21:07 zachaller

@zachaller The problem is that in the open API schema that is fetched from Kubernetes, the following block is the root of the issue:

x-kubernetes-list-map-keys:
  - containerPort
  - protocol

This basically means that we can't accept two identical items in the list that have the same containerPort and protocol. Removing that block solved the issue for me, but we should find a better solution to programmatically generate the right CRD. Please LMK if I'm wrong.

AhmedGrati avatar Jul 19 '23 11:07 AhmedGrati

Yea, I feel this is possible some upstream issue as well all we currently do is embed the upstream pod spec in our type.

        // Template describes the pods that will be created.
	// +optional
	Template corev1.PodTemplateSpec `json:"template" protobuf:"bytes,3,opt,name=template"`

Then we get the upstream spec from https://raw.githubusercontent.com/kubernetes/kubernetes/release-1.26/api/openapi-spec/swagger.json so I believe we are generating it correctly it would require some hunting to see why there is a difference in behavior from upstream and the defined spec.

We are missing some of the upstream spec I also noticed:

"x-kubernetes-patch-merge-key": "containerPort",
"x-kubernetes-patch-strategy": "merge"

This code could probably use some updates: https://github.com/argoproj/argo-rollouts/blob/master/hack/gen-crd-spec/main.go

If I had to guess I would start with trying to do what this is doing https://github.com/argoproj/argo-schema-generator instead but I would be curious if adding those two keys also fixes the issue just like removing the keys did?

zachaller avatar Jul 19 '23 14:07 zachaller

One other thing to note the output of controller-gen has it matching what rollouts is putting in the crd

                                               required:
                                                  - containerPort
                                                  type: object
                                                type: array
                                                x-kubernetes-list-map-keys:
                                                - containerPort
                                                - protocol
                                                x-kubernetes-list-type: map

zachaller avatar Jul 19 '23 14:07 zachaller

I would be curious if adding those two keys also fixes the issue just like removing the keys did?

Nope it didn't work.

AhmedGrati avatar Jul 23 '23 12:07 AhmedGrati

Experiencing this issue as well.

"Failed sync attempt to 1c77ba2eca172f0f01f97d6cc54e76e2a5dadd6d: one or more objects failed to apply, reason: Rollout.argoproj.io "redacted" is invalid: [spec.template.spec.containers[1].ports[1]: Duplicate value:
spec.template.spec.containers[1].ports:" ......

spec.template.spec.containers[1].ports looks something like this

          - name: name1
             containerPort: 8538
             protocol: TCP
           - name: name2
             containerPort: 8538
             protocol: TCP
           - name: name3
             containerPort: 8638
             protocol: TCP
           - name: name4
             containerPort: 8538
             protocol: TCP

This spec is legitimate and applies correctly as

apiVersion: apps/v1
kind: deployment

It does not work as

apiVersion: argoproj.io/v1alpha1
kind: Rollout

razgrim avatar Sep 18 '23 15:09 razgrim

This issue is stale because it has been open 60 days with no activity.

github-actions[bot] avatar Nov 18 '23 02:11 github-actions[bot]

Bump still an issue

razgrim avatar Jan 16 '24 15:01 razgrim