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

fix(helm): Fix GEL image tag, bucket name and proxy URLs #12878

Merged
merged 11 commits into from
May 8, 2024

Conversation

monodot
Copy link
Contributor

@monodot monodot commented May 3, 2024

What this PR does / why we need it:
The chart doesn't currently seem to deploy GEL without having to do a couple of workarounds. This PR:

  • Updates the image tag referenced in the deployment to the correct 3.0.1 (without the v prefix).
  • Respects the user's admin bucket name.
  • Adds the correct downstream proxy URLs in the enterprise-gateway for deployments in SSD mode (the Ring Health tab in the Grafana Enterprise Logs plugin was otherwise unable to list the nodes in SSD).

Which issue(s) this PR fixes:
Fixes #12845

Special notes for your reviewer:

  • I have updated the image tag manually here to get the chart working, but I think the grafanabot job will also need to be updated (the one that gets the latest tag and inserts it into the Helm chart), otherwise this change will be overwritten on the next release. For that, there's an Issue here: https://github.com/grafana/enterprise-logs/issues/2381
  • I have reverted to using the user's configured bucket name for the admin bucket, could you please confirm it looks correct?

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

monodot and others added 2 commits May 3, 2024 14:13
Set the correct enterprise-logs image tag to 3.0.1 (without the 'v')

Use the correct proxy URLs on the gateway when deploying in SSD mode

Use the user's configured admin bucket name

Clarify README to note support for distributed mode
@CLAassistant
Copy link

CLAassistant commented May 3, 2024

CLA assistant check
All committers have signed the CLA.

@monodot monodot marked this pull request as ready for review May 3, 2024 13:26
@monodot monodot requested a review from a team as a code owner May 3, 2024 13:26
Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for catching these. Can you add a default for the admin bucket only in the case of using minio, and make sure it's empty otherwise?

@@ -480,7 +480,7 @@ enterprise:
admin_client:
storage:
s3:
bucket_name: admin
bucket_name: {{ .Values.loki.storage.bucketNames.admin }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, looks like we broken this in #12548. Would you mind adding a default for this value only in the case where both minio and enterprise are enabled? Otherwise there should be no default so we don't run into the problem fixed in #12548

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this now. I've separated the admin storage config out into its own separate Helm value (enterprise.adminClientConfig), which includes a default for Minio only.

To set any other storage options (e.g. AWS, GCS, etc), users can just override that adminClientConfig value.

Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally a user only has to provide loki.storage.bucketNames and everything else should just work. I think we should default the admin config to use loki.storage.bucketNames.admin in all cases but minio, and in the minio case we can hardcode it. I don't think we want to force a user to have to provide a whole adminClient section when they are not using minio.

Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>
Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 8, 2024
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label May 8, 2024
Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>
Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>
Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, thanks for addressing all my feedback!

Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>
Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>
@vlad-diachenko vlad-diachenko merged commit 67ed2f7 into grafana:main May 8, 2024
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm size/L type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm chart is using wrong image tag and proxy config for GEL in SSD mode
4 participants