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

Unable to use multiples ports with the same port number #2750

Open
2 tasks done
Byh0ki opened this issue Apr 27, 2023 · 11 comments
Open
2 tasks done

Unable to use multiples ports with the same port number #2750

Byh0ki opened this issue Apr 27, 2023 · 11 comments
Labels
bug Something isn't working no-issue-activity

Comments

@Byh0ki
Copy link

Byh0ki commented Apr 27, 2023

Checklist:

  • I've included steps to reproduce the bug.
  • 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):

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 Byh0ki added the bug Something isn't working label Apr 27, 2023
@aksingla33
Copy link

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

@zachaller
Copy link
Collaborator

@aksingla33 yea I think this is a good first issue

@zachaller zachaller added the good first issue Good for newcomers label May 1, 2023
@shimatar0
Copy link

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!

@zachaller
Copy link
Collaborator

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.

@AhmedGrati
Copy link
Contributor

AhmedGrati commented Jul 19, 2023

@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.

@zachaller
Copy link
Collaborator

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
Copy link
Collaborator

zachaller commented Jul 19, 2023

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

@AhmedGrati
Copy link
Contributor

AhmedGrati commented Jul 23, 2023

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

Nope it didn't work.

@zachaller zachaller removed the good first issue Good for newcomers label Jul 24, 2023
@razgrim
Copy link

razgrim commented Sep 18, 2023

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

Copy link
Contributor

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

@razgrim
Copy link

razgrim commented Jan 16, 2024

Bump still an issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working no-issue-activity
Projects
None yet
Development

No branches or pull requests

6 participants