✨ Add PullSecret controller to save pull secret data locally #1322

anik120 · 2024-09-30T16:12:09Z

RFC: https://docs.google.com/document/d/1BXD6kj5zXHcGiqvJOikU2xs8kV26TPnzEKp6n7TKD4M/edit#heading=h.x3tfh25grvnv

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-09-30T16:12:25Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`018510e`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6705361773e3d00008a5194e
😎 Deploy Preview	https://deploy-preview-1322--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-09-30T16:19:38Z

Codecov Report

Attention: Patch coverage is 44.85981% with 59 lines in your changes missing coverage. Please review.

Project coverage is 74.99%. Comparing base (dd1730a) to head (018510e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/manager/main.go	40.74%	26 Missing and 6 partials ⚠️
internal/controllers/pull_secret_controller.go	41.86%	21 Missing and 4 partials ⚠️
internal/rukpak/source/containers_image.go	80.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1322      +/-   ##
==========================================
- Coverage   76.42%   74.99%   -1.44%     
==========================================
  Files          41       42       +1     
  Lines        2431     2507      +76     
==========================================
+ Hits         1858     1880      +22     
- Misses        400      443      +43     
- Partials      173      184      +11

Flag	Coverage Δ
e2e	`56.84% <27.10%> (-1.62%)`	⬇️
unit	`52.85% <24.29%> (-0.88%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

config/base/rbac/role.yaml

internal/controllers/secret_syncer_controller.go

cmd/manager/main.go

anik120 · 2024-10-01T17:13:31Z

Where the PR stands so far:

On start up:

.
.
I1001 16:50:17.917567       1 controller.go:217] "Starting workers" controller="secret" controllerGroup="" controllerKind="Secret" worker count=1
.
.

On creation of a secret, the namespace/name for which is passed via the flag:

I1001 16:51:08.001691       1 secret_syncer_controller.go:103] "Saved Secret data locally" controller="secret" controllerGroup="" controllerKind="Secret" Secret="olmv1-system/pull-secret" namespace="olmv1-system" name="pull-secret" reconcileID="fe0d20fe-17dd-4af7-9a76-d433de66ed3a"

The data in the file system:

$ cat /tmp/operator-controller/auth.json
{".dockerconfigjson":"{\n  \"auths\": {\n    \"registry.build09.ci.openshift.org\": {\n      \"auth\": \"dXNlcjp..

More details about this file here

On deletion of the Secret:

I1001 17:03:31.719907       1 secret_syncer_controller.go:54] "Secret" controller="secret" controllerGroup="" controllerKind="Secret" Secret="olmv1-system/pull-secret" namespace="olmv1-system" name="pull-secret" reconcileID="ea3490a2-fa78-46d4-a396-a10cad77dddc" pull-secret="in namespace" olmv1-system="deleted."
I1001 17:03:31.720283       1 secret_syncer_controller.go:109] "Emptying local auth file." controller="secret" controllerGroup="" controllerKind="Secret" Secret="olmv1-system/pull-secret" namespace="olmv1-system" name="pull-secret" reconcileID="ea3490a2-fa78-46d4-a396-a10cad77dddc"
I1001 17:03:31.720629       1 secret_syncer_controller.go:116] "Auth file emptied successfully" controller="secret" controllerGroup="" controllerKind="Secret" Secret="olmv1-system/pull-secret" namespace="olmv1-system" name="pull-secret" reconcileID="ea3490a2-fa78-46d4-a396-a10cad77dddc" file="/tmp/operator-controller/auth.json"

Content in the file

$ cat /tmp/operator-controller/auth.json
$

cmd/manager/main.go

internal/controllers/secret_syncer_controller.go

skattoju · 2024-10-01T18:52:10Z

fixes #1015

internal/controllers/secret_syncer_controller.go

cmd/manager/main.go

internal/controllers/secret_syncer_controller.go

everettraven · 2024-10-03T13:19:00Z

cmd/manager/main.go

@@ -101,6 +105,7 @@ func main() {
 	flag.StringVar(&cachePath, "cache-path", "/var/cache", "The local directory path used for filesystem based caching")
 	flag.BoolVar(&operatorControllerVersion, "version", false, "Prints operator-controller version information")
 	flag.StringVar(&systemNamespace, "system-namespace", "", "Configures the namespace that gets used to deploy system resources.")
+	flag.StringVar(&globalPullSecret, "global-pull-secret", "", "The namespace/name of the global pull secret that is going to be used to pull bundle images.")


Just as a thing to note, there is a flag.Var() method that would allow us to implement a custom flag value with custom logic for handling how it is set, including validations.

I did something similar to this in kapp back when we were contributing preflight checks: https://github.com/carvel-dev/kapp/blob/ca6887d26d782636fe62a2b3f3bbbf5c91a965f3/pkg/kapp/preflight/registry.go#L64-L97

Might be overkill for this particular case, but this may be a good way to handle any custom flag validations we want in the future

cmd/manager/main.go

everettraven · 2024-10-07T17:46:11Z

cmd/manager/main.go

+		cacheOptions.ByObject[&corev1.Secret{}] = crcache.ByObject{
+			Namespaces: map[string]crcache.Config{
+				globalPullSecretKey.Namespace: {
+					LabelSelector: k8slabels.Everything(),
+					FieldSelector: fields.SelectorFromSet(map[string]string{
+						"metadata.name": globalPullSecretKey.Name,
+					}),
+				},
+			},
+		}


Does this also override the configuration for secrets in the systemNamespace? IIUC we want to make sure we have access to all the release secrets that get created by the secret driver we use for partitioning helm release secrets for larger bundles.

Approved pending an answer to this

Dirty secret: iirc, the release secret driver is still a live client. So checking whether that is working won't actually tell you anything.

Also I don't actually see any configuration for secretes in the systemNamespace. There's configuration for &ocv1alpha1.ClusterExtension{} and &catalogd.ClusterCatalog{}, but nothing for Secrets.

Dismissing because I had a question I forgot about that should be verified

LalatenduMohanty · 2024-10-07T18:55:24Z

cmd/manager/main.go

 	if systemNamespace == "" {
 		systemNamespace = podNamespace()
 	}

 	setupLog.Info("set up manager")
+	cacheOptions := crcache.Options{


Nitpick: If you can add some comments around what the below code doing it will add a lot to the readability of the code.

I'm not convinced any further documentation is necessary. The crcache.Options struct is pretty well documented. Once the reader understands what the controller-runtime cache does and what Options can be used to configure, the code reads "configuring caching options for the objects we own + additional object Secret in a specific namespace".

RFC: https://docs.google.com/document/d/1BXD6kj5zXHcGiqvJOikU2xs8kV26TPnzEKp6n7TKD4M/edit#heading=h.x3tfh25grvnv

Signed-off-by: Joe Lanford <[email protected]>

* ✨ Add PullSecret controller to save pull secret data locally RFC: https://docs.google.com/document/d/1BXD6kj5zXHcGiqvJOikU2xs8kV26TPnzEKp6n7TKD4M/edit#heading=h.x3tfh25grvnv * main.go: improved cache configuration for watching pull secret Signed-off-by: Joe Lanford <[email protected]> --------- Signed-off-by: Joe Lanford <[email protected]>

anik120 requested a review from a team as a code owner September 30, 2024 16:12

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 30, 2024

joelanford reviewed Sep 30, 2024

View reviewed changes

config/base/rbac/role.yaml Outdated Show resolved Hide resolved

joelanford reviewed Sep 30, 2024

View reviewed changes

internal/controllers/secret_syncer_controller.go Outdated Show resolved Hide resolved

anik120 force-pushed the secret-controller branch from d2dea55 to 500da77 Compare October 1, 2024 07:18

rashmi43 reviewed Oct 1, 2024

View reviewed changes

internal/controllers/secret_syncer_controller.go Outdated Show resolved Hide resolved

skattoju reviewed Oct 1, 2024

View reviewed changes

cmd/manager/main.go Show resolved Hide resolved

anik120 force-pushed the secret-controller branch from 500da77 to 7e1b8f7 Compare October 1, 2024 17:01

joelanford reviewed Oct 1, 2024

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

joelanford reviewed Oct 1, 2024

View reviewed changes

internal/controllers/secret_syncer_controller.go Outdated Show resolved Hide resolved

anik120 force-pushed the secret-controller branch from 7e1b8f7 to bb1966c Compare October 2, 2024 18:15

everettraven reviewed Oct 2, 2024

View reviewed changes