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

duplicate containerPort with distinct protocol are not rendered correctly #13

Closed
herwigstuetz opened this issue Feb 7, 2023 · 4 comments · Fixed by #21
Closed

duplicate containerPort with distinct protocol are not rendered correctly #13

herwigstuetz opened this issue Feb 7, 2023 · 4 comments · Fixed by #21

Comments

@herwigstuetz
Copy link
Contributor

Exposing the same containerPort on both TCP and UDP does not work as only one port ends up in the manifest, see the example below.

After some digging, I found out that the x-kubernetes-patch-merge-key for ports in io.k8s.api.core.v1.Container in the swagger spec consists of only containerPort but not protocol. And since this is used by the generator to create the module, such ports get dropped during the evaluation in the module.

Since this is a known issue in kubernetes, kubenix probably shouldn't use x-kubernetes-patch-strategy for ports to generate the k8s manifests.

Example

nix build . on the flake

{
  inputs.kubenix.url = "github:hall/kubenix";

  outputs = {self, kubenix, ... }@inputs: let
    system = "aarch64-linux";
  in {
    packages.${system}.default = (kubenix.evalModules.${system} {
      module = { kubenix, ... }: {
        imports = with kubenix.modules; [k8s];
        kubernetes.resources.deployments.test.spec = {
          selector = {};
          template.spec.containers.test = {
            ports = [
              { containerPort = 53;
                protocol = "TCP";
              }
              { containerPort = 53;
                protocol = "UDP";
              }
            ];
          };
        };
      };
    }).config.kubernetes.resultYAML;
  };
}

only produces

apiVersion: apps/v1
kind: Deployment
metadata:
  annotations:
    kubenix/k8s-version: '1.26'
    kubenix/project-name: kubenix
  name: test
spec:
  selector: {}
  template:
    spec:
      containers:
      - name: test
        ports:
        - containerPort: 53
          protocol: TCP

and is missing

        - containerPort: 53
          protocol: UDP
@hall
Copy link
Owner

hall commented Feb 9, 2023

Thanks for opening this issue, @herwigstuetz!

I'll have to dig into this more over the weekend. Since it's an upstream issue without a clear resolution (from a quick glance, at least) my guess is that best we can do (if anything) would be to add a hacky workaround. Let us know if you have any suggestions but I'll see if I can't figure something out.

@herwigstuetz
Copy link
Contributor Author

@hall, thanks for looking into this, but also thanks for driving kubenix forward!

I think the merge strategy described in https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy might be worth looking into, but I'm not sure how easily this can be adopted for the kubenix modules.

For now, as a workaround, I'm basically just excluding io.k8s.api.core.v1.ContainerPort from using x-kubernetes-patch-merge-key.

@matejc
Copy link
Contributor

matejc commented Mar 12, 2023

How about this for a workaround?
In Kubenix:

              ports = {
                http = { containerPort = 8080; protocol = "TCP"; };
                not-http = { containerPort = 8080; protocol = "UDP"; };
              };

It outputs this Yaml:

ports:
  - containerPort: 8080
    protocol: TCP
  - containerPort: 8080
    protocol: UDP

@herwigstuetz
Copy link
Contributor Author

Thanks, this does work when I'm able to define my whole deployment in kubenix. Unfortunately, my use-case where I stumbled across this actually involves helm. I was trying to install this pihole helm chart. There, this does not work anymore.

But I am currently working on a fix, and I'm hoping that I can submit a PR soon.

herwigstuetz added a commit to herwigstuetz/kubenix that referenced this issue Mar 14, 2023
This change uses the attribute `name` if
`x-kubernetes-patch-merge-key` is not the full
`x-kubernetes-list-map-keys` to avoid data corruption.

Fixes hall#13
@hall hall closed this as completed in #21 May 25, 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 a pull request may close this issue.

3 participants