Can already filter the integrations by cluster here instead of within line 90.

Well, so long story, this doesn't work. I'll fix it in a follow up

out of curiosity: why do we skip ECR integrations?

The ECR autogenerated integrations are special and use AWS IAM

Maybe add a comment here, then?

Sounds good, I've created ROX-12996 as a follow-up to remove exactly this.

Maybe add a comment here, then?

I looked at v1.GetImageIntegrationsRequest and it seems that it supports specifying a cluster (name only, not ID?). However, the handling of that field is extremely weird (`central/imageintegration/datastore/datastore_impl.go:55-64):

	integrations, err := ds.storage.GetAll(ctx)
	if err != nil {
		return nil, err
	}

	integrationSlice := integrations[:0]
	for _, integration := range integrations {
		if request.GetCluster() != "" {
			continue
		}

So if request specifies a non-empty cluster, it will always return an empty slice, while still doing a read on the DB. Not sure if there's a bug or this param was just never supported, but either way, this probably deserves a JIRA filed for it and addressed some time.

So, seems like the original PR #658 was actually removing support for this parameter (but apparently, this wasn't done because there was still something left for autogenerated image integrations that uses this parameter).

I do agree, it does justify a JIRA, I created one: ROX-13071

Tbh, I find it a bit ugly that the pipeline is in charge of all of this. IMHO, the manager should both abstract away the store insertion logic as well as the need to run detection. However, that may be hard to refactor. Still, let's factor this out into its own method, at least?

This doesn't make too much sense imho. A modified integration might just as well be a reason to run detection again. It would be nice if we could somehow scope the re-evaluation to images potentially using that integration, but that's far, far beyond the scope of this PR.

I wonder if the concern is that doing this on every update might be too frequent. I don't think however that updates of image pull secrets are very different in frequency from creations - most are probably never updated. A concern is the initial cluster connection. when we receive many new integrations and would start the detection loop several times. To avoid this, it might make sense to add a hasSynced concurrency.Flag, that is set to true on Reconcile followed by triggering detection, and then trigger detection here on every update whenever s.hasSynced.Get() is true.

@misberner Hm, not sure I completely get it from your comment, sorry.
Your suggestion is to set the flag within Reconcile() and then only short-circuit if the flag is set, right?

Regarding the frequencies, I saw this comment indicating a "higher" volume of updates due to service accounts, but to be fair I'm not quite sure whether this is of any concern.

Let me try to rephrase:

• Ideally, if an image integration is updated, we would want to reprocess all deployments with previously unscannable/unretrievable images that potentially use this registry integration.
• As an approximation of the above, we could reprocess all deployments that potentially use this registry integration, unscannable images or not.
• As an approximation of the above, we could reprocess all deployments that are somewhat related to this registry integration (e.g., same cluster), even if they don't use it, and regardless of whether they have unscannable images.

The point is that an updated registry integration can only affect a deployment evaluation if we can now scan an image used by the deployment, that we previously could not scan (due to the content-addressing nature of IDs, "being able to scan an image" is a binary property that we can assume never reverts to false once it became `true). Otherwise, there is no information that can be gained from the updated image integration and that could change anything about the evaluation.

The existing code however only reprocesses a deployment when a new image integration is created. This is neither an over- nor under-approximation of the "ideal" handling stated at the top of this post. It unnecessarily reprocesses all deployments (even those not using the updated image integration nor using an image from one of the registries this integration covers, it might even reprocess deployments in other clusters, and it also reprocesses deployment for which we already have comprehensive scan results for all images). At the same time, it misses cases when a previously existing, erroneous image integration is fixed, and there actually are deployments with still unscanned images due to the previously erroneous image integration. Those will only be fixed after ~1h, whereas those with a missing image integration are fixed directly on creation.

My idea was therefore to just run reprocessing on every image integration update, in order to arrive at something that at least is a consistent over-approximation. Still reprocessing far too much, but at least not missing anything; a fully-overblown but sufficient solution rather than a half-overblown yet still insufficient one.

My suspicion was that the case this approach wasn't chosen was due to the fact that during reconciliation (i.e., a sensor reconnects), we would receive a flood of image integration updates, causing an avalanche of reprocessings. However: (a) this is not how ShortCircuit() works anyway, it just triggers a signal, but if the enricher/detector is still busy, a subseqent call will just do nothing (note: this alone makes my comment on setting a flag and calling ShortCircuit once after reconciliation pointless, so just ignore it), and (b) the comment you pointed out suggests that this is due to a high volume of (spurious?) updates on OpenShift rather than the reconciliation phase.

So, in summary, keep the existing behavior (which I think now has accidentally been flipped), until we either can do reprocessing in a more targeted way (unscanned images only?), or understand why image integration updates on OpenShift are so noisy.

@lvalerom @fredrb here's another fun thing to consider for the sensor resync project: if a secret changes, and that secret is referenced as an image pull secret by a service account, then all deployments that use this service account need to be re-evaluated.

Don't overload variables if the name doesn't fit the usage any more. validTLS is a misnomer in l.177.

Use a named return value for the bool return value, or at least add a comment

Didn't you now flip when to do a short-circuit? Why would we not want to short circuit upon creation of an image integration?

Oh well, yup.

Let me try to rephrase:

• Ideally, if an image integration is updated, we would want to reprocess all deployments with previously unscannable/unretrievable images that potentially use this registry integration.
• As an approximation of the above, we could reprocess all deployments that potentially use this registry integration, unscannable images or not.
• As an approximation of the above, we could reprocess all deployments that are somewhat related to this registry integration (e.g., same cluster), even if they don't use it, and regardless of whether they have unscannable images.

The point is that an updated registry integration can only affect a deployment evaluation if we can now scan an image used by the deployment, that we previously could not scan (due to the content-addressing nature of IDs, "being able to scan an image" is a binary property that we can assume never reverts to false once it became `true). Otherwise, there is no information that can be gained from the updated image integration and that could change anything about the evaluation.

The existing code however only reprocesses a deployment when a new image integration is created. This is neither an over- nor under-approximation of the "ideal" handling stated at the top of this post. It unnecessarily reprocesses all deployments (even those not using the updated image integration nor using an image from one of the registries this integration covers, it might even reprocess deployments in other clusters, and it also reprocesses deployment for which we already have comprehensive scan results for all images). At the same time, it misses cases when a previously existing, erroneous image integration is fixed, and there actually are deployments with still unscanned images due to the previously erroneous image integration. Those will only be fixed after ~1h, whereas those with a missing image integration are fixed directly on creation.

My idea was therefore to just run reprocessing on every image integration update, in order to arrive at something that at least is a consistent over-approximation. Still reprocessing far too much, but at least not missing anything; a fully-overblown but sufficient solution rather than a half-overblown yet still insufficient one.

My suspicion was that the case this approach wasn't chosen was due to the fact that during reconciliation (i.e., a sensor reconnects), we would receive a flood of image integration updates, causing an avalanche of reprocessings. However: (a) this is not how ShortCircuit() works anyway, it just triggers a signal, but if the enricher/detector is still busy, a subseqent call will just do nothing (note: this alone makes my comment on setting a flag and calling ShortCircuit once after reconciliation pointless, so just ignore it), and (b) the comment you pointed out suggests that this is due to a high volume of (spurious?) updates on OpenShift rather than the reconciliation phase.

So, in summary, keep the existing behavior (which I think now has accidentally been flipped), until we either can do reprocessing in a more targeted way (unscanned images only?), or understand why image integration updates on OpenShift are so noisy.