[alicefr]

The controller now keeps one Secret, bootc-pull-secrets, with one key ( <namespace>_<name>) per pull secret a pool references, and mounts it into the daemon.

The daemon copies that mounted entry to /run/ostree/auth.json before staging and deletes the file when there is none, instead of reading Secrets through the API.

This removes the daemon's cluster-wide secrets permission, so nodes can only read the mounted Secret.

Based on #273
Partially Fixes: ##272

[HarshwardhanPatil07]

I will let @cheesesashimi have the final look here, need to head at other place, thank you!

[cheesesashimi]

nit: Is it possible to place the aggregator in its own module? I'm mostly thinking about separation of concerns. For example, we could relocate it under internal/pullsecret. Or, since it's effectively a new controller, we could do something like this to keep things more logically grouped:

internal/controller/bootcnodepool/<all current bootcnode controller code>
internal/controller/pullsecret/<all pull secret aggregator code>

[cheesesashimi]

issue: Here are a few edge-cases we may want to consider:

1. Legacy pull secrets (e.g., corev1.DockerConfigKey / corev1.SecretTypeDockercfg. We should detect these and normalize them though I'll put my comments around normalization elsewhere.
2. Opqaue secrets with a .dockerconfigjson or .dockercfg key.
3. Transitions from Opaque -> {DockerConfigJson, DockerConfig} (and vice-versa). Although there is an immutable bit on secrets, we can't always assume that it is set to true, especially if another operator is what created the secret.

Something like the following should cover all of these edge-cases:

func isPullSecretCandidate(obj client.Object) bool {
	secret, ok := obj.(*corev1.Secret)
	if !ok {
		return false
	}

	_, hasJSON := secret.Data[corev1.DockerConfigJsonKey]
	_, hasLegacy := secret.Data[corev1.DockerConfigKey]

	return hasJSON || hasLegacy ||
		secret.Type == corev1.SecretTypeDockerConfigJson ||
		secret.Type == corev1.SecretTypeDockercfg
}

func (r *PullSecretAggregator) secretPredicate() predicate.Predicate {
	isAggregate := func(obj client.Object) bool {
		return client.ObjectKeyFromObject(obj) == r.secretKey()
	}

	isMatch := func(obj client.Object) bool {
		return isAggregate(obj) || isPullSecretCandidate(obj)
	}

	return predicate.Funcs{
		CreateFunc: func(e event.CreateEvent) bool {
			return isMatch(e.Object)
		},
		UpdateFunc: func(e event.UpdateEvent) bool {
			return isMatch(e.ObjectOld) || isMatch(e.ObjectNew)
		},
		DeleteFunc: func(e event.DeleteEvent) bool {
			return isMatch(e.Object)
		},
	}
}

Additionally, we may want to consider even tighter scoping in the future where we can give a list of secret names to watch so that we don't have to reconcile whenever any secret in a given namespace changes, regardless of whether we're consuming it or not.

[alicefr]

Additionally, we may want to consider even tighter scoping in the future where we can give a list of secret names to watch so that we don't have to reconcile whenever any secret in a given namespace changes, regardless of whether we're consuming it or not.

Agree, but if you don't mind I would postpone this since right now we are passing the allowed namespace statically. If something changes the operator needs to be restarted. So, I would introduce this after #257 and #92. We want also to restrict the RBAC to that secrets and we want to adjust them dynamically

[cheesesashimi]

issue: We'll need to normalize legacy secrets (e.g., corev1.DockerConfigKey). This is a problem I solved for the MCO team, which I did like this: https://github.com/openshift/machine-config-operator/blob/main/pkg/secrets/secrets.go. We might not want something that sophisticated here, but its an idea.

Also, since we're also doing a similar operation in the BootcNodePool controller, we should consider making it a common function that both controllers can use.

[cheesesashimi]

thought: Since we now have the pull secret mounted as a projected volume, we may want to consider using fsnotify to proactively update auth.json whenever the secrets change. It's also possible that the secrets could be changed while an update is in progress, so we may want to consider how to handle that too.

[alicefr]

I have considered the case where the secret change while we are updating. I think, it can be reduced to 2 cases, the old credentials are still valid and the pull will be successful or it will fail because it require new credentials. In the last case, we will reconcile and retry until the credentials are updated and correct. I think it is a good mechanism for keeping the logic relatively simple

[alicefr]

we may want to consider using fsnotify to proactively update auth.json whenever the secrets change

we aren't using fsnotify because we simply reread the secret every time we need to pull and we don't cache it. So, there is no need to watch for fsnotify event. We could review this mechanism if necessary but if you don't mind I would tackle this in a separate PR since it is already en existing mechanism