ideally, we would enqueue only when our configured lokistack is affected, not all lokistacks
but I guess that's fine, as we don't expect hundreds of lokistacks out there :-)

from Claude review:

  Problem: This will create reconcile requests for LokiStack objects, not FlowCollectors. When a LokiStack named "logging-loki" changes, it will try to reconcile a FlowCollector named "logging-loki", which likely doesn't exist.

  Fix Required:
  builder.Watches(&lokiv1.LokiStack{}, handler.EnqueueRequestsFromMapFunc(
      func(ctx context.Context, obj client.Object) []reconcile.Request {
          lokiStack := obj.(*lokiv1.LokiStack)
          var flowCollectors flowslatest.FlowCollectorList
          if err := mgr.GetClient().List(ctx, &flowCollectors); err != nil {
              log.FromContext(ctx).Error(err, "Failed to list FlowCollectors")
              return []reconcile.Request{}
          }

          var requests []reconcile.Request
          for _, fc := range flowCollectors.Items {
              if fc.Spec.Loki.Mode == flowslatest.LokiModeLokiStack &&
                 fc.Spec.Loki.LokiStack.Name == lokiStack.Name {
                  ns := fc.Spec.Loki.LokiStack.Namespace
                  if ns == "" {
                      ns = fc.Namespace
                  }
                  if ns == lokiStack.Namespace {
                      requests = append(requests, reconcile.Request{
                          NamespacedName: types.NamespacedName{
                              Name:      fc.Name,
                              Namespace: fc.Namespace,
                          },
                      })
                  }
              }
          }
          return requests
      },
  ))

I did not think about making a k8s query inside the handler to filter Lokistacks.

The approach I thought was to wait for the flowcollector to be created and start a dedicated controller with a static flowcollector name. This was adding a lot of complexity and I was not sure if it was worth it.

This looks like a more simple solution, to the price of a k8s querry in the handler function.

@jotak what do you think ?

Not sure we need to overcomplicate things here. It's basically my comment here: #2142 (comment) ; having a couple of false-positive reconcile events is not so important; we're talking about lokistack objects, it's not expected to have many, and they don't change often.

btw I think claude answer is wrong the enqueue request is not for a flow-collector named after the loki stack, it's for any flow-collector? (EnqueueRequestForObject{} with empty params)

If we really want to narrow down to our configured lokistack, in other situation we just keep in controller state the last-time seen element that we want to check (we do that in a couple of places for flowcollector.spec.namespace iirc); we could do the same with the configured lokistack.
Like this: https://github.com/netobserv/network-observability-operator/blob/main/internal/controller/flp/flp_controller.go#L60

from Claude review:

  Problems:
  - Logs at Info level instead of Warning
  - Actual error is discarded
  - Can't distinguish between "not found" and "permission denied"
  - Should surface in FlowCollector status

  Fix:
  if err := r.Client.Get(ctx, types.NamespacedName{Name: desired.Loki.LokiStack.Name, Namespace: ns}, lokiStack); err != nil {
      lokiStack = nil
      if apierrors.IsNotFound(err) {
          log.FromContext(ctx).Info("LokiStack resource not found, status will not be available",
              "name", desired.Loki.LokiStack.Name,
              "namespace", ns)
      } else {
          log.FromContext(ctx).Error(err, "Failed to get LokiStack resource",
              "name", desired.Loki.LokiStack.Name,
              "namespace", ns)
      }
      // TODO: Consider surfacing this in FlowCollector status
  }

not tested, but looking at the code I think there's a problem here:
In reconcileConfigMap, the lokiStack object is created and can have the following values:

• a reference to the lokistack that was found
• nil if an error occurred
• a reference to an empty (0-value) struct &lokiv1.LokiStack{} when not in LokiStack mode

So in this last situation, the returned value would be pending, which seems incorrect?

IMO, what we could do:

• if there was an error when fetching LokiStack, set this error as status (e.g. the console plugin could display something like "LokiStack not found")
• if the lokistack shows a non-ready status (error/pending condition), set a message with that condition status
• if the loki stack is ready, set as ready
• if not in lokistack mode, set as an empty string

Also, I'm not sure it's useful to check for the presence of the LokiStack API: if it's configured in LokiStack mode BUT the API is not present, the config is wrong, so it's ok to just display the error message that would come up when trying to fetch LokiStack?
wdyt?

In theory, we should not enter the case of not being in LokiStack, but since we are passing LokiStack as a pointer I addded a not nil check. I modified the function to make this more understandable.

When not in LokiStack mode, the operator fallback to the previous mode, using the StatusURL.

If the lokistack shows a non-ready status, finding the right error might be tricky, each LokiStack component has its own status meaning we may display the wrong error. IMO this is simpler to display a pending status and let the user investigate, the LokiStack was provided by the user.

I removed the LokiStack API check.

I just double-checked trying your PR, and confirm it provides a wrong status when not in LokiStack mode (it says pending) ; that's because you initialize it as non-nil.

This should work I think: