✨ Support serviceaccount pull secrets #2005
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
tmshort
force-pushed
the
use-sa-pull-secret
branch
from
d09d1b8 to
26246b6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2005 +/- ##
==========================================
+ Coverage 69.17% 73.55% +4.37%
==========================================
Files 79 80 +1
Lines 7037 7146 +109
==========================================
+ Hits 4868 5256 +388
+ Misses 1887 1565 -322
- Partials 282 325 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/approve |
openshift-ci
bot
added
the
approved
tmshort
force-pushed
the
use-sa-pull-secret
branch
2 times, most recently
from
11579bc to
1c0e4dd
Compare
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
| AuthFilePath string | ||
| } | ||
|
|
||
| func (r *PullSecretReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { |
There was a problem hiding this comment.
Is req used anywhere other than logging? If not, I think I'd suggest dropping it (e.g. rename to _). And changing the logging to just generically say something like "reconciling pull secrets".
The name/namespace of the request without the type info might be a little confusing to show up in the log. But maybe we could log the events that pass our predicate in our predicate where we do have type information. That way there's still detail about what is causing our reconciler to be triggered?
There was a problem hiding this comment.
Interestingly... the logger has a lot of additional information added to it, so you can tell what type of resource triggered the event, I wrapped the log below to make it easier to read:
I0606 18:43:42.641045 1 pull_secret_controller.go:113] "found secret"
logger="pull-secret-reconciler"
controller="service-account-controller"
controllerGroup=""
controllerKind="ServiceAccount"
ServiceAccount="olmv1-system/operator-controller-controller-manager"
reconcileID="26404229-a2d3-495d-86ee-0da390a1e8f4"
name="pull-dockercfg"
namespace="olmv1-system"
There was a problem hiding this comment.
And when a pull secret is modified:
I0606 18:58:10.831129 1 pull_secret_controller.go:113] "found secret"
logger="pull-secret-reconciler"
controller="pull-secret-controller"
controllerGroup=""
controllerKind="Secret"
Secret="olmv1-system/pull-dockercfg"
reconcileID="a783ca58-0213-4fe3-a19d-3ed7ca21f5ae"
name="pull-dockercfg"
namespace="olmv1-system"
| _, err := ctrl.NewControllerManagedBy(mgr). | ||
| For(&corev1.Secret{}). | ||
| Named("pull-secret-controller"). | ||
| WithEventFilter(newSecretPredicate(r)). | ||
| Build(r) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| _, err = ctrl.NewControllerManagedBy(mgr). | ||
| For(&corev1.ServiceAccount{}). | ||
| Named("service-account-controller"). | ||
| WithEventFilter(newNamespacedPredicate(r.ServiceAccountKey)). | ||
| Build(r) |
There was a problem hiding this comment.
I don't think I'd setup two separate controllers. IIRC, there's a way to have a single controller with multiple watches. You may need to drop down to the lower-level controller package though (can't remember if For is required).
There was a problem hiding this comment.
TBH, having two controllers actually seems the cleanest, in that we get different invocations that recognize ServiceAccount vs. Secret, and it's clear what triggered the reconcile (i.e. the logger clearly indicates what resource is the trigger). With a single controller, we have to somehow map the ServiceAccount to a set of Secrets (which we may not yet know about yet), which feels kinda kludgy. Otherwise, the Secret controller is getting a ServiceAccount for the request, rather than a Secret.
|
lint doesn't like my logging trick... |
cmd/catalogd/main.go
Outdated
| setupLog.Error(err, "Unable to get pod namesapce and serviceaccount") | ||
| return err |
There was a problem hiding this comment.
| setupLog.Error(err, "Unable to get pod namesapce and serviceaccount") | |
| return err | |
| setupLog.Error(err, "Failed to extract namespace/serviceaccount from JWT token") | |
| return fmt.Errorf("failed to get service account identity from token: %w", err) |
There was a problem hiding this comment.
It's a bit redundant to wrap the error, since it's also logged.
cmd/catalogd/main.go
Outdated
| return err | ||
| } | ||
|
|
||
| setupLog.Info("Read token", "serviceaccount", saKey) |
There was a problem hiding this comment.
| setupLog.Info("Read token", "serviceaccount", saKey) | |
| if saKey.Namespace == "" || saKey.Name == "" { | |
| setupLog.Error(nil, "Extracted service account name or namespace is empty", "saKey", saKey) | |
| return fmt.Errorf("invalid service account identity extracted: %v", saKey) | |
| } | |
| setupLog.Info("Successfully extracted serviceaccount identity from token", "namespace", saKey.Namespace, "name", saKey.Name) |
There was a problem hiding this comment.
There's no point in checking for an empty name or namespace, as the sautil.GetServiceAccount() function should (will) error out if both can't be retrieved.
cmd/operator-controller/main.go
Outdated
| return err | ||
| } | ||
|
|
||
| setupLog.Info("Read token", "serviceaccount", saKey) |
There was a problem hiding this comment.
The block below appears identical in both cmd/catalogd/main.go and cmd/operator-controller/main.go.
Could we move this logic into a helper function under pkg/shared/util/sa and call it from both main.go files? That would reduce duplication and help with future changes
There was a problem hiding this comment.
Yup, I can try to consolidate a bit.
openshift-ci
bot
added
lgtm
tmshort
force-pushed
the
use-sa-pull-secret
branch
from
30cded3 to
de52a82
Compare
Serviceaccounts reference pull secrets! * Determine our serviceaccount (via the new internal/shared/util/sa package). * Use a common pull_secret_controller * Update the pull_secret_controller to know about the service account * Update the pull_secret_controller to watch the namespace-local secrets * Update caching to include sa, and use filters for additional secrets * Add RBAC to access these secrets and sa * Update writing the auth.json file to handle dockercfg and dockerconfigjson * Update writing the auth.json file to include multiple secrets Signed-off-by: Todd Short <[email protected]>
Signed-off-by: Todd Short <[email protected]>
Signed-off-by: Todd Short <[email protected]>
Signed-off-by: Todd Short <[email protected]>
Signed-off-by: Todd Short <[email protected]>
tmshort
force-pushed
the
use-sa-pull-secret
branch
from
de52a82 to
2144fad
Compare
Signed-off-by: Todd Short <[email protected]>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, joelanford, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit efc6657
into
operator-framework:main
Serviceaccounts reference pull secrets!
Description
Reviewer Checklist