Skip to content

networks: Add Owner annotation to NetAttachDefs #897

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 10, 2025
Merged

networks: Add Owner annotation to NetAttachDefs #897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions api/v1/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
intstrutil "k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/client-go/kubernetes"
"sigs.k8s.io/controller-runtime/pkg/client"
logf "sigs.k8s.io/controller-runtime/pkg/log"

"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
Expand Down Expand Up @@ -651,6 +652,7 @@ func (cr *SriovIBNetwork) RenderNetAttDef() (*uns.Unstructured, error) {
} else {
data.Data["SriovNetworkNamespace"] = cr.Spec.NetworkNamespace
}
data.Data["Owner"] = OwnerRefToString(cr)
data.Data["SriovCniResourceName"] = os.Getenv("RESOURCE_PREFIX") + "/" + cr.Spec.ResourceName

data.Data["StateConfigured"] = true
Expand Down Expand Up @@ -719,6 +721,7 @@ func (cr *SriovNetwork) RenderNetAttDef() (*uns.Unstructured, error) {
} else {
data.Data["SriovNetworkNamespace"] = cr.Spec.NetworkNamespace
}
data.Data["Owner"] = OwnerRefToString(cr)
data.Data["SriovCniResourceName"] = os.Getenv("RESOURCE_PREFIX") + "/" + cr.Spec.ResourceName
data.Data["SriovCniVlan"] = cr.Spec.Vlan

Expand Down Expand Up @@ -837,6 +840,7 @@ func (cr *OVSNetwork) RenderNetAttDef() (*uns.Unstructured, error) {
} else {
data.Data["NetworkNamespace"] = cr.Spec.NetworkNamespace
}
data.Data["Owner"] = OwnerRefToString(cr)
data.Data["CniResourceName"] = os.Getenv("RESOURCE_PREFIX") + "/" + cr.Spec.ResourceName

if cr.Spec.Capabilities == "" {
Expand Down Expand Up @@ -994,3 +998,14 @@ func (s *SriovNetworkNodeState) ResetKeepUntilTime() bool {
s.SetAnnotations(annotations)
return true
}

func OwnerRefToString(cr client.Object) string {
if cr == nil {
return "owner-object-not-found"
}
if cr.GetObjectKind().GroupVersionKind().Empty() {
return "unknown/" + cr.GetNamespace() + "/" + cr.GetName()
}

return cr.GetObjectKind().GroupVersionKind().GroupKind().String() + "/" + cr.GetNamespace() + "/" + cr.GetName()
}
40 changes: 19 additions & 21 deletions api/v1/helper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
intstrutil "k8s.io/apimachinery/pkg/util/intstr"

Expand Down Expand Up @@ -149,6 +150,8 @@ func TestRendering(t *testing.T) {
{
tname: "simple",
network: v1.SriovNetwork{
TypeMeta: metav1.TypeMeta{APIVersion: v1.GroupVersion.String(), Kind: "SriovNetwork"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "test"},
Spec: v1.SriovNetworkSpec{
NetworkNamespace: "testnamespace",
ResourceName: "testresource",
Expand All @@ -158,6 +161,8 @@ func TestRendering(t *testing.T) {
{
tname: "chained",
network: v1.SriovNetwork{
TypeMeta: metav1.TypeMeta{APIVersion: v1.GroupVersion.String(), Kind: "SriovNetwork"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "test"},
Spec: v1.SriovNetworkSpec{
NetworkNamespace: "testnamespace",
ResourceName: "testresource",
Expand Down Expand Up @@ -194,10 +199,8 @@ func TestRendering(t *testing.T) {
if err != nil {
t.Fatalf("failed reading .golden: %s", err)
}
t.Log(b.String())
if !bytes.Equal(b.Bytes(), g) {
t.Errorf("bytes do not match .golden file")
}

assert.Equal(t, string(g), b.String(), "bytes do not match .golden file [%s]", gp)
})
}
}
Expand All @@ -210,6 +213,8 @@ func TestIBRendering(t *testing.T) {
{
tname: "simpleib",
network: v1.SriovIBNetwork{
TypeMeta: metav1.TypeMeta{APIVersion: v1.GroupVersion.String(), Kind: "SriovIBNetwork"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "test"},
Spec: v1.SriovIBNetworkSpec{
NetworkNamespace: "testnamespace",
ResourceName: "testresource",
Expand Down Expand Up @@ -241,10 +246,8 @@ func TestIBRendering(t *testing.T) {
if err != nil {
t.Fatalf("failed reading .golden: %s", err)
}
t.Log(b.String())
if !bytes.Equal(b.Bytes(), g) {
t.Errorf("bytes do not match .golden file")
}

assert.Equal(t, string(g), b.String(), "bytes do not match .golden file [%s]", gp)
})
}
}
Expand All @@ -257,9 +260,8 @@ func TestOVSRendering(t *testing.T) {
{
tname: "simpleovs",
network: v1.OVSNetwork{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
},
TypeMeta: metav1.TypeMeta{APIVersion: v1.GroupVersion.String(), Kind: "OVSNetwork"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "test"},
Spec: v1.OVSNetworkSpec{
NetworkNamespace: "testnamespace",
ResourceName: "testresource",
Expand All @@ -269,9 +271,8 @@ func TestOVSRendering(t *testing.T) {
{
tname: "chained",
network: v1.OVSNetwork{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
},
TypeMeta: metav1.TypeMeta{APIVersion: v1.GroupVersion.String(), Kind: "OVSNetwork"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "test"},
Spec: v1.OVSNetworkSpec{
NetworkNamespace: "testnamespace",
ResourceName: "testresource",
Expand All @@ -288,9 +289,8 @@ func TestOVSRendering(t *testing.T) {
{
tname: "complexconf",
network: v1.OVSNetwork{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
},
TypeMeta: metav1.TypeMeta{APIVersion: v1.GroupVersion.String(), Kind: "OVSNetwork"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "test"},
Spec: v1.OVSNetworkSpec{
NetworkNamespace: "testnamespace",
ResourceName: "testresource",
Expand Down Expand Up @@ -334,10 +334,8 @@ func TestOVSRendering(t *testing.T) {
if err != nil {
t.Fatalf("failed reading .golden: %s", err)
}
t.Log(b.String())
if !bytes.Equal(b.Bytes(), g) {
t.Errorf("bytes do not match .golden file")
}

assert.Equal(t, string(g), b.String(), "bytes do not match .golden file [%s]", gp)
})
}
}
Expand Down
7 changes: 4 additions & 3 deletions api/v1/testdata/TestIBRendering/simpleib.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
"kind": "NetworkAttachmentDefinition",
"metadata": {
"annotations": {
"k8s.v1.cni.cncf.io/resourceName": "/testresource"
"k8s.v1.cni.cncf.io/resourceName": "/testresource",
"sriovnetwork.openshift.io/owner-ref": "SriovIBNetwork.sriovnetwork.openshift.io/ns/test"
},
"name": null,
"name": "test",
"namespace": "testnamespace"
},
"spec": {
"config": "{ \"cniVersion\":\"1.0.0\", \"name\":\"\",\"type\":\"ib-sriov\",\"capabilities\":foo,\"ipam\":{} }"
"config": "{ \"cniVersion\":\"1.0.0\", \"name\":\"test\",\"type\":\"ib-sriov\",\"capabilities\":foo,\"ipam\":{} }"
}
}
3 changes: 2 additions & 1 deletion api/v1/testdata/TestOVSRendering/chained.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"kind": "NetworkAttachmentDefinition",
"metadata": {
"annotations": {
"k8s.v1.cni.cncf.io/resourceName": "/testresource"
"k8s.v1.cni.cncf.io/resourceName": "/testresource",
"sriovnetwork.openshift.io/owner-ref": "OVSNetwork.sriovnetwork.openshift.io/ns/test"
},
"name": "test",
"namespace": "testnamespace"
Expand Down
3 changes: 2 additions & 1 deletion api/v1/testdata/TestOVSRendering/complexconf.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"kind": "NetworkAttachmentDefinition",
"metadata": {
"annotations": {
"k8s.v1.cni.cncf.io/resourceName": "/testresource"
"k8s.v1.cni.cncf.io/resourceName": "/testresource",
"sriovnetwork.openshift.io/owner-ref": "OVSNetwork.sriovnetwork.openshift.io/ns/test"
},
"name": "test",
"namespace": "testnamespace"
Expand Down
3 changes: 2 additions & 1 deletion api/v1/testdata/TestOVSRendering/simpleovs.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"kind": "NetworkAttachmentDefinition",
"metadata": {
"annotations": {
"k8s.v1.cni.cncf.io/resourceName": "/testresource"
"k8s.v1.cni.cncf.io/resourceName": "/testresource",
"sriovnetwork.openshift.io/owner-ref": "OVSNetwork.sriovnetwork.openshift.io/ns/test"
},
"name": "test",
"namespace": "testnamespace"
Expand Down
7 changes: 4 additions & 3 deletions api/v1/testdata/TestRendering/chained.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
"kind": "NetworkAttachmentDefinition",
"metadata": {
"annotations": {
"k8s.v1.cni.cncf.io/resourceName": "/testresource"
"k8s.v1.cni.cncf.io/resourceName": "/testresource",
"sriovnetwork.openshift.io/owner-ref": "SriovNetwork.sriovnetwork.openshift.io/ns/test"
},
"name": null,
"name": "test",
"namespace": "testnamespace"
},
"spec": {
"config": "{ \"cniVersion\":\"1.0.0\", \"name\":\"\",\"plugins\": [ {\"type\":\"sriov\",\"vlan\":0,\"vlanQoS\":0,\"ipam\":{} },\n{ \"type\": \"vrf\", \"vrfname\": \"blue\" }\n] }"
"config": "{ \"cniVersion\":\"1.0.0\", \"name\":\"test\",\"plugins\": [ {\"type\":\"sriov\",\"vlan\":0,\"vlanQoS\":0,\"ipam\":{} },\n{ \"type\": \"vrf\", \"vrfname\": \"blue\" }\n] }"
}
}
7 changes: 4 additions & 3 deletions api/v1/testdata/TestRendering/simple.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
"kind": "NetworkAttachmentDefinition",
"metadata": {
"annotations": {
"k8s.v1.cni.cncf.io/resourceName": "/testresource"
"k8s.v1.cni.cncf.io/resourceName": "/testresource",
"sriovnetwork.openshift.io/owner-ref": "SriovNetwork.sriovnetwork.openshift.io/ns/test"
},
"name": null,
"name": "test",
"namespace": "testnamespace"
},
"spec": {
"config": "{ \"cniVersion\":\"1.0.0\", \"name\":\"\",\"type\":\"sriov\",\"vlan\":0,\"vlanQoS\":0,\"ipam\":{} }"
"config": "{ \"cniVersion\":\"1.0.0\", \"name\":\"test\",\"type\":\"sriov\",\"vlan\":0,\"vlanQoS\":0,\"ipam\":{} }"
}
}
1 change: 1 addition & 0 deletions bindata/manifests/cni-config/ovs/ovs-cni-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ metadata:
namespace: {{.NetworkNamespace}}
annotations:
k8s.v1.cni.cncf.io/resourceName: {{.CniResourceName}}
sriovnetwork.openshift.io/owner-ref: {{.Owner}}
spec:
config: '{
"cniVersion":"1.0.0",
Expand Down
1 change: 1 addition & 0 deletions bindata/manifests/cni-config/sriov/sriov-cni-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ metadata:
namespace: {{.SriovNetworkNamespace}}
annotations:
k8s.v1.cni.cncf.io/resourceName: {{.SriovCniResourceName}}
sriovnetwork.openshift.io/owner-ref: {{.Owner}}
spec:
config: '{
"cniVersion":"1.0.0",
Expand Down
15 changes: 15 additions & 0 deletions controllers/generic_network_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"

sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1"
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/utils"
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars"
)
Expand Down Expand Up @@ -189,6 +190,20 @@ func (r *genericNetworkReconciler) Reconcile(ctx context.Context, req ctrl.Reque
}
} else {
reqLogger.Info("NetworkAttachmentDefinition CR already exist")

foundOwner := found.GetAnnotations()[consts.OwnerRefAnnotation]
expectedOwner := netAttDef.GetAnnotations()[consts.OwnerRefAnnotation]

// Note for the future: the `foundOwner != ""` condition can be removed to make the operator not touching the NetworkAttachmentDefinition created
// by the user.
if foundOwner != "" && foundOwner != expectedOwner {
Copy link
Collaborator

@SchSeba SchSeba Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a quick question if we have !equality.Semantic.DeepEqual(found.GetAnnotations(), netAttDef.GetAnnotations() below do we still need this validation here?

Copy link
Member Author

@zeeke zeeke Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we do:

in case we have:

kind: NetworkAttachmentDefinition
metadata:
  name: foo
  namespace: foo-ns
  annotations:
     owner-ref: SriovNetwork/sriov-operator-ns/foo
spec:
  <sriov cni config>

which comes from an SriovNetwork in the sriov-operator-ns namespace

when reconciling another network object:

kind: SriovNetwork
metadata:
  name: foo
  namespace: foo-ns
spec: ...

the generated net-attach-def has owner-ref: SriovNetwork/foo-ns/foo, which would make the condition !equality.Semantic.DeepEqual(found.GetAnnotations(), netAttDef.GetAnnotations() true and the netAttachDef woudl be overridden.

The policy, as of now, is the first reconciler networkobject is the owner

reqLogger.Info("A NetworkAttachmentDefinition with the same name already exists and it does not belong to this resource",
"Namespace", netAttDef.Namespace, "Name", netAttDef.Name,
"CurrentOwner", foundOwner, "ExpectedOwner", expectedOwner,
)
return reconcile.Result{}, nil
}

if !equality.Semantic.DeepEqual(found.Spec, netAttDef.Spec) || !equality.Semantic.DeepEqual(found.GetAnnotations(), netAttDef.GetAnnotations()) {
reqLogger.Info("Update NetworkAttachmentDefinition CR", "Namespace", netAttDef.Namespace, "Name", netAttDef.Name)
netAttDef.SetResourceVersion(found.GetResourceVersion())
Expand Down
Loading
Loading