Refactor runtime object handling #2918
Open
gazarenkov wants to merge 11 commits into
Open
Conversation
# Conflicts: # pkg/model/appconfig.go # pkg/model/configmapfiles.go # pkg/model/deployment.go # pkg/model/deployment_test.go # pkg/model/dynamic-plugins.go # pkg/model/dynamic-plugins_test.go # pkg/model/runtime.go # pkg/model/runtime_test.go # pkg/model/secretfiles.go
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2918 +/- ##
==========================================
+ Coverage 61.07% 61.59% +0.52%
==========================================
Files 37 37
Lines 2109 2148 +39
==========================================
+ Hits 1288 1323 +35
Misses 686 686
- Partials 135 139 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
|
/agentic_review |
Code Review by Qodo
1. Deployment nil deref panic
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
Comment on lines
74
to
+101
| } | ||
|
|
||
| // implementation of RuntimeObject interface | ||
| func (b *BackstageDeployment) setObject(obj runtime.Object) { | ||
|
|
||
| var err error | ||
| b.deployable, err = CreateDeployable(obj) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("cannot set deployment object: %v", err)) | ||
| } | ||
| func (b *BackstageDeployment) GetKey() string { | ||
| return DeploymentKey | ||
| } | ||
|
|
||
| // implementation of RuntimeObject interface | ||
| func (b *BackstageDeployment) addToModel(model *BackstageModel, backstage api.Backstage) (bool, error) { | ||
| if b.deployable.GetObject() == nil { | ||
| return false, fmt.Errorf("backstage Deployment is not initialized, make sure there is deployment.yaml in default or raw configuration") | ||
| func (b *BackstageDeployment) addToModel(model *BackstageModel, backstage api.Backstage, config runtime.Object, scheme *runtime.Scheme) error { | ||
| // Set the deployment from config parameter if not nil | ||
| if config != nil { | ||
| var err error | ||
| b.deployable, err = CreateDeployable(config) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot set deployment object: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if BackstageContainerIndex(b.podSpec()) < 0 { | ||
| return false, fmt.Errorf("backstage Deployment is not initialized, Backstage Container is not identified") | ||
| } | ||
| b.model = model | ||
|
|
||
| if b.deployable.PodObjectMeta().Annotations == nil { | ||
| b.deployable.PodObjectMeta().Annotations = map[string]string{} | ||
| } | ||
| b.deployable.PodObjectMeta().Annotations[ExtConfigHashAnnotation] = model.ExternalConfig.WatchingHash | ||
| // Call setMetaInfo if deployment is not nil | ||
| if b.deployable.GetObject() != nil { | ||
| model.setRuntimeObject(b) | ||
| b.setMetaInfo(backstage, scheme) | ||
|
|
||
| model.backstageDeployment = b | ||
| model.setRuntimeObject(b) | ||
| b.model = model | ||
| if BackstageContainerIndex(b.podSpec()) < 0 { | ||
| return fmt.Errorf("backstage Deployment is not initialized, Backstage Container is not identified") | ||
| } |
There was a problem hiding this comment.
1. Deployment nil deref panic 🐞 Bug ☼ Reliability
BackstageDeployment.addToModel() and Object() dereference b.deployable without guarding against it being nil when config is absent, which can panic during InitObjects instead of returning an error. This can crash reconciliation if deployment.yaml is missing from both overlay and defaults.
Agent Prompt
### Issue description
`BackstageDeployment` can panic because `b.deployable` is never initialized when `config == nil`, yet `addToModel()` and `Object()` call `b.deployable.GetObject()`.
### Issue Context
- Factory returns an empty `&BackstageDeployment{}`.
- `addToModel()` only initializes `b.deployable` when `config != nil`.
- If config is missing, current code still calls `b.deployable.GetObject()` (nil deref).
### Fix Focus Areas
- pkg/model/deployment.go[40-119]
- pkg/model/runtime.go[149-193]
### Expected fix
- Make `Object()` return `nil` when `b.deployable == nil`.
- In `addToModel()`, if `config == nil` (deployment is required), return a clear error (similar to previous behavior) rather than skipping/continuing.
- Optionally, add a validation step in `InitObjects()` ensuring required objects (deployment+service) exist.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+95
to
169
| if backstage.Spec.Application != nil && backstage.Spec.Application.DynamicPluginsConfigMapName != "" { | ||
| specPlugins := &p.model.ExternalConfig.DynamicPlugins | ||
|
|
||
| // if the ConfigMap is set but does not have the data or expected key | ||
| if specPlugins.Data == nil || specPlugins.Data[DynamicPluginsFile] == "" { | ||
| return fmt.Errorf("dynamic plugin configMap expects '%s' Data key", DynamicPluginsFile) | ||
| } | ||
|
|
||
| //// resolve references | ||
| // TODO | ||
| //plugins, err := GetPluginsData(specPlugins) | ||
| //if err != nil { | ||
| // return err | ||
| //} | ||
| //for _, plugin := range plugins { | ||
| // if plugin.Package | ||
| // | ||
| //} | ||
|
|
||
| if p.ConfigMap != nil { | ||
| // Merge user's config with default config | ||
| //mergedData, err := p.mergeWith(specPlugins.Data[DynamicPluginsFile]) | ||
| mergedData, err := MergePluginsData(p.ConfigMap.Data[DynamicPluginsFile], specPlugins.Data[DynamicPluginsFile]) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to merge dynamic plugins config: %w", err) | ||
| } | ||
| p.ConfigMap.Data[DynamicPluginsFile] = mergedData | ||
| } else { | ||
| // No default config, point to user's config | ||
| p.ConfigMap = specPlugins | ||
| } | ||
| } | ||
|
|
||
| p.setMetaInfo(backstage, scheme) | ||
| // Always add wrapper to model (unconditional) | ||
| model.setRuntimeObject(p) | ||
| model.DynamicPlugins = *p | ||
| return true, nil | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // implementation of RuntimeObject interface | ||
| // ConfigMap name must be the same as (deployment.yaml).spec.template.spec.volumes.name.dynamic-plugins-conf.ConfigMap.name | ||
| func (p *DynamicPlugins) updateAndValidate(backstage api.Backstage) error { | ||
| // TODO | ||
| // extract pluginConfigs | ||
| // merge with app-config (deep merge) | ||
| func (p *DynamicPlugins) updateAndValidate(backstage api.Backstage, scheme *runtime.Scheme) error { | ||
|
|
||
| // Only proceed if there's a ConfigMap to mount or dynamic plugins config in spec | ||
| if p.ConfigMap == nil && (backstage.Spec.Application == nil || backstage.Spec.Application.DynamicPluginsConfigMapName == "") { | ||
| // No dynamic plugins configuration, nothing to do | ||
| return nil | ||
| } | ||
|
|
||
| deployment := p.model.getDeployment() | ||
| if deployment == nil { | ||
| return fmt.Errorf("backstage deployment not found in model") | ||
| } | ||
|
|
||
| _, initContainer := p.getInitContainer() | ||
| if initContainer == nil { | ||
| return fmt.Errorf("failed to find initContainer named %s", dynamicPluginInitContainerName) | ||
| } | ||
| if backstage.Spec.Application == nil || backstage.Spec.Application.DynamicPluginsConfigMapName == "" { | ||
| if err := p.model.backstageDeployment.mountFilesFrom(containersFilter{names: []string{dynamicPluginInitContainerName}}, ConfigMapObjectKind, | ||
| p.ConfigMap.Name, initContainer.WorkingDir, DynamicPluginsFile, true, utils.SortedKeys(p.ConfigMap.Data)); err != nil { | ||
| return fmt.Errorf("failed to mount dynamic plugins configMap: %w", err) | ||
| } | ||
|
|
||
| if err := deployment.mountFilesFrom(containersFilter{names: []string{dynamicPluginInitContainerName}}, ConfigMapObjectKind, | ||
| p.ConfigMap.Name, initContainer.WorkingDir, DynamicPluginsFile, true, utils.SortedKeys(p.ConfigMap.Data)); err != nil { | ||
| return fmt.Errorf("failed to mount dynamic plugins configMap: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // implementation of RuntimeObject interface | ||
| func (p *DynamicPlugins) setMetaInfo(backstage api.Backstage, scheme *runtime.Scheme) { | ||
| p.ConfigMap.SetName(DynamicPluginsDefaultName(backstage.Name)) | ||
| setMetaInfo(p.ConfigMap, backstage, scheme) | ||
| } | ||
|
|
||
| func (p *DynamicPlugins) addExternalConfig(spec api.BackstageSpec) error { | ||
| if spec.Application != nil && spec.Application.DynamicPluginsConfigMapName != "" { | ||
|
|
||
| _, initContainer := p.getInitContainer() | ||
| if initContainer == nil { | ||
| return fmt.Errorf("failed to find initContainer named %s", dynamicPluginInitContainerName) | ||
| } | ||
|
|
||
| dp := &p.model.ExternalConfig.DynamicPlugins | ||
|
|
||
| // if the ConfigMap is set but does not have the data or expected key | ||
| if dp.Data == nil || dp.Data[DynamicPluginsFile] == "" { | ||
| return fmt.Errorf("dynamic plugin configMap expects '%s' Data key", DynamicPluginsFile) | ||
| } | ||
|
|
||
| if p.ConfigMap != nil { | ||
| mergedData, err := p.mergeWith(dp.Data[DynamicPluginsFile]) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to merge dynamic plugins config: %w", err) | ||
| } | ||
| p.ConfigMap.Data[DynamicPluginsFile] = mergedData | ||
| err = p.model.backstageDeployment.mountFilesFrom(containersFilter{names: []string{dynamicPluginInitContainerName}}, ConfigMapObjectKind, | ||
| p.ConfigMap.Name, initContainer.WorkingDir, DynamicPluginsFile, true, utils.SortedKeys(p.ConfigMap.Data)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to mount dynamic plugins configMap: %w", err) | ||
| } | ||
| } else { | ||
| err := p.model.backstageDeployment.mountFilesFrom(containersFilter{names: []string{dynamicPluginInitContainerName}}, ConfigMapObjectKind, | ||
| dp.Name, initContainer.WorkingDir, DynamicPluginsFile, true, utils.SortedKeys(dp.Data)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to mount dynamic plugins configMap: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if p.ConfigMap != nil { | ||
| p.ConfigMap.SetName(DynamicPluginsDefaultName(backstage.Name)) | ||
| setMetaInfo(p.ConfigMap, backstage, scheme) | ||
| } |
There was a problem hiding this comment.
2. External cm ssa create fails 🐞 Bug ≡ Correctness
DynamicPlugins.addToModel can set p.ConfigMap to ExternalConfig.DynamicPlugins (loaded from a live cluster ConfigMap) and then rename it; applying this renamed object via server-side apply can fail because the object still carries cluster-populated metadata (e.g., resourceVersion/UID). This breaks the spec-only dynamic plugins path when no default dynamic-plugins.yaml is present.
Agent Prompt
### Issue description
When `DynamicPluginsConfigMapName` is set and there is no default dynamic-plugins config object, the code reuses the *live* external ConfigMap object (`ExternalConfig.DynamicPlugins`) and then renames/owns it. That reused object includes `ObjectMeta` fields from the cluster (`resourceVersion`, `uid`, `managedFields`, etc.). Applying it under a new name using server-side apply can fail on create.
### Issue Context
- `preprocessSpec()` loads the external ConfigMap via `client.Get`, populating full metadata.
- `DynamicPlugins.addToModel()` assigns `p.ConfigMap = specPlugins` and then `SetName(...)`.
- `applyPayload()` uses SSA apply, which will create the object if it doesn't exist.
### Fix Focus Areas
- pkg/model/dynamic-plugins.go[85-170]
- internal/controller/spec_preprocessor.go[147-154]
- internal/controller/backstage_controller.go[159-176]
### Expected fix
Choose one of:
1) **Preferred:** Create a fresh `corev1.ConfigMap{}` and copy only `Data`/`BinaryData` (and any required annotations) from the external ConfigMap; do not reuse the external object's `ObjectMeta`.
2) If reuse is required, explicitly clear create-blocking fields before apply (e.g., `ResourceVersion`, `UID`, `ManagedFields`, `OwnerReferences`, possibly `CreationTimestamp`).
Add/adjust tests to simulate external ConfigMaps with populated metadata (resourceVersion/uid) and ensure reconciliation still succeeds.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 participants
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.
Description
This PR refactors the core operator runtime model to simplify object initialization flow
Key Changes
Which issue(s) does this PR fix or relate to
https://redhat.atlassian.net/browse/RHDHBUGS-2975
PR acceptance criteria
How to test changes / Special notes to the reviewer
It does not change the behaviour