NETOBSERV-1811 set namespace ownership

[jpinsonneau]

Description

Set namespace ownership using a new netobserv-managed: true label so users can bypass reconcile loop if they want to customize labels by setting it to false.

The ownerReference is not set to namespace object to avoid unexpected deletions.

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

• Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
• Does this PR require product documentation?
  • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
• Does this PR require a product release notes entry?
  • If so, fill in "Release Note Text" in the JIRA.
• Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
  • If so, make sure it is described in the JIRA ticket.
• QE requirements (check 1 from the list):
  • Standard QE validation, with pre-merge tests unless stated otherwise.
  • Regression tests only (e.g. refactoring with no user-facing change).
  • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jpinsonneau. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[memodi]

/ok-to-test

[github-actions]

New images:

• quay.io/netobserv/network-observability-operator:b643c95
• quay.io/netobserv/network-observability-operator-bundle:v0.0.0-b643c95
• quay.io/netobserv/network-observability-operator-catalog:v0.0.0-b643c95

They will expire after two weeks.

To deploy this build:

# Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:b643c95 make deploy

# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-b643c95

Or as a Catalog Source:

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: netobserv-dev
  namespace: openshift-marketplace
spec:
  sourceType: grpc
  image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-b643c95
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

[memodi]

@jpinsonneau - I tested this and it works. The custom labels and modifications does persist after this change.

Do you think it would be useful to add regression test for this bug?

[jpinsonneau]

@jpinsonneau - I tested this and it works. The custom labels and modifications does persist after this change.

Do you think it would be useful to add regression test for this bug?

I don't feel it's mandatory but it's good to have especially since we are going to start documenting aroud this behavior to solve the customer case linked in the ticket.

FYI this apply on any yaml owned by the controller, not only namespace.

[jotak]

Hold on, what happens with this change if we delete the FlowCollector => the namespace will be deleted as well, right? That could be too brutal. E.g. if a user installed Loki in the same namespace, they'd loose it, including its PVC etc.

[jotak]

An alternative to setting ownership could be here to just extract the extra labels and reinject them in the desired namespace object ? wdyt?

[jpinsonneau]

An alternative to setting ownership could be here to just extract the extra labels and reinject them in the desired namespace object ? wdyt?

So if you simply invert the DownstreamDeployment flag it will not reconcile anymore ?
In that case maybe just add a netobserv-managed label in the namespace and check of its value before reconciling ?
Why not doing that for any object we manage ?

The console usually asks you if you want to cascade or not the deletion but I don't think it's the case using kubectl / oc commands.
Can't we implement a smart finalizer here ? 🤔
https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/#ownership-and-finalizers
https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/

[jotak]

An alternative to setting ownership could be here to just extract the extra labels and reinject them in the desired namespace object ? wdyt?

So if you simply invert the DownstreamDeployment flag it will not reconcile anymore ?

Not sure to get what you mean.. If we change openshift.io/cluster-monitoring it would still be reconciled and reverted to what we want, I don't mean any regression about that. Only the labels that we don't set ourselves would be reinjected. Something like that:

		for k, v := range nsExist.ObjectMeta.Labels {
			if _, exists := desiredNs.ObjectMeta.Labels[k]; !exists {
				desiredNs.ObjectMeta.Labels[k] = v
			}
		}

In that case maybe just add a netobserv-managed label in the namespace and check of its value before reconciling ? Why not doing that for any object we manage ?

I'm not against that but I think it's a different thing. Here reading @stleerh 's ticket, I don't think he wants to make the namespace unmanaged. It's more a matter of having it co-managed, we want to manage the bits that we need but without deleting stuff written by others. In fact, we already do that for some resources, like Services: on update, we don't recreate them totally from scratch, we keep some existing bits. The 5 lines of code above would just do that.

[jpinsonneau]

@jotak following our discussion I reworked the code to implement a netobserv-managed label.
Reconciliation is ignored if it's explicitly set to false.
Let me know if that works better for you !

@memodi sorry we changed the way it's implemented here. Let's validate the approach before doing QE validation.

[jotak]

merging for branch cut, to be verified post-merge