You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign kadel for approval. For more information see the Code Review Process.
The full list of commands accepted by this bot can be found here.
Details
Needs approval from an approver in each of these files:
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🔒 Security concerns
Potential secret exposure: pkg/model/secretenvs.go injects all key/value pairs from any Secret in the namespace labeled with rhdh.redhat.com/default-config into the Backstage pod environment. This broad mechanism can unintentionally expose sensitive values if a user labels an unrelated Secret (or a Secret contains more keys than intended). Consider scoping/allow-listing which secrets/keys can be injected, or documenting the risk clearly.
The controller appends pointers to range variables when building NamespacedConfig.EnvSecrets. In Go, taking the address of the loop variable can result in all pointers referencing the same memory, so the model may end up using the last secret repeatedly. Consider storing by value, or taking the address of secretList.Items[i] instead.
// discoverNamespacedConfig finds user-provided resources in the namespace via labels// Returns secrets with label rhdh.redhat.com/default-config="" (generic) or matching flavorfunc (r*BackstageReconciler) discoverNamespacedConfig(ctx context.Context, namespacestring, flavorstring) (model.NamespacedConfig, error) {
namespacedConfig:=model.NewNamespacedConfig()
// List all secrets with default-config label (any value)secretList:=&corev1.SecretList{}
iferr:=r.List(ctx, secretList, namespacedConfig.ListOptions(namespace)); err!=nil {
returnnamespacedConfig, fmt.Errorf("failed to list default config secrets: %w", err)
}
for_, secret:=rangesecretList.Items {
labelValue:=secret.Labels[model.DefaultConfigLabel]
// Include if generic (empty value) or matches flavoriflabelValue==""||labelValue==flavor {
namespacedConfig.EnvSecrets=append(namespacedConfig.EnvSecrets, &secret)
}
}
returnnamespacedConfig, nil
}
Flavor default lookup depends on LOCALBIN and assumes flavored defaults exist under <LOCALBIN>/default-config/flavors/<flavor>/<key>. In the shipped manifests, flavor files are mounted to /default-config/flavors/... (not under LOCALBIN/default-config/...). Verify LOCALBIN is set appropriately in the operator deployment (or adjust the path logic) so flavored defaults can actually be found at runtime.
In discoverNamespacedConfig, fix the loop over secretList.Items to correctly capture the secret variable before taking its address. This prevents all elements in namespacedConfig.EnvSecrets from incorrectly pointing to the last secret in the list.
- for _, secret := range secretList.Items {-+ for i := range secretList.Items {+ secret := secretList.Items[i]
labelValue := secret.Labels[model.DefaultConfigLabel]
// Include if generic (empty value) or matches flavor
if labelValue == "" || labelValue == flavor {
namespacedConfig.EnvSecrets = append(namespacedConfig.EnvSecrets, &secret)
}
}
Apply / Chat
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical bug where taking the address of a loop variable would cause all appended pointers to reference the last element, leading to incorrect secret configurations.
High
Skip missing flavor directory
Update DefFile to silently fall back to the default configuration if a specified flavor directory does not exist, preventing startup failures for invalid or unused flavors.
func DefFile(key string, flavor string) (string, error) {
localBin := os.Getenv("LOCALBIN")
if localBin == "" {
localBin = "."
}
if flavor != "" {
- // Check flavor directory exists
flavorDir := filepath.Join(localBin, "default-config", "flavors", flavor)
- if _, err := os.Stat(flavorDir); err != nil {- if os.IsNotExist(err) {- return "", fmt.Errorf("flavor '%s' does not exist: directory not found at %s", flavor, flavorDir)+ if info, err := os.Stat(flavorDir); err == nil && info.IsDir() {+ flavorPath := filepath.Join(flavorDir, key)+ if _, err := os.Stat(flavorPath); err == nil {+ return flavorPath, nil
}
- return "", fmt.Errorf("failed to check flavor directory '%s': %w", flavor, err)
}
-- // Try flavor file first- flavorPath := filepath.Join(flavorDir, key)- if _, err := os.Stat(flavorPath); err == nil {- return flavorPath, nil- }+ // silently skip missing or invalid flavorDir and fall back
}
- // Fallback to default-config
return filepath.Join(localBin, "default-config", key), nil
}
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly proposes to make the flavor discovery more robust by silently falling back to default configurations if a flavor directory is missing, which aligns with the feature's design of flavors being optional overrides.
Medium
High-level
Re-evaluate the implicit namespace-level configuration
The "Namespaced Config" feature implicitly discovers secrets using labels. This should be changed to an explicit configuration within the BackstageSpec to improve clarity and maintainability.
// In backstage_controller.gofunc (r*BackstageReconciler) Reconcile(ctx, req) {
// ...// Implicitly discover secrets in the namespace via labelsnamespacedConfig, err:=r.discoverNamespacedConfig(ctx, backstage.Namespace, backstage.Spec.Flavor)
// ...// Pass discovered config to the modelbsModel, err:=model.InitObjects(ctx, backstage, externalConfig, namespacedConfig, ...)
// ...
}
// In model/secretenvs.gofunc (p*SecretEnvs) addExternalConfig(spec) {
// Inject env vars from discovered secretsfor_, secret:=rangep.model.NamespacedConfig.EnvSecrets {
p.model.backstageDeployment.addEnvVarsFrom(..., secret.Name, "")
}
// ...
}
After:
// In api/v1alpha5/backstage_types.gotypeBackstageSpecstruct {
// ...// Explicitly list secrets to use for namespaced config// +optionalNamespacedEnvFromSecrets []string`json:"namespacedEnvFromSecrets,omitempty"`// ...
}
// In backstage_controller.gofunc (r*BackstageReconciler) Reconcile(ctx, req) {
// ...// No implicit discovery. Controller fetches secrets listed in the spec.namespacedConfig:=model.NewNamespacedConfig()
for_, secretName:=rangebackstage.Spec.NamespacedEnvFromSecrets {
// fetch secret by name and add to namespacedConfig.EnvSecrets
}
bsModel, err:=model.InitObjects(ctx, backstage, externalConfig, namespacedConfig, ...)
// ...
}
Suggestion importance[1-10]: 8
__
Why: This is a significant design suggestion that correctly identifies a move towards implicit, label-based configuration, which can reduce clarity and make debugging harder.
Medium
Organization best practice
Provide runnable flavor example
Update the example to demonstrate spec.flavor usage and include placeholder Secrets (and labels) needed for the flavor so the manifest can be applied without missing dependencies.
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 6
__
Why:
Relevant best practice - Add complete, runnable examples by including required dependent resources (e.g., Secrets/ConfigMaps) with safe placeholder values so users can apply them out-of-the-box.
Low
Validate the flavor field
Add kubebuilder validation (e.g., enum) and clarify behavior for unknown flavors to prevent reconcile loops due to typos and make the feature self-describing in the CRD.
// Flavor specifies a pre-configured template for Backstage deployment (e.g., "orchestrator", "ai").
// When specified, operator loads default configuration from the flavor template instead of standard defaults.
-// Optional.+// +kubebuilder:validation:Enum=ai;orchestrator
// +optional
Flavor string `json:"flavor,omitempty"`
Apply / Chat
Suggestion importance[1-10]: 5
__
Why:
Relevant best practice - When introducing optional features controlled by CR fields, document and validate the field to avoid confusing failures and improve lifecycle management.
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
Introduce Flavors as specified: https://docs.google.com/document/d/1CgxFZt7ITVb8LsbxZ1YRJP5CZZw7WMIZA3I_vUL8SmE/edit?tab=t.0
Which issue(s) does this PR fix or relate to
PR acceptance criteria
How to test changes / Special notes to the reviewer