Fix sharding flags not being honored

[matheuscscp]

Fixes: #350

Implement sharding (flag already existed).

[matheuscscp]

This PR needs an update here:

https://fluxcd.io/flux/components/source/options/#source-watcher-flags

[Iam-Karan-Suresh]

This PR needs an update here:

https://fluxcd.io/flux/components/source/options/#source-watcher-flags

Hi @matheuscscp I would like to work on that. If it is ok I would like to create a pr

[matheuscscp]

This PR needs an update here:
https://fluxcd.io/flux/components/source/options/#source-watcher-flags

Hi @matheuscscp I would like to work on that. If it is ok I would like to create a pr

Sure, just wait until we merge this one

[stefanprodan]

The flags are there to match SC, removing them will break Flux bootstrapping

[stefanprodan]

Also sharding is not in scope, we are going to break all users of sharding by doing what’s in this PR!

[matheuscscp]

Break?

[matheuscscp]

The flags are there to match SC, removing them will break Flux bootstrapping

Fixed, thanks!

[matheuscscp]

Also sharding is not in scope, we are going to break all users of sharding by doing what’s in this PR!

I think source-watcher is broken now, the binary advertises a flag that does not work, this is quite unexpected:

./source-watcher -h 2>&1 | grep label-selector
      --watch-configs-label-selector string       Watch for ConfigMaps and Secrets with matching labels. (default "reconcile.fluxcd.io/watch=Enabled")
      --watch-label-selector string               Watch for resources with matching labels e.g. 'sharding.fluxcd.io/shard=shard1'.

It's 💯 ok to leave --watch-configs-label-selector there to avoid the bootstrap issue (and if we start looking up ConfigMaps/Secrets later we can just honor the flag), but it's quite absurd to expose --watch-lavel-selector and not honor it, the controller can perfectly do that, and it must if the flag must be kept. This is 100% a bug in my opinion. Whatever breaking change this represents (I don't think it does), we should do it.

[matheuscscp]

1. I have investigated all the officially supported Flux installation methods and none of them is adding --watch-label-selector to source-watcher automatically, so if users are setting this flag then it's very intentionally, expecting it to work. Since the controller accepts the flag without crashing, those users are being silently deceived. These flags are a bespoke pattern among Flux users, people will set them without looking up the docs.
2. If any ArtifactGenerators out there are inheriting the sharding labels from somewhere (e.g. commonMetadata from KS/HR/RSET), then in the worst case the cluster administrator needs to add YAML for sharding source-watcher, tenants do not need to change their ArtifactGenerators. Also, any such ArtifactGenerators cannot be the source of the cluster sync if they are applied by Kustomizations, HelmReleases or ResourceSets, which means this breakage can be solved by the cluster administrator via GitOps, no kubectl access is needed.
3. "break all users of sharding" is not true: they need to be using source-watcher and need to be adding sharding labels to ArtifactGenerators. Three conditions need to be true: sharding, source-watcher, sharding labels in ArtifactGenerators. Additionally, flux bootstrap users also need to be explicitly adding --watch-label-selector to source-watcher, as flux bootstrap does not automatically do that (the sharding docs for flux bootstrap tell users to write the sharding config by hand). But, again, if users are explicitly adding the flag to source-watcher, they are expecting it to work and are being silently deceived, since the controller accepts the flag without crashing (point number 1). In this case their setup is broken now and this PR fixes it.
4. FluxInstance.spec.sync does not support ExternalArtifact.
5. flux bootstrap supports only GitRepository.
6. Out of the 6 shardable Flux controllers (notification-controller genuinely does not need it, as it can scale out), this is the only controller that does not support it (and yet advertises the flag).
7. We just implemented a feature that could add a significant overhead to the ArtifactGenerator reconciliation. Also, if source-controller needs sharding to pull large artifacts from outside the cluster, a controller processing these large artifacts will probably need a bit of compute as well. "This controller does not need sharding" is a very bold claim, nobody knows what features are coming ahead. Justifying this behavior skew between source-watcher and the other controllers based on this claim does not sound right to me at all.
8. This controller has been released only for two Flux minor releases. It makes absolutely no sense not to fix this bug now. We deserve leeway to make this (probably impactless!) breaking change. Other CNCF projects like Helm and SPIFFE/SPIRE ship big things like this (Helm Chart API v3, SPIFFE/SPIRE Broker API) under an experimental banner. We rushed and released this controller as "GA". This is a mistake we need to fix.

[stefanprodan]

LGTM

Thanks @matheuscscp

[fluxcdbot]

Successfully created backport PR for release/v2.2.x:

• [release/v2.2.x] Fix sharding flags not being honored #353