gloo-ee chart renders invalid yaml when default gw proxy disabled and kubeResourceOverride is used

jenshu commented 1 year ago

Gloo Edge Version

1.12.x

Kubernetes Version

None

Describe the bug

Trying to install GlooEE with these values

gloo:
  gatewayProxies:
    gatewayProxy:
      disabled: true
    otherProxy:
      disabled: false
      kubeResourceOverride:
        spec:
          template:
            metadata:
              labels:
                foo: bar
license_key: foobar

results in an error Error: Failed to render chart: exit status 1: Error: unable to build kubernetes objects from release manifest: error parsing : invalid Yaml document separator: apiVersion: apps/v1 due to the --- separator not being on its own line:

# Source: gloo-ee/charts/gloo/templates/7-gateway-proxy-deployment.yaml
---apiVersion: apps/v1
kind: Deployment

This seems to only occur when all the following are true:

using the gloo-ee chart (couldn't reproduce in the gloo chart, but we should double check that)
when the default gatewayProxy is disabled
the enabled proxy comes alphabetically after gatewayProxy (i.e. couldn't reproduce with a proxy named aProxy)
the enabled proxy uses a kubeResourceOverride

Might be fixed by moving the divider to right after this line

We should add testing to ensure we're not making the same mistake in other templates too.

Steps to reproduce the bug

see above

Expected Behavior

render valid yaml

Additional Context

No response

ben-taussig-solo commented 7 months ago

I have just moved this issue to blocked. Some context:

A community user had put up this PR with the intention of resolving the issue: https://github.com/solo-io/gloo/pull/9294
Per Jenny's request in Slack, I put up a solo-projects PR with a failing test that should pass when this issue gets fixed: https://github.com/solo-io/solo-projects/pull/5960
However, that test did not pass when I pulled in the helm chart generated from the community PR
I put up another Gloo PR to see if I could easily resolve the issue; however, none of my changes were sufficient to get the s-p test passing. That PR is here: https://github.com/solo-io/gloo/pull/9305
At this point, I think we need to regroup on this work -- I had picked this up impromptu and it has required more involved effort than I had expected

sam-heilbron commented 7 months ago

It would be nice if we expanded our manifest renderer utility: https://github.com/solo-io/go-utils/tree/main/helmutils, so that we could proactively catch issues like this in the future

ben-taussig-solo commented 7 months ago

After reviewing this slack thread, I believe that:

This issue seems to describe a slightly different bug from the one the community user was experiencing
The community PR should definitely resolve the user's helm formatting issue, but we don't necessarily understand the specific circumstances under which that issue occurs
Because of this discrepancy, the PR I put up to test that the issue described here is resolved did not pass CI when we pulled in the helm chart generated from the community PR

AlfredoFausto commented 3 months ago

Can we prioritize this @nfuden @jbohanon??

nfuden commented 3 months ago

@kcbabo for prioritization question

sam-heilbron commented 2 months ago

This issue, still exists. There is a bug in our Helm chart, that is trigged when using the kubeResourceOverride. However, there was an enhancement made to the Helm API, to support the priority class name. The following issues were resolved by the linked PR:

The idea was that supporting the priorityClassName on the proxy would bypass the need to rely on the kubeResourceOverride. This was based on a conversation with @AlfredoFausto .

cc @BeauSelisker28 @kcbabo

solo-io / gloo