feat: Implement MacSec Controller by sven-rosenzweig · Pull Request #366 · ironcore-dev/network-operator

sven-rosenzweig · 2026-05-21T10:33:37Z

No description provided.

nikatza

This is a cool feature! Thanks for putting time into it :)

So I have lots very small comments that can be addressed quite easy. Other than that, I noticed that the NXOS provider has many inlined todos and fixmes. I am not sure what is the plan with those, so I ask here :)

For validation: I have noticed there are no gNMI test files (those txtars). Could you add those, please? And, do you think it would make sense to add some controller tests to verify the rotation works?

Again, this is really nice and I hope we can deploy soon!! 🚀

felix-kaestner · 2026-10-29T12:07:11Z

+}
+
+// CryptographicAlgorithm is the MKA key-derivation algorithm used for a pre-shared key.
+// +kubebuilder:validation:Enum=aes-128-cmac;aes-256-cmac


Suggested change

// +kubebuilder:validation:Enum=aes-128-cmac;aes-256-cmac

// +kubebuilder:validation:Enum=AES-128-CMAC;AES-256-CMAC

ref/ https://github.com/kubernetes/community/blob/main/contributors/devel/sig-architecture/api-conventions.md#constants

felix-kaestner · 2026-10-29T12:08:12Z

+	// Sort them in the order they should be used for redundancy rotation.
+	// +kubebuilder:validation:MinItems=1
+	// +required
+	PreSharedKeys []PreSharedKey `json:"preSharedKeys"`


Should this resource internally handle the rotation? I feel like the rotation of keys would be something a higher-level controller would orchestrate and is rather unusual to have on a Brick.

I would say no. A higher level resource should make sure that there is a valid key associated with the macsec connection. supporting multiple keys allows us to hitless rollover to the new key.

Implement layer2 encryption for Cisco IOS-XR and NXOS The MacSec resource is modeled as a tuple of: Device, Interface, Keys and a policy. Multiple secrets are supported to allow key rotation and rollover. Key secrets/passwords are obfuscated with Cisco Type 7 encryption. Follow up is to target the operational status of the keychain and macsec policy Signed-off-by: Sven Rosenzweig <sven.rosenzweig@sap.com>

Signed-off-by: Sven Rosenzweig <sven.rosenzweig@sap.com>

github-actions · 2026-11-03T16:32:03Z

Merging this branch changes the coverage (6 decrease, 4 increase)

Impacted Packages	Coverage Δ	🤖
github.com/ironcore-dev/network-operator/api/core/v1alpha1	2.35% (-0.03%)	👎
github.com/ironcore-dev/network-operator/cmd	0.00% (ø)
github.com/ironcore-dev/network-operator/internal/controller/cisco/nx	64.97% (-0.88%)	👎
github.com/ironcore-dev/network-operator/internal/controller/core	62.49% (+0.11%)	👍
github.com/ironcore-dev/network-operator/internal/controller/evpn	81.25% (+0.15%)	👍
github.com/ironcore-dev/network-operator/internal/controller/overlay	66.36% (-4.55%)	👎
github.com/ironcore-dev/network-operator/internal/controller/pool	75.11% (-0.42%)	👎
github.com/ironcore-dev/network-operator/internal/provider	1.75% (ø)
github.com/ironcore-dev/network-operator/internal/provider/cisco/encryption	88.89% (+88.89%)	🌟
github.com/ironcore-dev/network-operator/internal/provider/cisco/iosxr	35.78% (-4.61%)	👎
github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos	17.45% (-0.32%)	👎
github.com/ironcore-dev/network-operator/internal/webhook/core/v1alpha1	95.37% (+0.34%)	👍

Coverage details

_{Compared to commit 33b9ef5 (run #41658633865)}

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/ironcore-dev/network-operator/api/core/v1alpha1/groupversion_info.go	0.00% (ø)	2	0	2
github.com/ironcore-dev/network-operator/api/core/v1alpha1/macsec_types.go	20.00% (+20.00%)	5 (+5)	1 (+1)	4 (+4)	🎉
github.com/ironcore-dev/network-operator/api/core/v1alpha1/zz_generated.deepcopy.go	0.00% (ø)	3056 (+76)	0	3056 (+76)
github.com/ironcore-dev/network-operator/cmd/main.go	0.00% (ø)	331 (+6)	0	331 (+6)
github.com/ironcore-dev/network-operator/internal/controller/core/macsec_controller.go	69.52% (+69.52%)	210 (+210)	146 (+146)	64 (+64)	🌟
github.com/ironcore-dev/network-operator/internal/provider/cisco/encryption/type7.go	88.89% (+88.89%)	18 (+18)	16 (+16)	2 (+2)	🌟
github.com/ironcore-dev/network-operator/internal/provider/cisco/iosxr/intf.go	64.71% (ø)	68	44	24
github.com/ironcore-dev/network-operator/internal/provider/cisco/iosxr/macsec.go	0.00% (ø)	32 (+32)	0	32 (+32)
github.com/ironcore-dev/network-operator/internal/provider/cisco/iosxr/provider.go	16.14% (-1.61%)	316 (+40)	51 (+2)	265 (+38)	👎
github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos/macsec.go	0.00% (ø)	21 (+21)	0	21 (+21)
github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos/provider.go	4.23% (-0.08%)	2576 (+46)	109	2467 (+46)	👎
github.com/ironcore-dev/network-operator/internal/provider/provider.go	1.75% (ø)	57	1	56
github.com/ironcore-dev/network-operator/internal/transport/gnmiext/client.go	0.00% (ø)	0	0	0
github.com/ironcore-dev/network-operator/internal/transport/gnmiext/redact.go	0.00% (ø)	0	0	0
github.com/ironcore-dev/network-operator/internal/webhook/core/v1alpha1/macsec_webhook.go	100.00% (+100.00%)	19 (+19)	19 (+19)	0	🌟

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files:

github.com/ironcore-dev/network-operator/internal/controller/core/macsec_controller_test.go
github.com/ironcore-dev/network-operator/internal/controller/core/suite_test.go
github.com/ironcore-dev/network-operator/internal/provider/cisco/encryption/type7_test.go
github.com/ironcore-dev/network-operator/internal/webhook/core/v1alpha1/macsec_webhook_test.go
github.com/ironcore-dev/network-operator/internal/webhook/core/v1alpha1/webhook_suite_test.go
github.com/ironcore-dev/network-operator/test/gnmi/gnmi_suite_test.go

nikatza · 2026-11-03T16:29:46Z

@@ -0,0 +1,181 @@
+// SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors


We should remove the year from here

nikatza · 2026-11-03T16:37:36Z

+    "stp-items": {
+      "inst-items": {
+        "if-items": {
+          "If-list": [
+            {
+              "bpdufilter": "default",
+              "bpduguard": "default",
+              "id": "eth1/1",
+              "mode": "default"
+            }
+          ]
+        }
+      }


I am wondering if this piece of config should still be here... is it expected or are we missing some delete?

nikatza · 2026-11-03T16:38:51Z

+	"github.com/ironcore-dev/network-operator/api/core/v1alpha1"
+)
+
+// log is for logging in this package.


Suggested change

// log is for logging in this package.

// macseclog is for logging in this package.

nikatza · 2026-11-03T16:58:55Z

+type KeyValidity struct {
+	KeyName string
+	Valid   bool
+}


Do you think we need a OperStatus at all? I understand that keys are either configured or not, but they can't be operational per-se (even the models don't expose that). In any case, they should have been rejected by CEL/webhook. So I am wondering if the intention of this is to cover that a key has expired since it was installed? If so may be this needs to be clarified (the naming might be confusing me).

(The initial reason for the OperStatus is that we usually just transport this value from the device, and not create one in the provider, happy to sync about this)

nikatza · 2026-11-03T17:01:49Z

+		lifetime := NewLifetime(psk.StartTime.Time, psk.EndTime.Time)
+		cryptoAlg, err := ExtractCryptographicAlgo(psk.Algorithm)
+		if err != nil {
+			return err


This should probably return an InvalidArgument error from apiStatus, since this is a configuration that a user needs to fix.

nikatza · 2026-11-03T17:11:17Z

+ - source: # Uncomment the following block if you have a DefaultingWebhook (--defaulting )
+     kind: Certificate
+     group: cert-manager.io
+     version: v1
+     name: serving-cert
+     fieldPath: .metadata.namespace # Namespace of the certificate CR
+   targets:
+     - select:
+         kind: MutatingWebhookConfiguration
+       fieldPaths:
+         - .metadata.annotations.[cert-manager.io/inject-ca-from]
+       options:
+         delimiter: '/'
+         index: 0
+         create: true
+ - source:
+     kind: Certificate
+     group: cert-manager.io
+     version: v1
+     name: serving-cert
+     fieldPath: .metadata.name
+   targets:
+     - select:
+         kind: MutatingWebhookConfiguration
+       fieldPaths:
+         - .metadata.annotations.[cert-manager.io/inject-ca-from]
+       options:
+         delimiter: '/'
+         index: 1
+         create: true


Is this change actually needed?

nikatza · 2026-11-03T17:18:59Z

+
+	// EndTime is when this key expires. Must be after startTime.
+	// +required
+	// Validates if the string is a valid datetime in RFC3339 format.


The CEL validation is still a cheap thing we could do (while keeping the webhook, which is already useful).

nikatza · 2026-11-03T17:28:26Z

+// +kubebuilder:printcolumn:name="Device",type="string",JSONPath=".spec.deviceRef.name",description="The device this MacSec is configured on"
+// +kubebuilder:printcolumn:name="Interface",type="string",JSONPath=".spec.interfaceRef.name",description="The interface this MacSec is configured on"
+// +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status",description="Ready status of the MacSec"
+// +kubebuilder:printcolumn:name="Configured",type=string,JSONPath=`.status.conditions[?(@.type=="Configured")].status`,priority=1


Looking at the changes done to the provider, I wonder what happens when a key expires on the device. Validation webhook won't be called. We essentially detect the drift and try to reinstall the keys (since the controller doesnt validate that either). The device rejects the keys and reenqueue indefinitely (updating the ready condition in the meantime). I am doing several assumptions and might be wrong here. Do you have any thoughts on this regard?

nikatza · 2026-11-03T17:29:08Z

@@ -0,0 +1,557 @@
+// SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors


Year should be removed here too :)

nikatza · 2026-11-03T17:33:01Z

+		return ctrl.Result{}, apistatus.WrapTerminalError(err)
+	}
+
+	return ctrl.Result{RequeueAfter: Jitter(r.RequeueInterval)}, nil


Should this be a simple ctrl.Result{}? Since there are no errors we should avoid requeuing the resource.

nikatza

Are we missing a macsec.txtar fuer iosxr?

Thanks for your last update, the redacted part is a nice touch!

github-actions Bot added the size/XXL label May 21, 2026

sven-rosenzweig force-pushed the feat/macsec branch from 8163a65 to 90d2868 Compare May 21, 2026 10:35

hardikdr added the area/switch-automation Automation processes for network switch management and operations. label May 22, 2026

hardikdr added this to Roadmap May 22, 2026

felix-kaestner force-pushed the main branch from a8600a2 to 62b696c Compare June 4, 2026 08:19

sven-rosenzweig force-pushed the feat/macsec branch from 90d2868 to 0ec655e Compare September 29, 2026 15:48

sven-rosenzweig force-pushed the feat/macsec branch 2 times, most recently from 8b93d0e to a138d40 Compare October 12, 2026 15:46

github-advanced-security Bot found potential problems Oct 12, 2026

View reviewed changes

Comment thread internal/transport/gnmiext/client.go Fixed

sven-rosenzweig force-pushed the feat/macsec branch from a138d40 to 5a7a6b5 Compare October 16, 2026 12:36

felix-kaestner assigned nikatza Oct 22, 2026

felix-kaestner added the review-scheduled The PR is being reviewed. label Oct 22, 2026

sven-rosenzweig force-pushed the feat/macsec branch 5 times, most recently from b2a91f3 to 7f0c17c Compare October 23, 2026 10:47

sven-rosenzweig marked this pull request as ready for review October 23, 2026 10:54

sven-rosenzweig force-pushed the feat/macsec branch from 7f0c17c to dd5de75 Compare October 23, 2026 11:26

nikatza reviewed Oct 28, 2026

View reviewed changes

felix-kaestner reviewed Oct 29, 2026

View reviewed changes

sven-rosenzweig force-pushed the feat/macsec branch 2 times, most recently from 267e582 to 93ff0d4 Compare November 2, 2026 14:32

sven-rosenzweig force-pushed the feat/macsec branch from 93ff0d4 to 1517d8f Compare November 2, 2026 14:37

fixup! feat: Implement MACsec Controller

9c74ab1

Signed-off-by: Sven Rosenzweig <sven.rosenzweig@sap.com>

sven-rosenzweig force-pushed the feat/macsec branch 4 times, most recently from a39f102 to 3a818fe Compare November 3, 2026 16:09

squash: macsex txttar test

1e0cd8a

sven-rosenzweig force-pushed the feat/macsec branch from 3a818fe to 1e0cd8a Compare November 3, 2026 16:25

nikatza reviewed Nov 3, 2026

View reviewed changes

	// +kubebuilder:validation:Enum=aes-128-cmac;aes-256-cmac
	// +kubebuilder:validation:Enum=AES-128-CMAC;AES-256-CMAC

		@@ -0,0 +1,181 @@
		// SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors

	// log is for logging in this package.
	// macseclog is for logging in this package.

		@@ -0,0 +1,557 @@
		// SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors

Conversation

sven-rosenzweig commented May 21, 2026

Uh oh!

Uh oh!

nikatza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 3, 2026

Merging this branch changes the coverage (6 decrease, 4 increase)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikatza Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikatza Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikatza left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nikatza Nov 3, 2026 •

edited

Loading

nikatza Nov 3, 2026 •

edited

Loading