feat(auth): auth config single source of truth - #77
Conversation
…ructionLabel to garden CM, add ShootAuthConfiguredByLabel to Shoot, watch garden CMs by label predicate in shoot controller On-behalf-of: @SAP krzysztof.zagorski@sap.com Signed-off-by: Zaggy21 <k.zaggy@gmail.com>
On-behalf-of: @SAP krzysztof.zagorski@sap.com Signed-off-by: Zaggy21 <k.zaggy@gmail.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR changes Shoot OIDC authentication handling to overwrite the Garden authentication ConfigMap with the Greenhouse source-of-truth content, adds ownership/traceability labels, and updates controller behavior to react to relevant ConfigMap changes.
Changes:
- Switch OIDC auth sync from “merge” semantics to verbatim overwrite of Garden CM
config.yamlfrom the Greenhouse CM. - Add labeling/annotation to track which CareInstruction configured a Shoot and which CM is managed.
- Extend the Shoot controller to watch labeled ConfigMaps and enqueue reconciliations; update/replace tests accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| controller/shoot/shoot_controller_test.go | Updates integration tests to assert overwrite semantics and new labels. |
| controller/shoot/shoot_controller.go | Adds a ConfigMap watch and a mapper function to enqueue Shoot reconciles on CM changes. |
| controller/shoot/auth_test.go | Replaces merge-focused unit tests with overwrite-focused tests using fake clients. |
| controller/shoot/auth.go | Implements overwrite behavior, adds “configured-by” label on Shoots, and annotates/labels managed Garden CMs. |
| api/v1alpha1/careinstruction_types.go | Introduces a constant for the Shoot “auth-configured-by” label key. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
controller/shoot/shoot_controller.go:130
- This watch handler performs a
Liston every qualifying ConfigMap event. Ifr.GardenClientis not the controller-runtime cached client backing the watch, this can turn into frequent API-server list calls under churn. Prefer using the manager’s cached client for the list (or ensureGardenClientis the cache-backed client) and consider adding a field/label index forShootAuthConfiguredByLabelso lookups scale better with the number of Shoots.
// EnqueueShoots maps a ConfigMap change to reconcile requests for Shoots that were configured by the same CareInstruction.
func (r *ShootController) EnqueueShoots(ctx context.Context, obj client.Object) []ctrl.Request {
ciName := obj.GetLabels()[v1alpha1.CareInstructionLabel]
var shoots gardenerv1beta1.ShootList
if err := r.GardenClient.List(ctx, &shoots,
client.InNamespace(obj.GetNamespace()),
client.MatchingLabels{v1alpha1.ShootAuthConfiguredByLabel: ciName},
); err != nil {
r.Error(err, "failed to list Shoots for ConfigMap watch")
return nil
}
controller/shoot/auth.go:22
authConfigMapManagedByis used as an annotation key later, but the name doesn’t indicate it’s an annotation (and the key itself ends with/managed-by, which is commonly a label convention). To reduce confusion, consider renaming the constant to something explicit likeauthConfigMapManagedByAnnotation(or switch to a conventional managed-by label if that’s the intent).
const (
authConfigMapKey = "config.yaml"
authConfigMapManagedBy = "shoot-grafter.cloudoperators.dev/managed-by"
)
…updated, update readme On-behalf-of: @SAP krzysztof.zagorski@sap.com Signed-off-by: Zaggy21 <k.zaggy@gmail.com>
…ts lookups On-behalf-of: @SAP krzysztof.zagorski@sap.com Signed-off-by: Zaggy21 <k.zaggy@gmail.com>
…apManagedByAnnotation On-behalf-of: @SAP krzysztof.zagorski@sap.com Signed-off-by: Zaggy21 <k.zaggy@gmail.com>
Merging this branch changes the coverage (1 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
uwe-mayer
left a comment
There was a problem hiding this comment.
This looks good! 🚀
one minor comment about the label. Should we make it more generic?
| // ShootStatusExcluded indicates the shoot was excluded by the ShootSelector filter criteria. | ||
| ShootStatusExcluded = "Excluded" | ||
|
|
||
| // ShootAuthConfiguredByLabel is the label placed on a Shoot to identify which CareInstruction |
There was a problem hiding this comment.
On a second thought I think we can use the more generic shoot-grafter.cloudoperators.dev/careinstruction label, to not reduce the information to auth only?
WDYT?
| sc := &shoot.ShootController{ | ||
| GreenhouseClient: r.Client, | ||
| GardenClient: gardenClient, | ||
| GardenClient: shootControllerMgr.GetClient(), |
There was a problem hiding this comment.
Why are we changing this?
Summary
shoot-grafter.cloudoperators.dev/careinstruction=<careInstructionName>to record ownership.shoot-grafter.cloudoperators.dev/auth-configured-by=<careInstructionName>label to each Shoot that receives OIDC configuration.Watches(&corev1.ConfigMap{})in ShootController.SetupWithManager filtered by CareInstructionLabel + data-changed predicate, so a change to the garden-cluster auth CM triggers re-reconciliation of all Shoots in the namespace.shoot-grafter.cloudoperators.dev/managed-byto signal it is maintained by automation and should not be edited by hand.