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

operator: Add warning for old schema configuration #11158

Merged
merged 18 commits into from
Dec 18, 2023
Merged

Conversation

btaani
Copy link
Contributor

@btaani btaani commented Nov 7, 2023

What this PR does / why we need it:
This PR adds a new warning condition to the LokiStack in case the object storage schema version is older than V13

Which issue(s) this PR fixes:
Fixes LOG-4548

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • 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

@btaani btaani marked this pull request as ready for review November 7, 2023 13:37
@btaani btaani requested review from periklis, xperimental and a team as code owners November 7, 2023 13:37
Copy link
Collaborator

@periklis periklis left a comment

Choose a reason for hiding this comment

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

In general this PR goes into the right direction. A couple of suggestions for better UX:

operator/internal/handlers/lokistack_create_or_update.go Outdated Show resolved Hide resolved
operator/apis/loki/v1/lokistack_types.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@xperimental xperimental left a comment

Choose a reason for hiding this comment

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

Overall a good start 👍
I have not tested this on a cluster yet, only looked at the source so far, but I added some comments already.

I second Peri's hint for an alert. I'm currently assuming that this is how the "insights" feature works in OpenShift.

operator/apis/loki/v1/lokistack_types.go Outdated Show resolved Hide resolved
operator/apis/loki/v1/lokistack_types.go Outdated Show resolved Hide resolved
operator/internal/status/storage.go Outdated Show resolved Hide resolved
operator/internal/status/storage.go Outdated Show resolved Hide resolved
operator/internal/status/lokistack.go Outdated Show resolved Hide resolved
operator/internal/status/storage.go Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Nov 20, 2023

Trivy scan found the following vulnerabilities:

  • HIGH, Target: docker.io/grafana/loki:main-724a841 (alpine 3.18.4), Type: alpine openssl: Incorrect cipher key and IV length processing in libcrypto3 v3.1.3-r0. Fixed in v3.1.4-r0
  • HIGH, Target: docker.io/grafana/loki:main-724a841 (alpine 3.18.4), Type: alpine openssl: Incorrect cipher key and IV length processing in libssl3 v3.1.3-r0. Fixed in v3.1.4-r0
    \nTo see more details on these vulnerabilities, and how/where to fix them, please run docker build -t grafana/loki:main-724a841 -f cmd/loki/Dockerfile .
    trivy i grafana/loki:main-724a841 on your branch. If these were not introduced by your PR, please considering fixing them in via a subsequent PR. Thanks!

operator/apis/loki/v1beta1/lokistack_types.go Outdated Show resolved Hide resolved
operator/apis/loki/v1/lokistack_types.go Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
## Main

- [11158](https://github.com/grafana/loki/pull/11158) **btaani**: operator: Add support to add additional storage schema V13
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the feature we're adding is the warning nudging the user to upgrade the schema:

Suggested change
- [11158](https://github.com/grafana/loki/pull/11158) **btaani**: operator: Add support to add additional storage schema V13
- [11158](https://github.com/grafana/loki/pull/11158) **btaani**: operator: Add warning for old schema configuration

operator/internal/status/status.go Outdated Show resolved Hide resolved
operator/internal/status/storage.go Outdated Show resolved Hide resolved
@btaani btaani changed the title operator: Add support to add additional storage schema V13 operator: Add warning for old schema configuration Dec 15, 2023
operator/apis/loki/v1/lokistack_types.go Outdated Show resolved Hide resolved
operator/internal/status/lokistack.go Outdated Show resolved Hide resolved
operator/internal/status/lokistack.go Outdated Show resolved Hide resolved
operator/internal/status/lokistack.go Outdated Show resolved Hide resolved
operator/internal/status/lokistack.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@xperimental xperimental left a comment

Choose a reason for hiding this comment

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

The new warning works nicely 👍

I left a few comments in the PR. I'll defer to @periklis for another pass.

operator/internal/manifests/internal/config/build_test.go Outdated Show resolved Hide resolved
operator/internal/status/lokistack_test.go Outdated Show resolved Hide resolved
operator/internal/status/lokistack.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@periklis periklis left a comment

Choose a reason for hiding this comment

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

LGTM and smoke testing working fine too. A couple of nits and one remaining question about removed tests.

operator/internal/status/lokistack.go Outdated Show resolved Hide resolved
operator/internal/status/lokistack.go Show resolved Hide resolved
operator/internal/status/lokistack_test.go Show resolved Hide resolved
@periklis periklis enabled auto-merge (squash) December 18, 2023 14:01
@periklis periklis merged commit 8931f4a into grafana:main Dec 18, 2023
14 checks passed
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants