Unset (default) images in TempoStack CR #675
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #675 +/- ##
==========================================
- Coverage 78.22% 78.22% -0.01%
==========================================
Files 65 66 +1
Lines 5024 5023 -1
==========================================
- Hits 3930 3929 -1
Misses 907 907
Partials 187 187
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Various tools are capable of automatic rebuilding of images in case of CVEs (e.g. freshmaker [1]). These tools rebuild all images and update the RELATED_IMAGE_ environment variables in the CSV accordingly (ref. grafana#638). However, they do not update the version field (the github.com/grafana/tempo-operator/internal/version.operatorVersion LDFLAG when building the operator), they only update the version of the CSV. Therefore the upgrade process won't run, and nothing will replace the images field in the TempoStack CRs, i.e. the tempo/tempo-query/gateway pods are not updated with the new image version. This PR modifies the handling to use the default images if the image field of the TempoStack CR is unset. Then the operator will use the image from RELATED_IMAGE_ env variables, and the containers are always using the latest images as specified in the RELATED_IMAGE_ env vars. As a bonus, if a user is manually editing the images field, it won't get reset on every upgrade. [1] https://github.com/redhat-exd-rebuilds/freshmaker Resolves: grafana#674 Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
andreasgerstmayr
force-pushed
the
unset-images-field-in-crd-if-default
branch
from
7a67977
to
7e4a74c
Compare
| // This upgrade unsets the image fields in the TempoStack CR. | ||
| // From 0.6.0 onwards, the image location is not stored in the CR unless it got changed manually. | ||
| func upgrade0_6_0(ctx context.Context, u Upgrade, tempo *v1alpha1.TempoStack) (*v1alpha1.TempoStack, error) { | ||
| tempo.Spec.Images.Tempo = "" |
There was a problem hiding this comment.
If I understand this correctly, this is going to unset all the images on the CRs, but if an user specified an image and then do the upgrade, he will lose that setting?
There was a problem hiding this comment.
Yes. Hopefully for the last time though, after that PR the operator should not touch that field anymore.
Currently we also overwrite the image on every upgrade:
tempo-operator/internal/upgrade/upgrade.go
Line 167 in e89fe16
Is there a correct way to determine if the setting is the default or was changed by the user?
Looking for the current default docker.io/grafana/tempo:2.2.3 for tempo won't work in two cases:
- downstream products with different image locations
- in case the operator gets upgraded from e.g. 0.3.0 to 0.6.0 directly, i.e. skipping a version
There was a problem hiding this comment.
Thinking more about it. If the user configures a custom image, let's say a rebuild with modifications of tempo 2.2.3, and then upgrades the tempo operator and the new operator version requires tempo version 2.4.
This will likely break the installation anyway?
There was a problem hiding this comment.
In other words, setting a custom image only makes sense if the TempoStack is set to unmanaged imho (unmanaged also means it won't get upgraded).
There was a problem hiding this comment.
I don't think it is a way, I think the approach here is the right one. May be we need to put in the changelog or in some place that if you upgrade from 0.5.0 to 0.6.0 and you have custom images those changes will be overridden.
There was a problem hiding this comment.
Yes, that make sense to me.
There was a problem hiding this comment.
+1, I updated the text in the changelog entry to make it more clear.
…in-crd-if-default
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
|
fwiw, opentelemetry-operator (https://github.com/open-telemetry/opentelemetry-operator/blob/d76cf668d95bf70fff8e473cfb163f35d500b911/internal/manifests/collector/container.go#L40-L43) and jaeger-operator (https://github.com/jaegertracing/jaeger-operator/blob/50cd8c138b6f0b7de6463097069aa6f024b7e0bb/pkg/util/util.go#L277-L287) follow the same pattern regarding the |
| # (Optional) One or more lines of additional information to render under the primary note. | ||
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: | |
There was a problem hiding this comment.
I would document here the workaround
There was a problem hiding this comment.
Which workaround?
Various tools are capable of automatic rebuilding of images in case of
CVEs (e.g. freshmaker [1]). These tools rebuild all images and update
the RELATED_IMAGE_ environment variables in the CSV accordingly (ref. #638).
However, they do not update the version field (the
github.com/grafana/tempo-operator/internal/version.operatorVersion LDFLAG
when building the operator), they only update the version of the CSV.
Therefore the upgrade process won't run, and nothing will replace the
images field in the TempoStack CRs, i.e. the tempo/tempo-query/gateway
pods are not updated with the new image version.
This PR modifies the handling to use the default images if the image
field of the TempoStack CR is unset. Then the operator will use the image
from RELATED_IMAGE_ env variables, and the containers are always using the
latest images as specified in the RELATED_IMAGE_ env vars. As a bonus,
if a user is manually editing the images field, it won't get reset on every
upgrade.
[1] https://github.com/redhat-exd-rebuilds/freshmaker
Resolves: #674