🐛 Generalize SecretManager to also handle ConfigMaps #463
Conversation
Required for metal3-io#431. Signed-off-by: Dmitry Tantsur <[email protected]>
metal3-io-bot
added
the
size/M
dtantsur
changed the title
Signed-off-by: Dmitry Tantsur <[email protected]>
dtantsur
force-pushed
the
configmap-secret
branch
from
33923ab to
d1c5487
Compare
metal3-io-bot
added
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Rozzii The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
metal3-io-bot
added
the
approved
There was a problem hiding this comment.
Pull request overview
This PR generalizes the SecretManager to handle both Secrets and ConfigMaps, supporting the requirements for issue #431. The refactoring extracts Secret-specific logic into generic methods that work with any client.Object, then adds new ConfigMap-specific methods that leverage this generalized implementation.
Key Changes:
- Refactored
findSecretandclaimSecretinto genericfindObjectandclaimObjectmethods that acceptclient.Object - Added
AcquireConfigMapandObtainConfigMapmethods parallel to the existing Secret methods - Updated
AddSecretSelectorto cache both Secrets and ConfigMaps with the ironic environment label - Modified the Ironic controller to use
SecretManager.ObtainConfigMap()instead of directly fetching ConfigMaps - Added
updatepermission for ConfigMaps in RBAC configuration
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/secretutils/secret_manager.go | Refactored to use generic findObject and claimObject methods; added new AcquireConfigMap and ObtainConfigMap methods |
| pkg/secretutils/label.go | Extended to add ConfigMap selector alongside Secret selector; updated to use maps.Insert for adding both selectors |
| pkg/secretutils/label_test.go | Updated test expectations from 1 to 2 selectors to account for both Secret and ConfigMap |
| internal/controller/ironic_controller.go | Changed to use SecretManager.ObtainConfigMap() for retrieving ConfigMaps |
| config/rbac/role.yaml | Added update verb to ConfigMaps permissions to support label/owner reference updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| secret := &corev1.ConfigMap{} | ||
| err := sm.findObject(key, secret) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to fetch config map %s in namespace %s: %w", key.Name, key.Namespace, err) | ||
| } | ||
| err = sm.claimObject(secret, owner, scheme) | ||
| return secret, err |
There was a problem hiding this comment.
The variable name 'secret' is misleading here since it's actually a ConfigMap. Rename it to 'configMap' for clarity and consistency with the method's purpose.
| secret := &corev1.ConfigMap{} | |
| err := sm.findObject(key, secret) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to fetch config map %s in namespace %s: %w", key.Name, key.Namespace, err) | |
| } | |
| err = sm.claimObject(secret, owner, scheme) | |
| return secret, err | |
| configMap := &corev1.ConfigMap{} | |
| err := sm.findObject(key, configMap) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to fetch config map %s in namespace %s: %w", key.Name, key.Namespace, err) | |
| } | |
| err = sm.claimObject(configMap, owner, scheme) | |
| return configMap, err |
| secret := &corev1.ConfigMap{} | ||
| err := sm.findObject(key, secret) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to fetch config map %s in namespace %s: %w", key.Name, key.Namespace, err) | ||
| } | ||
| err = sm.claimObject(secret, owner, scheme) | ||
| return secret, err |
There was a problem hiding this comment.
The variable name 'secret' is misleading here since it's actually a ConfigMap. Use 'configMap' consistently throughout the method.
| secret := &corev1.ConfigMap{} | |
| err := sm.findObject(key, secret) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to fetch config map %s in namespace %s: %w", key.Name, key.Namespace, err) | |
| } | |
| err = sm.claimObject(secret, owner, scheme) | |
| return secret, err | |
| configMap := &corev1.ConfigMap{} | |
| err := sm.findObject(key, configMap) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to fetch config map %s in namespace %s: %w", key.Name, key.Namespace, err) | |
| } | |
| err = sm.claimObject(configMap, owner, scheme) | |
| return configMap, err |
| func (sm *SecretManager) claimSecret(secret *corev1.Secret, owner client.Object, scheme *runtime.Scheme) error { | ||
| log := sm.log.WithValues("secret", secret.Name, "secretNamespace", secret.Namespace) | ||
| func (sm *SecretManager) claimObject(object client.Object, owner client.Object, scheme *runtime.Scheme) error { | ||
| log := sm.log.WithValues("secret", object.GetName(), "secretNamespace", object.GetNamespace()) |
There was a problem hiding this comment.
The log field names 'secret' and 'secretNamespace' are now incorrect since this method handles both Secrets and ConfigMaps. Consider renaming them to 'object' and 'objectNamespace' or 'name' and 'namespace' to be generic.
| log := sm.log.WithValues("secret", object.GetName(), "secretNamespace", object.GetNamespace()) | |
| log := sm.log.WithValues("object", object.GetName(), "objectNamespace", object.GetNamespace()) |
| if !metav1.HasLabel(secret.ObjectMeta, LabelEnvironmentName) { | ||
| currentLabels := object.GetLabels() | ||
| if _, found := currentLabels[LabelEnvironmentName]; !found { | ||
| log.Info("setting secret environment label") |
There was a problem hiding this comment.
The log message 'setting secret environment label' is misleading when the object is a ConfigMap. Consider making it generic, such as 'setting environment label' or dynamically including the object type.
| log.Info("setting secret environment label") | |
| log.Info(fmt.Sprintf("setting %s environment label", object.GetObjectKind().GroupVersionKind().Kind)) |
| if err := controllerutil.SetOwnerReference(owner, secret, scheme); err != nil { | ||
| ownerLog.Info("setting owner reference") | ||
| if err := controllerutil.SetOwnerReference(owner, object, scheme); err != nil { | ||
| return fmt.Errorf("failed to set secret owner reference: %w", err) |
There was a problem hiding this comment.
The error message 'failed to set secret owner reference' is misleading when the object is a ConfigMap. Consider making it generic, such as 'failed to set owner reference' or dynamically including the object type.
| return fmt.Errorf("failed to set secret owner reference: %w", err) | |
| return fmt.Errorf("failed to set %s owner reference: %w", object.GetObjectKind().GroupVersionKind().Kind, err) |
| func (sm *SecretManager) AcquireConfigMap(key types.NamespacedName, owner client.Object, scheme *runtime.Scheme) (*corev1.ConfigMap, error) { | ||
| secret := &corev1.ConfigMap{} | ||
| err := sm.findObject(key, secret) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to fetch config map %s in namespace %s: %w", key.Name, key.Namespace, err) | ||
| } | ||
| err = sm.claimObject(secret, owner, scheme) | ||
| return secret, err | ||
| } | ||
|
|
||
| // ObtainConfigMap retrieves a ConfigMap and ensures that it has a label that will | ||
| // ensure it is present in the cache (and that we can watch for changes). | ||
| // This version does not set owner references. | ||
| func (sm *SecretManager) ObtainConfigMap(key types.NamespacedName) (*corev1.ConfigMap, error) { | ||
| return sm.AcquireConfigMap(key, nil, nil) | ||
| } |
There was a problem hiding this comment.
The new AcquireConfigMap and ObtainConfigMap methods lack test coverage. Given that the existing Secret methods have comprehensive tests (including edge cases like fallback to API reader, owner references, and label preservation), similar tests should be added for the ConfigMap methods to ensure they behave correctly.
| if err := sm.client.Update(sm.ctx, secret); err != nil { | ||
| return fmt.Errorf("failed to update secret %s in namespace %s: %w", secret.Name, secret.Namespace, err) | ||
| if err := sm.client.Update(sm.ctx, object); err != nil { | ||
| return fmt.Errorf("failed to update %s %s in namespace %s: %w", object.GetObjectKind().GroupVersionKind().Kind, object.GetName(), object.GetNamespace(), err) |
There was a problem hiding this comment.
Using object.GetObjectKind().GroupVersionKind().Kind may return an empty string for objects retrieved from the API, as GetObjectKind() often returns an empty ObjectKind unless explicitly set. Consider using a type switch or reflection to determine the actual type (e.g., *corev1.Secret or *corev1.ConfigMap) for a more reliable error message.
tuminoid
left a comment
tuminoid
left a comment
There was a problem hiding this comment.
Some nits from the bot, and I think this shouldn't be titled bug fix as it just expands support into configmaps?
It does stop IrSO from listening to all ConfigMaps, although admittedly it's not clear from the summary |
Ahhh. OK, then it makes sense. Maybe fix the desc? :) |
4 participants
Required for #431.
Signed-off-by: Dmitry Tantsur [email protected]