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

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

Open
jenshu opened this issue Mar 29, 2023 · 6 comments

Comments

@jenshu
Copy link
Contributor

jenshu commented Mar 29, 2023

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

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

@sam-heilbron
Copy link
Contributor

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

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

Can we prioritize this @nfuden @jbohanon??

@nfuden
Copy link
Contributor

nfuden commented Aug 9, 2024

@kcbabo for prioritization question

@sam-heilbron
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants