Implement Openconfig NTP provider by adamtrizuljak-sap · Pull Request #517 · ironcore-dev/network-operator

adamtrizuljak-sap · 2026-08-21T11:34:37Z

Work in progress

Add OpenConfig provider implementation for the NTP resource (internal/provider/openconfig/ntp.go) and a gnmi testdata file (test/gnmi/testdata/openconfig/ntp.txtar).
~~Add optional field NTPSpec.SourceAddress and validation logic (see below)~~
Get the IP address of the NTPSpec.SourceInterface from the device using the GRPC client and use it to populate the source-address field in Openconfig
Add a simple Containerlab setup with one SRLinux node + the associated network-operator Device and NTP resources

Applying examples/openconfig-containerlab/kubernetes/01-devices/v1alpha1-leaf1-clab-ntp.yaml produces the following config in SRLinux:

--{ + running }--[  ]--
A:admin@srl# info system ntp | as json
{
  "admin-state": "enable",
  "server": [
    {
      "address": "de.pool.ntp.org",
      "prefer": true,
      "network-instance": "mgmt",
      "source-address": "172.20.20.2"
    },
    {
      "address": "pool.ntp.org",
      "network-instance": "mgmt",
      "source-address": "172.20.20.2"
    }
  ]
}

Cisco vs Openconfig compatibility issue (solved)

NTPSpec defines the SourceInterfaceName field https://github.com/ironcore-dev/network-operator/blob/main/api/core/v1alpha1/ntp_types.go#L36. Openconfig has something similar source-address https://openconfig.net/projects/models/schemadocs/yangdoc/openconfig-system.html#system-ntp-servers-server-config-source-address

A way to handle it would be to look up the interface given by SourceInterfaceName , get its IPv4 address and put it into source-address. Does it make sense? Do you usually do such operations, or do you keep the provider code clean from reading any additional k8s resources?

Other way would be to just ignore SourceInterfaceName and not pass it into the Openconfig struct, or even throw a apistatus.FieldViolation saying that it's unsupported

Solution

After discussion we decided to do the original proposal and obtain the IP address from the interface. However, if the management interface is used for NTP we can't obtain the address from an Interface resource, because we don't create such resource for the management interface. Therefore the provider reads the interface state directly from the device over GRPC and uses its IP address to populate the source-address field.

This solution does not change the NTPSpec and should work with any SourceInterfaceName that has an IP address (either manually configured, or obtained from DHCP).

Previous solution - abandoned

~~Discussed with @nikatza and @rgildein~~

Mark NTPSpec.SourceInterfaceName as optional

Add optional field NTPSpec.SourceAddress

Maps to https://pubhub.devnetcloud.com/media/dme-docs-10-4-3/docs/System/datetime:NtpSrcIp/ in Cisco

Maps to https://openconfig.net/projects/models/schemadocs/yangdoc/openconfig-system.html#system-ntp-servers-server-config-source-address in Openconfig

In Cisco controller, ensure that at least one of the fields is defined

In Openconfig controller, throw an unsupported error if SourceInterfaceName is defined

~~Since both fields are optional, the change will be backwards-compatible with the previous NTPSpec and the generated custom resources.~~

felix-kaestner · 2026-09-03T14:40:40Z

 	Prefer bool `json:"prefer,omitempty"`

 	// The name of the vrf used to communicate with the NTP server.
+	// Maps to NetworkInstance in Openconfig. If empty, will be set to `mgmt`.


We try to avoid such vendor specifics in our core apis, as these are meant to be vendor-agnostic and work across all vendors.

Also note that the management vrf is called differently on different platforms, so it might not always be called "mgmt".

This was done specifically for Nokia, but you're right that it should be as much vendor-agnostic as possible. Especially since we will likely be implementing the Nokia provider soon and therefore the Openconfig implementation should remain as pure as possible. I'll remove this.

On the other hand, the name vrfName is already vendor-specific :)

I wouldn't say that vrfName is vendor specific. The term VRF is quite universally known, the term is even used in the linux kernel (https://docs.kernel.org/networking/vrf.html). And it also refers to the VRF resource type that we have defined (https://github.com/ironcore-dev/network-operator/blob/main/api/core/v1alpha1/vrf_types.go) within it's spec.name.

- Add OpenConfig provider implementation for the NTP resource (`internal/provider/openconfig/ntp.go`) and a gnmi testdata file (`test/gnmi/testdata/openconfig/ntp.txtar`) - Get the IP address of the `NTPSpec.SourceInterface` from the device using the GRPC client and use it to populate the `source-address` field in Openconfig - Add a simple Containerlab setup with one SRLinux node + the associated network-operator `NTP` resource If the management interface is used for NTP we can't obtain the address from an `Interface` resource, because we don't create such resource for the management interface. Therefore the provider reads the interface state directly from the device over GRPC and uses its IP address to populate the `source-address` field. Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>

github-actions · 2026-09-08T10:04:35Z

Merging this branch will decrease overall coverage

Impacted Packages	Coverage Δ	🤖
github.com/ironcore-dev/network-operator/api/core/v1alpha1	2.52% (ø)
github.com/ironcore-dev/network-operator/internal/provider/openconfig	7.64% (-0.33%)	👎

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/ironcore-dev/network-operator/api/core/v1alpha1/ntp_types.go	12.50% (ø)	8	1	7
github.com/ironcore-dev/network-operator/internal/provider/openconfig/interface.go	10.22% (-0.72%)	137 (+9)	14	123 (+9)	👎
github.com/ironcore-dev/network-operator/internal/provider/openconfig/ntp.go	0.00% (ø)	16 (+16)	0	16 (+16)

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.

nikatza

I have some minor comments, happy to discuss them if needed :) 🚀

nikatza · 2026-09-08T11:26:16Z

 	Prefer bool `json:"prefer,omitempty"`

 	// The name of the vrf used to communicate with the NTP server.
+	// Maps to network-instance in Openconfig


Do we need this comment here? It does seem to bring any relevant information for the API user at first. May be the commit message or the provider implementation are more appropriate place.

nikatza · 2026-09-08T11:32:10Z

@@ -0,0 +1,19 @@
+apiVersion: networking.metal.ironcore.dev/v1alpha1


I fear these new files included in this folder as they don't really fit well here in this PR. I think the examples folder is reserved to more complex scenarios. If that is OK for you we could remove them and clarify in our weekly. What do you think?

nikatza · 2026-09-08T11:46:43Z

+}
+
+func (a *interfaceAddrs) XPath() string {
+	return fmt.Sprintf("openconfig-interfaces:interfaces/interface[name=%s]/subinterfaces/subinterface[index=0]/openconfig-if-ip:ipv4/addresses", a.ifName)


We have two hard-coded elements here: the index (0) and the AFI (IPv4). I think we should be able to fetch IPv6 or indicate the address family we want. Same goes for the index. What we could do is add these fields in interfaceAddrs (with tag json:"-") and build our xpath using those values.

If so the provider should be changed accordingly, e.g., use index 0 when the interface name matches some constraints (or just document why 0 is hard-coded). For the IPv4 and IPv6 AFI: you could make that dependent on the AFI of the NTPServers in the spec.

nikatza · 2026-09-08T11:47:28Z

+}
+
+// interfaceIPAddr retrieves the first IPv4 address from the state of the named interface.
+func (p *Provider) interfaceIPAddr(ctx context.Context, name string) (string, error) {


We should be able to get IPv6 addresses too :)

nikatza · 2026-09-08T11:48:09Z

+	addrs := &interfaceAddrs{ifName: name}
+	if err := p.client.GetState(ctx, addrs); err != nil {
+		if errors.Is(err, gnmiext.ErrNil) {
+			return "", apistatus.NewFailedPreconditionError(fmt.Sprintf("interface %q has no IPv4 address A", name))


"IPv4 address A" --> "IPv4 address"?

nikatza · 2026-09-08T12:55:52Z

+k8s_yaml('./examples/openconfig-containerlab/kubernetes/01-devices/v1alpha1-leaf1-clab-ntp.yaml')
+k8s_resource(new_name='ntp-clab', objects=['ntp-clab:ntp'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['containerlab'])


I would propose to leave the existing resource from samples and keep the tilt file compact.

felix-kaestner · 2026-09-08T12:56:03Z

 	Prefer bool `json:"prefer,omitempty"`

 	// The name of the vrf used to communicate with the NTP server.
+	// Maps to network-instance in Openconfig


Suggested change

// Maps to network-instance in Openconfig

// Maps to network-instance in Openconfig.

felix-kaestner · 2026-09-08T12:56:26Z

+  labels:
+    app.kubernetes.io/name: network-operator
+    app.kubernetes.io/managed-by: kustomize
+    networking.metal.ironcore.dev/device-name: leaf1-clab


Suggested change

networking.metal.ironcore.dev/device-name: leaf1-clab

networking.metal.ironcore.dev/device-name: leaf1

felix-kaestner · 2026-09-08T12:57:22Z

+	addrs := &interfaceAddrs{ifName: name}
+	if err := p.client.GetState(ctx, addrs); err != nil {
+		if errors.Is(err, gnmiext.ErrNil) {
+			return "", apistatus.NewFailedPreconditionError(fmt.Sprintf("interface %q has no IPv4 address A", name))


What is "IPv4 address A" and "IPv4 address B" in this file?

felix-kaestner · 2026-09-08T12:58:51Z

+// DNS represents the OpenConfig /system/ntp container.
+type NTP struct {
+	Config  *NTPConfig  `json:"config"`
+	Servers *NTPServers `json:"servers"`


Suggested change

Servers *NTPServers `json:"servers"`

Servers *NTPServers `json:"servers,omitempty"`

This field is optional and may be nil in which case I think we don't want to serialize "null" into the JSON payload. As we do an update (gnmi replace), omitting this field is also fine.

felix-kaestner · 2026-09-08T13:18:25Z

 k8s_resource(new_name='route-prefix', objects=['route-prefix:probe'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['samples'])
 k8s_resource(new_name='vtep-peers', objects=['vtep-peers:probe'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['samples'])

+k8s_yaml('./examples/openconfig-containerlab/kubernetes/01-devices/v1alpha1-leaf1-clab-ntp.yaml')


The naming here is a bit weird. This is nothing really containerlab specific in the sample.

felix-kaestner · 2026-09-08T13:19:55Z

+// NTPServerConfig holds the config container for a NTP server.
+type NTPServerConfig struct {
+	Address         string `json:"address"`
+	Prefer          bool   `json:"prefer,omitempty"`


Suggested change

Prefer bool `json:"prefer,omitempty"`

Prefer bool `json:"prefer"`

I think we don't want to omit a value of false (default).

github-actions Bot added the size/L label Aug 21, 2026

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

hardikdr added this to Roadmap Aug 22, 2026

adamtrizuljak-sap force-pushed the feat/openconfig-ntp branch from a355b07 to e1c927a Compare September 3, 2026 08:31

adamtrizuljak-sap marked this pull request as ready for review September 3, 2026 14:21

felix-kaestner reviewed Sep 3, 2026

View reviewed changes

adamtrizuljak-sap force-pushed the feat/openconfig-ntp branch 2 times, most recently from 6974f26 to edf5a26 Compare September 7, 2026 15:01

adamtrizuljak-sap force-pushed the feat/openconfig-ntp branch from edf5a26 to 2ab090d Compare September 8, 2026 10:00

nikatza reviewed Sep 8, 2026

View reviewed changes

felix-kaestner reviewed Sep 8, 2026

View reviewed changes

		@@ -0,0 +1,19 @@
		apiVersion: networking.metal.ironcore.dev/v1alpha1

		k8s_yaml('./examples/openconfig-containerlab/kubernetes/01-devices/v1alpha1-leaf1-clab-ntp.yaml')
		k8s_resource(new_name='ntp-clab', objects=['ntp-clab:ntp'], trigger_mode=TRIGGER_MODE_MANUAL, auto_init=False, labels=['containerlab'])

	// Maps to network-instance in Openconfig
	// Maps to network-instance in Openconfig.

	networking.metal.ironcore.dev/device-name: leaf1-clab
	networking.metal.ironcore.dev/device-name: leaf1

	Servers *NTPServers `json:"servers"`
	Servers *NTPServers `json:"servers,omitempty"`

	Prefer bool `json:"prefer,omitempty"`
	Prefer bool `json:"prefer"`

Uh oh!

Conversation

adamtrizuljak-sap commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cisco vs Openconfig compatibility issue (solved)

Solution

Previous solution - abandoned

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!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 8, 2026

Merging this branch will decrease overall coverage

Changed files (no unit tests)

Uh oh!

nikatza left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adamtrizuljak-sap commented Aug 21, 2026 •

edited

Loading