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

Generated kubernetes module requires protocol in Container.ports which is not required by spec #52

Open
xhalo32 opened this issue Dec 27, 2023 · 2 comments

Comments

@xhalo32
Copy link

xhalo32 commented Dec 27, 2023

Line

type = (types.nullOr (coerceAttrsOfSubmodulesToListByKey "io.k8s.api.core.v1.ContainerPort" "name" [ "containerPort" "protocol" ]));
requires that protocol key be present in all helm generated Pod specifications. For example the following kubenix resource definition

resources.pods.hello.spec = {
  containers = [
    {
      name = "hello";
      image = "hello";
      ports = [
        {containerPort = 1234;}
      ];
    }
  ];
};

fails with the following error

       error: attribute 'protocol' missing

       at /nix/store/jznzhivlpsahv83fsvkp866bda50nd23-source/modules/generated/v1.27.nix:82:28:

           81|               (key:
           82|                 if isAttrs value.${key}
             |                            ^
           83|                 then toString value.${key}.content

It should not fail when protocol is missing from the ports.

There are certainly other similar issues with e.g. EphemeralContainer which need to be addressed as well.

Workaround

Currently I have just deleted the "protocol" from

type = (types.nullOr (coerceAttrsOfSubmodulesToListByKey "io.k8s.api.core.v1.ContainerPort" "name" [ "containerPort" "protocol" ]));
and everything is working smoothly.

Backstory

I ran into this issue while trying to deploy prometheus as a Helm chart. After a bit of debugging I noticed that the prometheus chart doesn't specify the port protocol: https://github.com/prometheus-community/helm-charts/blob/d628ebad62f119ef2985319a5f7a1dd5bee1863b/charts/prometheus/templates/deploy.yaml#L157

@xhalo32
Copy link
Author

xhalo32 commented Dec 27, 2023

I have made hacky preliminary changes to the k8s generator in my fork (which I guarantee are incorrect in some way) which has fixed my issue.

My change does not reintroduce #13

@pizzapim
Copy link
Contributor

To me this looks like an upstream Kubernetes bug. This conversation mentions some current problems with merge strategies with respect to some fields including ports: kubernetes-sigs/kustomize#3111

the current consensus is that merging should look for a x-kubernetes-list-map-keys field and use the list to merge if the list exists, then fall back to the singular field x-kubernetes-patch-merge-key and use that. The libraries must respect the intent of the schema.

I think their solution still does not account for the issue at hand though. So in my opinion a hacky way to fix it is desirable.

Here is a broader conversation about port related issues: kubernetes/kubernetes#105610

schradert added a commit to schradert/canivete that referenced this issue Jul 1, 2024
Relax requirement of required protocol in Container.ports because some
upstream sources, like Prometheus charts, don't actually specify it.

hall/kubenix#52
elohmeier added a commit to elohmeier/kubenix that referenced this issue Jul 2, 2024
schradert added a commit to schradert/canivete that referenced this issue Jul 22, 2024
Relax requirement of required protocol in Container.ports because some
upstream sources, like Prometheus charts, don't actually specify it.

hall/kubenix#52
GiyoMoon added a commit to GiyoMoon/kubenix that referenced this issue Sep 26, 2024
GiyoMoon added a commit to GiyoMoon/kubenix that referenced this issue Sep 26, 2024
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

No branches or pull requests

2 participants