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

12.2 config issue #25

Open
phyzical opened this issue Apr 5, 2024 · 10 comments
Open

12.2 config issue #25

phyzical opened this issue Apr 5, 2024 · 10 comments

Comments

@phyzical
Copy link

phyzical commented Apr 5, 2024

wg-access-server-0.12.1...wg-access-server-0.12.2#diff-59df7a132e09c397ffc236cf3e4a6786ad1324a5639f8ea0b81a6911f08c6ac5R109-R111

Any one know why this line was removed?

Now it seems to just ignore the configmap entirly

@phyzical
Copy link
Author

phyzical commented Apr 5, 2024

i tried replicating the logic

extraVolumeMounts:
  - name: configmap-config
    mountPath: /config.yaml
    subPath: config.yaml

but then it started crashlooping with no logs

@phyzical
Copy link
Author

phyzical commented Apr 5, 2024

also now if you dont provide fullNameOverride the chart doesn't work. for example the PVC complains it doesn't have a name

@GoliathLabs
Copy link
Member

@hooray4me can you elaborate?

@hooray4me
Copy link
Contributor

It's been a minute since I looked at this chart, but wg-access-server.fullname didn't exist... wg-access-server.name pulls from the Chart name.

@hooray4me
Copy link
Contributor

hooray4me commented Jun 13, 2024

Now I remember... the helper's logic was producing a blank result for wg-access-server.fullname

{{- if .Values.fullnameOverride -}}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}
{{- else -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}}
{{- end -}}
{{- end -}}

^ That with the default values in the values.yaml of
nameOverride: ""
fullnameOverride:

It made more sense to ride off the Chart name.

@phyzical
Copy link
Author

phyzical commented Aug 8, 2024

Ah sorry, only just got back around to checking the latest version,

Can confirm 12.3 with that config injection rollback all is working again

@phyzical phyzical closed this as completed Aug 8, 2024
@phyzical
Copy link
Author

phyzical commented Aug 9, 2024

Hmm actually spoke too soon, 12.3 seems to never add the config.yaml either?

@phyzical phyzical reopened this Aug 9, 2024
@elcomtik
Copy link
Collaborator

I can confirm that the issue persists, I just tested it when trying to update to the latest version.

I think this could be a small PR to fix this. @phyzical feel free to create one, I'll merge it.

@elcomtik
Copy link
Collaborator

It looks like I can no longer merge PR In this project, so I'm sorry for the misleading info. Still, good PR will probably be merged. I don't have time for that now, so I won't promise I will create one by myself.

@phyzical I managed to use your workaround also adding MTU setting for clientConfig. Without it, it failed as you observed with yaml parser trying to convert an empty string to an integer. I think the chart default value should be set the same as it is for wireguard mtu setting

extraVolumeMounts:
  - name: configmap-config
    mountPath: /config.yaml
    subPath: config.yaml

config:
  clientConfig:
    mtu: 1420

@phyzical
Copy link
Author

@elcomtik thx ill try adding the clientConfig mtu default and try updating again, if it works ill adds these as defaults in a pr

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

4 participants