fix: Use pointer-to-struct for fields with json omitempty#1063
Open
alanconway wants to merge 1 commit intorhobs:mainfrom
Open
fix: Use pointer-to-struct for fields with json omitempty#1063alanconway wants to merge 1 commit intorhobs:mainfrom
alanconway wants to merge 1 commit intorhobs:mainfrom
Conversation
Use pointers for API struct fields marked "omitempty".
"omitempty" does not work on non-pointer struct fields.
A zero valued struct will serialize at least as "{}" and may contain zero valued fields
if any fields are not "omitempty".
This can cause problems with default values and round-trip (de)serialization
and break even if the kubebuilder "+optional" comment is applied
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alanconway The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR updates tracing object storage configuration handling by renaming GCSSToken to GCSWIFSpec (Workload Identity Federation), correcting TLS ConfigMap field descriptions, and converting several struct fields from value types to pointer types to better represent optional configurations. Changes include updates to CRD manifests, Go API types, controller reconciliation logic, validation rules, and corresponding test cases across observability operator definitions. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controllers/observability/observability_controller.go (1)
207-233:⚠️ Potential issue | 🟠 MajorClear tracing status when tracing is nil or disabled.
Line 209 avoids the nil dereference, but the
elsestill only runs whenCapabilitiesis nil. If users removespec.capabilities.tracingor set tracing disabled, stale Tempo/OpenTelemetry status can remain.Proposed fix
- if instance.Spec.Capabilities != nil { - capabilities := instance.Spec.Capabilities - if capabilities.Tracing != nil && capabilities.Tracing.Enabled { + capabilities := instance.Spec.Capabilities + if capabilities != nil && capabilities.Tracing != nil && capabilities.Tracing.Enabled { otelcol := &otelv1beta1.OpenTelemetryCollector{} err := o.client.Get(ctx, types.NamespacedName{ Namespace: instance.Namespace, Name: otelCollectorName(instance.Name), }, otelcol) @@ instance.Status.Tempo = fmt.Sprintf("%s/%s (%s)", instance.Namespace, tempoName(instance.Name), tempo.Status.TempoVersion) instance.Status.OpenTelemetry = fmt.Sprintf("%s/%s (%s)", instance.Namespace, otelCollectorName(instance.Name), otelcol.Status.Version) - } } else { instance.Status.Tempo = "" instance.Status.OpenTelemetry = "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/observability/observability_controller.go` around lines 207 - 233, The code only clears instance.Status.Tempo and instance.Status.OpenTelemetry when instance.Spec.Capabilities is nil, leaving stale values when Capabilities exists but capabilities.Tracing is nil or capabilities.Tracing.Enabled is false; update the logic in the reconciliation block (around the otelcol/tempo retrieval) so that when capabilities.Tracing == nil or capabilities.Tracing.Enabled == false you explicitly set instance.Status.Tempo = "" and instance.Status.OpenTelemetry = "" (i.e., add an else branch for the capabilities.Tracing check that clears those status fields), leaving the existing nil Capabilities branch as-is.pkg/controllers/observability/tempo_components.go (1)
63-78:⚠️ Potential issue | 🟠 MajorCreate the TLS secret under the name referenced by TempoStack.
Line 74 configures Tempo to read the cert from
tempoStorageSecretName(instance.Name), but Line 156 creates the TLS Secret astempoSecretName(instance.Name). That leaves Tempo pointing at a non-existent cert Secret and can collide with the object storage credentials Secret.🐛 Proposed fix
objectStorageTLSSecret = &corev1.Secret{ TypeMeta: metav1.TypeMeta{ Kind: "Secret", APIVersion: corev1.SchemeGroupVersion.String(), }, ObjectMeta: metav1.ObjectMeta{ - Name: tempoSecretName(instance.Name), + Name: tempoStorageSecretName(instance.Name), Namespace: instance.Namespace, }, }Also applies to: 150-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/observability/tempo_components.go` around lines 63 - 78, Tempo's TLS cert Secret is being referenced as tempoStorageSecretName(instance.Name) in tempo.Spec.Storage.TLS.Cert but the code that creates the Secret uses tempoSecretName(instance.Name), causing Tempo to point to a non-existent Secret and risking a name collision with object storage credentials; update the Secret creation code so the TLS Secret is created with the same name used by Tempo (use tempoStorageSecretName(instance.Name)) and ensure any CA Secret/ConfigMap uses tempoStorageCAConfigMapName(instance.Name) so names align and avoid collision with tempoSecretName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/observability/tempo_components.go`:
- Around line 263-270: The error message in the GCSWIF branch is stale: when Get
fails for the key JSON secret (variable keyJSONSecret) you should update the
fmt.Errorf call in the objectStorageSpec.GCSWIF handling to reference GCSWIF
(not GCSSTS), e.g. change the message passed to fmt.Errorf in the block that
fetches objectStorageSpec.GCSWIF.KeyJSONSecret.Name so it accurately says
"failed to get GCSWIF keyJSON secret <name>: %w" while keeping the existing
error wrapping and returned values.
---
Outside diff comments:
In `@pkg/controllers/observability/observability_controller.go`:
- Around line 207-233: The code only clears instance.Status.Tempo and
instance.Status.OpenTelemetry when instance.Spec.Capabilities is nil, leaving
stale values when Capabilities exists but capabilities.Tracing is nil or
capabilities.Tracing.Enabled is false; update the logic in the reconciliation
block (around the otelcol/tempo retrieval) so that when capabilities.Tracing ==
nil or capabilities.Tracing.Enabled == false you explicitly set
instance.Status.Tempo = "" and instance.Status.OpenTelemetry = "" (i.e., add an
else branch for the capabilities.Tracing check that clears those status fields),
leaving the existing nil Capabilities branch as-is.
In `@pkg/controllers/observability/tempo_components.go`:
- Around line 63-78: Tempo's TLS cert Secret is being referenced as
tempoStorageSecretName(instance.Name) in tempo.Spec.Storage.TLS.Cert but the
code that creates the Secret uses tempoSecretName(instance.Name), causing Tempo
to point to a non-existent Secret and risking a name collision with object
storage credentials; update the Secret creation code so the TLS Secret is
created with the same name used by Tempo (use
tempoStorageSecretName(instance.Name)) and ensure any CA Secret/ConfigMap uses
tempoStorageCAConfigMapName(instance.Name) so names align and avoid collision
with tempoSecretName.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 27c981da-b5db-4b69-87f6-9b160772cc57
📒 Files selected for processing (14)
bundle/manifests/observability-operator.clusterserviceversion.yamlbundle/manifests/observability.openshift.io_observabilityinstallers.yamldeploy/crds/common/observability.openshift.io_observabilityinstallers.yamldocs/api.mdpkg/apis/observability/v1alpha1/objectstorage.gopkg/apis/observability/v1alpha1/tracing.gopkg/apis/observability/v1alpha1/tracing_test.gopkg/apis/observability/v1alpha1/types.gopkg/apis/observability/v1alpha1/zz_generated.deepcopy.gopkg/controllers/observability/observability_controller.gopkg/controllers/observability/reconcilers.gopkg/controllers/observability/reconcilers_test.gopkg/controllers/observability/tempo_components.gotest/e2e/observability_installer_test.go
Comment on lines
+263
to
+270
| } else if objectStorageSpec.GCSWIF != nil { | ||
| keyJSONSecret := &corev1.Secret{} | ||
| err := k8sClient.Get(ctx, client.ObjectKey{ | ||
| Namespace: instance.Namespace, | ||
| Name: objectStorageSpec.GCSSTSSpec.KeyJSONSecret.Name, | ||
| Name: objectStorageSpec.GCSWIF.KeyJSONSecret.Name, | ||
| }, keyJSONSecret) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get GCSSTS keyJSON secret %s: %w", objectStorageSpec.GCSSTSSpec.KeyJSONSecret.Name, err) | ||
| return nil, fmt.Errorf("failed to get GCSSTS keyJSON secret %s: %w", objectStorageSpec.GCSWIF.KeyJSONSecret.Name, err) |
There was a problem hiding this comment.
Update the stale GCSWIF error label.
This branch handles GCSWIF, but the returned error still says GCSSTS, which will mislead users when secret lookup fails.
📝 Proposed fix
if err != nil {
- return nil, fmt.Errorf("failed to get GCSSTS keyJSON secret %s: %w", objectStorageSpec.GCSWIF.KeyJSONSecret.Name, err)
+ return nil, fmt.Errorf("failed to get GCSWIF keyJSON secret %s: %w", objectStorageSpec.GCSWIF.KeyJSONSecret.Name, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if objectStorageSpec.GCSWIF != nil { | |
| keyJSONSecret := &corev1.Secret{} | |
| err := k8sClient.Get(ctx, client.ObjectKey{ | |
| Namespace: instance.Namespace, | |
| Name: objectStorageSpec.GCSSTSSpec.KeyJSONSecret.Name, | |
| Name: objectStorageSpec.GCSWIF.KeyJSONSecret.Name, | |
| }, keyJSONSecret) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get GCSSTS keyJSON secret %s: %w", objectStorageSpec.GCSSTSSpec.KeyJSONSecret.Name, err) | |
| return nil, fmt.Errorf("failed to get GCSSTS keyJSON secret %s: %w", objectStorageSpec.GCSWIF.KeyJSONSecret.Name, err) | |
| } else if objectStorageSpec.GCSWIF != nil { | |
| keyJSONSecret := &corev1.Secret{} | |
| err := k8sClient.Get(ctx, client.ObjectKey{ | |
| Namespace: instance.Namespace, | |
| Name: objectStorageSpec.GCSWIF.KeyJSONSecret.Name, | |
| }, keyJSONSecret) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get GCSWIF keyJSON secret %s: %w", objectStorageSpec.GCSWIF.KeyJSONSecret.Name, err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/observability/tempo_components.go` around lines 263 - 270,
The error message in the GCSWIF branch is stale: when Get fails for the key JSON
secret (variable keyJSONSecret) you should update the fmt.Errorf call in the
objectStorageSpec.GCSWIF handling to reference GCSWIF (not GCSSTS), e.g. change
the message passed to fmt.Errorf in the block that fetches
objectStorageSpec.GCSWIF.KeyJSONSecret.Name so it accurately says "failed to get
GCSWIF keyJSON secret <name>: %w" while keeping the existing error wrapping and
returned values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use pointers for API struct fields marked "omitempty".
"omitempty" does not work on non-pointer struct fields.
A zero valued struct will serialize at least as "{}" and may contain zero valued fields
if any fields are not "omitempty".
This can cause problems with default values and round-trip (de)serialization
and break even if the kubebuilder "+optional" comment is applied.
Additional fixes: