Skip to content

Add OpenConfig user provider - #513

Open
rgildein wants to merge 6 commits into
mainfrom
feat/openconfig-user
Open

Add OpenConfig user provider#513
rgildein wants to merge 6 commits into
mainfrom
feat/openconfig-user

Conversation

@rgildein

@rgildein rgildein commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Add OpenConfig provider implementation for the User resource targeting the standard OpenConfig path:
openconfig-system:system/aaa/authentication/users/user[username=X]/config

What changed

Initial implementation:

  • EnsureUser / DeleteUser for the OpenConfig provider
  • Supports username, role, and ssh-key — standard OpenConfig user config leaves
  • Uses gNMI update (Patch) instead of replace — Juniper rejects gNMI replace for new list entries with statement not found; update works for both create and update
  • spec.roles with more than one entry returns UnsupportedFieldError — the OpenConfig role leaf is a single value, confirmed on both Juniper and Nokia

Password hashing:

  • Plaintext password from the Kubernetes Secret is hashed with SHA-512 crypt ($6$) before being sent to the device via the password-hashed gNMI field
  • password-hashed is stored verbatim by Juniper, enabling idempotent reconciliation: the existing hash is read back, its salt extracted, and the password re-hashed — if the password is unchanged the output is identical and no gNMI Set is issued
  • If the password changes, or the user does not exist yet, a fresh hash with a random salt is generated

Testing

Validated against a real Juniper vJunos-Evolved device:

  • User created with username, password, role, and ssh-key
  • Hash stable across multiple reconcile cycles (no gNMI Set after initial write)
  • Password update triggers a new hash and a gNMI Set
Juniper vJunos-Evolved

Device: Juniper vJunos-Evolved 26.2R1.7 via containerlab

Verify user config:

gnmic -a <device-address>:57401 -u admin -p 'admin@123' --skip-verify \
  --encoding JSON_IETF --type CONFIG get \
  --path 'openconfig-system:system/aaa/authentication/users/user[username=vjunos]'
Nokia SR Linux

Nokia SRL's OpenConfig user model does not expose password or ssh-key as writable leaves — only username and role are supported via OpenConfig. A separate Nokia-specific provider will be needed for password and SSH key management.

@rgildein rgildein self-assigned this Aug 20, 2026
@rgildein
rgildein marked this pull request as draft August 20, 2026 14:13
@rgildein
rgildein force-pushed the feat/openconfig-user branch from 1227441 to aa94822 Compare August 21, 2026 05:51
Implement UserProvider for the OpenConfig provider, targeting the
standard OpenConfig path:
openconfig-system:system/aaa/authentication/users/user[username=X]/config

Nokia SR Linux limitation: the OpenConfig user model on SRLinux does
not expose password or ssh-public-key as writable config leaves — only
username and role are settable, and only a single role is accepted.
The provider raises UnsupportedFieldError for spec.password,
spec.sshPublicKey, and spec.roles when more than one role is given.
Since spec.password is mandatory in the CRD, User CRs will always
reach Ready=False (terminal) on Nokia SRL via the OpenConfig provider.

Also adds a gnmi testdata file documenting the expected device state
for a user created with a single role.

Co-authored-by: Claude <claude@anthropic.com>
Signed-off-by: Robert Gildein <rgildein@users.noreply.github.com>
@rgildein
rgildein force-pushed the feat/openconfig-user branch from aa94822 to f480dcc Compare August 21, 2026 06:26
@hardikdr hardikdr added the area/switch-automation Automation processes for network switch management and operations. label Aug 21, 2026
@hardikdr hardikdr added this to Roadmap Aug 21, 2026
Comment thread internal/apistatus/apistatus.go Outdated
rgildein and others added 2 commits September 2, 2026 08:31
Rewrite the user provider to target vanilla OpenConfig rather than
Nokia SRLinux-specific behavior:

- Support password, ssh-key and role — all standard OpenConfig user
  config leaves (openconfig-system:system/aaa/authentication/users)
- Use gNMI update (Patch) instead of replace (Update) for user
  creation — Juniper rejects replace for new entries with
  "statement not found"
- Password excluded from UnmarshalJSON to avoid perpetual diffs
  (device returns hashed value that never matches plaintext)
- Remove Nokia-specific UnsupportedFieldError for password/sshPublicKey
- Remove CodeIgnoredField from apistatus — not needed for vanilla
  OpenConfig; revert related changes to conditions.go and
  user_controller.go
- Only retain single-role constraint (OpenConfig role leaf is a
  single value, not a leaf-list — confirmed on both Juniper and Nokia)
- Replace user.txt testdata with proper user.txtar including secrets
  and full expected gNMI state with ssh-key

Tested against Juniper vJunos-Evolved 26.2R1.7 via containerlab.

Co-authored-by: Claude <claude@anthropic.com>
Signed-off-by: Robert Gildein <rgildein@users.noreply.github.com>
@rgildein
rgildein marked this pull request as ready for review September 2, 2026 08:51
Comment thread internal/provider/openconfig/user.go Outdated
Comment on lines +53 to +56

// UserConfig holds the user config container leaves.
// Password is write-only — the device returns a hashed value that would never match
// the plaintext, so we exclude it from unmarshal to avoid perpetual diffs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please have a look into what we are doing on the nxos provider in https://github.com/ironcore-dev/network-operator/blob/main/internal/provider/cisco/nxos/provider.go#L2620-L2641

We take the plaintext password as we retrieve it from the kubernetes secret and compute the hash ourselves (which we can do if the hash includes the algorithm and salt value). If the hash we compute from the plaintext value matches, what is stored in hashed form on the device. We can retain that value.

Otherwise, we would end up with a gnmi write on every reconcilation, which we definitely want to avoid.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok so if user set up password we hashed and set hashed password on device. Make sense, I'll check the mechanism how we are hashing it.

Comment thread internal/provider/openconfig/user.go Outdated
// User targets an OpenConfig user entry.
type User struct {
Username string `json:"-"`
Config *UserConfig `json:"config,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Config *UserConfig `json:"config,omitempty"`
Config *UserConfig `json:"config"`

This field is always present and should therefore not have an omitempty tag. See

network-operator/AGENTS.md

Lines 156 to 161 in e21328d

**`omitempty` guidelines:**
1. **Safe:** The field's Go zero value matches the platform default or "absent" state. Omitting it from the payload is semantically equivalent to the device's default.
2. **Safe:** The field is a pointer or slice representing "not configured" (nil) vs "configured" (non-nil). Mutually exclusive choices (e.g. `accept`/`drop`) fall into this category.
3. **Dangerous:** The platform default is non-zero (e.g. `admin-state` defaults to `"enable"`, `port` defaults to `49`). Omitting the Go zero value would either misrepresent intent or cause a false diff on subsequent GET responses.
4. **Unnecessary:** The field is unconditionally set to a non-zero value by the provider code. The tag never triggers, but removing it documents intent — the field is always present.

SSHKey: req.SSHKey,
},
}
return p.client.Patch(ctx, u)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any particular reason we use a patch over an update here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Update was failing on Juniper device, I think it was to avoid changing username <==> creating new user. I can try out with update and share the message if you want.

rgildein and others added 2 commits September 8, 2026 10:00
Use SHA-512 crypt ($6$) to hash the user password before setting it via
the password-hashed field. Reuse the existing hash when the password
matches to avoid unnecessary gNMI Set calls.

Co-authored-by: Claude <claude@anthropic.com>
Signed-off-by: Robert Gildein <rgildein@users.noreply.github.com>
Comment thread internal/provider/openconfig/user.go
Comment thread internal/provider/openconfig/user_test.go Outdated
@felix-kaestner

Copy link
Copy Markdown
Contributor

@rgildein when merging this PR, could you make sure to squash the commits into a single one so we keep the history linear and clean? 🙏

Signed-off-by: Robert Gildein <rgildein@users.noreply.github.com>
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/ironcore-dev/network-operator/internal/provider/openconfig 9.82% (+1.84%) 👍

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/ironcore-dev/network-operator/internal/provider/openconfig/user.go 54.17% (+54.17%) 24 (+24) 13 (+13) 11 (+11) 🌟

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/provider/openconfig/user_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/switch-automation Automation processes for network switch management and operations. size/L

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants