Skip to content

Eden to evetest: rewrite networking, security, apps and lps tests - #6244

Draft
milan-zededa wants to merge 4 commits into
lf-edge:masterfrom
milan-zededa:eden-to-evetest
Draft

Eden to evetest: rewrite networking, security, apps and lps tests#6244
milan-zededa wants to merge 4 commits into
lf-edge:masterfrom
milan-zededa:eden-to-evetest

Conversation

@milan-zededa

@milan-zededa milan-zededa commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Description

Rewrites a batch of Eden E2E test scenarios (networking, security, apps, LPS, upgrade)
as native Go tests in the evetest framework, plus some framework/library enhancements
and one SDN bugfix needed to support them.

Commits

  • zedrouter, zedagent: rename NI status field to CombinedNTPServers
    NetworkInstanceConfig.NtpServers (the NI's own raw configured list) and
    NetworkInstanceStatus.NTPServers (the combined NI+port list zedrouter
    actually maintains) differed only by case, so zedagent's reporting code
    silently picked up the wrong one via field promotion through the embedded
    config struct. Renamed the status field to CombinedNTPServers and fixed
    zedagent to publish it.

  • evetest/sdn: fix dnsmasq router option not being suppressed
    A DHCP server config with GatewayIPv4 left nil (the documented way to
    make clients skip installing a default IPv4 route) simply omitted the
    dhcp-option=option:router line instead of explicitly suppressing it.
    dnsmasq's own default, absent an explicit override, is to advertise its
    own listening address on that subnet as the router, so clients still
    received one anyway.

  • evetest: framework/library enhancements
    Adds the EdgeDevice/EdgeCluster/harness capabilities the test
    rewrites below need: flow-log streaming (IterateDeviceFlowLogs,
    SubscribeToDeviceFlowLogs, the gRPC server iterator, and the
    evetest eve flow-logs CLI subcommand), EdgeDevice: GetArch,
    PowerOff, PowerOn, WaitForClusterNodeIsReady,
    and EdgeDevice.DialViaSSH (an SSH-tunneled net.Conn, used to reach
    the Kubevirt VNC proxy which only binds to the device's loopback).
    Also removes the now-unused SkipIfHypervisorKubevirt helper (all tests
    now support running at-least single-node EVE-K).

  • evetest: rewrite eden test suites (networking, security, apps, lps, upgrade)
    The actual eden -> evetest ports:

    • networking: many new tests, now clearly surpassing eden in the coverage
      of EVE networking.
    • security: TestAppArmorEnabled (kernel AppArmor status) and
      TestVCom (vcomlink TPM-over-vsock request/response from inside a
      guest VM).
    • apps: TestVNC (console access, VM and container apps) and app
      purge.
    • LPS: local profile server scenarios (app/device local info,
      profile switching, radio silence).
    • upgrade: support upgrade scenario with OCI datastore.

This leaves only the "storage" and "smoke" test suites as not yet ported from eden to evetest.

PR dependencies

Marking this PR as draft until a required Adam change is merged:
lf-edge/adam#157

How to test and validate this PR

# networking
make evetest NAME=TestFlowLog
make evetest NAME=TestNetworkConfigFallback
make evetest NAME=TestPortFailover
make evetest NAME=TestIntermittentConnectivity
make evetest NAME=TestMgmtTrafficRoutedViaApp
make evetest NAME=TestNIReplace
make evetest NAME=TestMoveAppBetweenNIs
make evetest NAME=TestAirGapSwitchNI
make evetest NAME=TestLimitedIPSpace
make evetest NAME=TestDeviceNTPConfig
make evetest NAME=TestApplicationNTPConfig

# security
make evetest NAME=TestAppArmorEnabled
make evetest NAME=TestVCom

# apps
make evetest NAME=TestVNC
make evetest NAME=TestPurgeNeverActivatedApp

# lps (local profile server)
make evetest NAME=TestAppLocalInfo
make evetest NAME=TestDevLocalInfo
make evetest NAME=TestProfile
make evetest NAME=TestRadioSilence

# upgrade
EVETEST_DATASTORE_TYPE=OCI make evetest NAME=TestEVEUpgrade

Changelog notes

Fixed a bug where a network instance's device-level NTP servers (learned
from DHCP or configured directly on the port) were not included in the
NTP server list reported to the controller for apps attached to that
network instance -- only the network instance's own explicitly
configured NTP servers were. Apps now see the full, correct combined
list.

All other changes are test-only (evetest framework additions and eden
test-suite rewrites) plus one SDN test-harness bugfix; no other
user-facing EVE behavior changes.

PR Backports

- 17.0-stable: Eventually to be backported (the entire evetest dir).
- 16.0-stable: No, as evetest is not available there.
- 14.5-stable: No, as evetest is not available there.
- 13.4-stable: No, as evetest is not available there.

Checklist

  • I've provided a proper description
  • I've added the proper documentation
  • I've tested my PR on amd64 device
  • I've tested my PR on arm64 device
  • I've written the test verification instructions
  • I've set the proper labels to this PR
  • I've checked the boxes above, or I've provided a good reason why I didn't check them.

@milan-zededa
milan-zededa requested review from europaul and rene July 31, 2026 14:41
@milan-zededa milan-zededa added the testing New test or test framework changes label Jul 31, 2026
@milan-zededa

Copy link
Copy Markdown
Contributor Author

@claude

Comment thread evetest/ssh.go Dismissed
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude finished @milan-zededa's task in 3m 45s —— View job


Code review

  • Read pillar changes (zedrouter / zedagent NTP rename)
  • Read SDN dnsmasq fix
  • Read evetest framework/library changes
  • Read test rewrites
  • Post review

I reviewed the production-code changes and the framework/library additions in depth, and scanned the test rewrites. This is a well-structured, thoroughly-commented PR.

Production code (the parts with user-facing impact) look correct:

  • The NTPServersCombinedNTPServers rename is complete and consistent — I confirmed no stale NetworkInstanceStatus.NTPServers references remain (the other .NTPServers hits are on unrelated types: port DhcpConfig, DHCPServer, monitorapi). The rename genuinely fixes the field-promotion ambiguity described in the commit, and since NetworkInstanceStatus is an ephemeral /run pubsub topic republished on boot, the JSON field rename carries no upgrade/persistence risk.
  • The dnsmasq fix (explicit dhcp-option=option:router to suppress the router option when GatewayIPv4 is nil) is correct — an empty-value option is dnsmasq's documented way to suppress the default.

One latent bug I flagged inline (framework code): the OCI-datastore image-ref split in UpgradeEVE uses strings.Cut (first colon), which mis-splits registry-with-port refs. Harmless for the Docker Hub refs the current test uses, but worth fixing to LastIndex.

Other things I checked and consider fine:

  • AdamClient.IterateDeviceFlowLogs / SubscribeToDeviceFlowLogs: goroutine lifecycle, unsubscribe idempotency (sync.Once), channel close, and streamCtx cancellation are all handled correctly; the reconnect loop mirrors the existing metrics-subscription pattern.
  • waitUntilAppState / WaitUntilAppIsRunning refactor (subscribe-before-snapshot, live flag to suppress replay logging, nil-timer guards during replay) is intricate but internally consistent.
  • Good incidental fix: ReadPublication persistent path corrected from /persistent/status//persist/status/.
  • dialViaSSH / sshTunnelConn.Close() correctly own and tear down the underlying SSH client. (Minor: the ctx arg isn't honored by ssh.Dial, which uses its own 5s timeout — not worth changing.)

No security or correctness concerns in the pillar changes. Nice work.
· eden-to-evetest

Comment thread evetest/edgedevice.go Outdated
milan-zededa and others added 4 commits July 31, 2026 17:07
ZInfoApp was not reporting all NTP servers for an application's network
instance: NetworkInstanceConfig.NtpServers (the NI's own raw configured
list) and NetworkInstanceStatus.NTPServers (the combined NI+port list
zedrouter actually maintains) differed only by case, so the reporting
code in zedagent silently picked up the wrong one via field promotion
through the embedded config struct.

Renamed the status field to CombinedNTPServers to make the two
unambiguous by name, and fixed zedagent to publish it.

Signed-off-by: Milan Lenco <milan@zededa.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
When a DHCP server's GatewayIPv4 is left nil (the documented way to make
clients not install a default IPv4 route), the config renderer simply
omitted the dhcp-option=option:router line instead of explicitly
suppressing it. dnsmasq's own default, absent an explicit override, is to
advertise its own listening address on that subnet as the router, so
clients still received one -- defeating the purpose of leaving
GatewayIPv4 unset.

Signed-off-by: Milan Lenco <milan@zededa.com>

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
Add the EdgeDevice/EdgeCluster/harness capabilities needed by the
eden-to-evetest test rewrites:

- Flow-log streaming: AdamClient.IterateDeviceFlowLogs/
  SubscribeToDeviceFlowLogs, the corresponding gRPC server iterator, and
  the `evetest eve flow-logs` CLI subcommand.
- EdgeDevice.GetArch, PowerOff, PowerOn and WaitForClusterNodeIsReady.
- EdgeDevice.DialViaSSH (an SSH-tunneled net.Conn, used for reaching the
  Kubevirt VNC proxy which only binds to the device's loopback).

Also removes the now-unused SkipIfHypervisorKubevirt test-parameter
helper; its call sites move to an inline
`if hypervisor == evetest.HypervisorKubevirt` check in the test-suite
rewrite commit that follows.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
…pgrade)

Port the corresponding eden E2E scenarios to the evetest Go framework
across networking (flow logs, DPC fallback/failover, intermittent
connectivity, mgmt traffic routed via an app-based NAT gateway, and
several smaller fixes), security (AppArmor status, vcomlink TPM-over-vsock),
apps (VNC console access, app purge), LPS and upgrade. Adds the
TwoMgmtPortsWithPublicNTP and MgmtViaAppTopology network models used by
the rewritten tests.

Signed-off-by: Milan Lenco <milan@zededa.com>

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
@milan-zededa

Copy link
Copy Markdown
Contributor Author

@claude

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude finished @milan-zededa's task in 2m 12s —— View job


Re-review

  • Re-read pillar changes (NTP rename)
  • Re-check framework/library changes (adam flow-logs, edgedevice, ssh)
  • Verify the strings.LastIndex fix landed
  • Scan new/rewritten test suites
  • Post findings

The one finding from my previous pass is resolved and I found nothing new that blocks. Summary below.

Fix confirmedUpgradeEVE now splits the OCI image reference with strings.LastIndex (edgedevice.go), so registry-with-port refs no longer mis-split. Good.

Production code (pillar) — re-verified correct:

  • The NTPServersCombinedNTPServers rename is complete. I re-grepped all of pkg/pillar: every remaining .NTPServers hit is on an unrelated type (DhcpConfig/port config, DHCPServer, monitorapi, kubevirt vendor) — no stale NetworkInstanceStatus.NTPServers references remain, so field promotion can no longer silently resolve to the NI-only config list. Since NetworkInstanceStatus is an ephemeral /run topic republished on boot, the JSON field rename carries no persistence/upgrade risk.
  • dnsmasq fix (explicit empty dhcp-option=option:router to suppress option 3 when GatewayIPv4 is nil) matches dnsmasq's documented suppression semantics.

Framework additions — checked, no issues:

  • IterateDeviceFlowLogs / SubscribeToDeviceFlowLogs: 404-on-empty handling, protojson decode loop, streamCtx cancellation, sync.Once unsubscribe, channel close, and the fixed-delay reconnect loop all mirror the existing metrics-subscription pattern correctly.
  • waitUntilAppState / WaitUntilAppIsRunning subscribe-before-snapshot refactor: the overlap window (a message landing between subscribe and the GET snapshot appears in both, but state application is idempotent) and the nil-timer guards during replay are internally consistent. nonDownloadStart is initialized to time.Now() at declaration and reset before the live phase, so nonDownloadElapsed can't be corrupted by replay — the "negligible elapsed" reasoning holds.
  • dialViaSSH / sshTunnelConn.Close() correctly own and tear down the underlying SSH client; scpFromEVE/scpToEVE now shell-escape the remote path (good catch for keys with spaces).

Not re-raising: the CodeQL InsecureIgnoreHostKey flag on ssh.go:86 — your justification (ephemeral per-test EVE VMs, shared throwaway key) is reasonable for test-only harness code.

No security or correctness concerns. LGTM.
· eden-to-evetest

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 23.57%. Comparing base (b7d4463) to head (c5ad210).

Files with missing lines Patch % Lines
pkg/pillar/cmd/zedrouter/networkinstance.go 0.00% 2 Missing ⚠️
pkg/pillar/cmd/zedagent/handlemetrics.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6244      +/-   ##
==========================================
+ Coverage   23.21%   23.57%   +0.36%     
==========================================
  Files         510      520      +10     
  Lines       93470    95186    +1716     
==========================================
+ Hits        21697    22441     +744     
- Misses      70025    70811     +786     
- Partials     1748     1934     +186     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

testing New test or test framework changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants