bitnami / charts

Bitnami Helm Charts
https://bitnami.com
Other
9.01k stars 9.23k forks source link

[bitnami/minio] MinIO chart generating invalid networkPolicy when using extraIngress parameter #29337

Closed bartramakers closed 1 month ago

bartramakers commented 2 months ago

Name and Version

bitnami/minio 14.7.7

What architecture are you using?

None

What steps will reproduce the bug?

Use the minio chart with the following configuration in values.yaml

Are you using any custom parameters or values?

networkPolicy:
    enabled: true
    allowExternal: false
    allowExternalEgress: false
    extraIngress:
    - from:
      - namespaceSelector:
          matchLabels:
            name: redacted1
    - from:
      - namespaceSelector:
          matchLabels:
            name: redacted2

What is the expected behavior?

To have it create a NetworkPolicy with an ingress parameter like follows;

ingress:
    # Allow inbound connections
    - ports:
        - port: 9001
        - port: 9000
      from:
        - podSelector:
            matchLabels:
              redacted-minio-client: "true"
        - podSelector:
            matchLabels:
              app.kubernetes.io/instance: redacted
              app.kubernetes.io/name: minio
    - from:
      - namespaceSelector:
          matchLabels:
            name: redacted1
    - from:
      - namespaceSelector:
          matchLabels:
            name: redacted2

What do you see instead?

Instead, the extraIngress contents are included at the expected place, but also after the first entry for the from parameter:

  ingress:
    # Allow inbound connections
    - ports:
        - port: 9001
        - port: 9000
      from:
        - podSelector:
            matchLabels:
              redacted-minio-client: "true"
        - podSelector:
            matchLabels:
              app.kubernetes.io/instance: redacted
              app.kubernetes.io/name: minio
        - from:
          - namespaceSelector:
              matchLabels:
                name: redacted1
        - from:
          - namespaceSelector:
              matchLabels:
                name: redacted2
    - from:
      - namespaceSelector:
          matchLabels:
            name: redacted1
    - from:
      - namespaceSelector:
          matchLabels:
            name: redacted2

Additional information

Resulting in the following error on install:

W0910 16:29:17.937731  110795 warnings.go:70] unknown field "spec.ingress[0].from[2].from"
W0910 16:29:17.937753  110795 warnings.go:70] unknown field "spec.ingress[0].from[3].from"
javsalgar commented 2 months ago

Thank you so much for reporting! Indeed the issue is in the networkPolicy.yaml file, instead of:

        {{- if $extraIngress }}
        {{- toYaml $extraIngress | nindent 8 }}
        {{- end }}

it should be:

        {{- if $extraIngress }}
        {{- toYaml $extraIngress | nindent 6 }}
        {{- end }}

As you spotted the issue, would you like to submit a PR?

bartramakers commented 2 months ago

I'm happy to create a PR with that change in it, no problem at all!

But that would only solve the incorrect indentation part of this issue, and the values from the extraIngress would still be present twice in the output.

The extraInress parameter is output twice by the template. Once on line 75-78, and then again on line 81, but in a different way:

75    {{- $extraIngress := coalesce .Values.networkPolicy.extraIngress .Values.networkPolicy.extraFromClauses }}
76    {{- if $extraIngress }}
77    {{- toYaml $extraIngress | nindent 6 }}
78    {{- end }}
80   {{- if .Values.networkPolicy.extraIngress }}
81   {{- include "common.tplvalues.render" ( dict "value" .Values.networkPolicy.extraIngress "context" $ ) | nindent 4 }}
82   {{- end }}

I don't know the underlying idea of the coalesce operation for the extraIngress and extraFromClauses so i'm not sure which one would be at the correct position or in what scenario you would need both of them. Do you have any idea on this?

javsalgar commented 2 months ago

Good catch! Indeed, lines 80-82 should be removed, as we are already doing the coalesce operation above. We've been working on standardizations and probably that snippet was accidentally added .

bartramakers commented 2 months ago

I'll include the removal in the PR then, thanks!

github-actions[bot] commented 1 month ago

This Issue has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thanks for the feedback.

github-actions[bot] commented 1 month ago

Due to the lack of activity in the last 5 days since it was marked as "stale", we proceed to close this Issue. Do not hesitate to reopen it later if necessary.