Fix the dead endpoints, dead paths and dead files the samples audit found - #863
Merged
Merged
Conversation
Audited the samples against the feature docs of the stack. Three groups of fixes, all of them things a reader of a sample would have been misled by. Endpoints the samples advertised but never served. Every sample server declared an https base address next to its opc.tcp one, but no sample ever registered a transport binding for it. The stack creates a listener only for a scheme whose factory is registered and skips the rest without an error, so those endpoints were never opened while GetDiscoveryUrls kept advertising them. The three samples which reference the https binding package now call AddHttpsTransport() and keep their endpoint; the seventeen quickstart servers and the two GDS servers, which do not, drop the address. The ApplicationInstance path now hands the server the transport registry of the container, so a registration reaches the server it was made for. A new tier 0 test keeps the two in step: a config may only declare a base address of another scheme if its sample is on the list of samples which register one. The tier 2 harness strips non-tcp addresses before it starts a server, which is why none of this ever failed a test. Dead paths the user could walk into. The UserAuthentication server advertised an IssuedToken policy for the WS-Security Kerberos profile, with an IssuerEndpointUrl naming a machine that has not existed for years, and the Kerberos tab of its client threw NotSupportedException because the token provider it needed does not exist on modern .NET. Both are gone, with a pointer to the identity provider documentation for the flows the stack does support. The dead SAML helper next to it goes too. Clock and API use. No sample used TimeProvider, against 38 files reading DateTime.UtcNow and ten raw Timers, so a server run against a fake clock kept simulating on the wall clock. The sample node managers now resolve the clock of the server through the ITimeProviderProvider seam and create their timers from it; PerfTest measures its update duration with GetElapsedTime rather than by subtracting two wall clock readings. The NodeManagement client replaces its hand written event filter and publish pump with the ModelChangeTracker of the stack, which also evicts the changed nodes from the node cache - the part the hand written pump did not do, so the client could read out of a stale cache after a model change. The reverse connect manager of the aggregation server is disposed asynchronously from StopAsync instead of through the obsolete synchronous Dispose, and the argument list control awaits GetBuiltInTypeAsync. README: the console walkthrough pointed at Samples/NetCoreConsoleServer and Samples/NetCoreConsoleClient, neither of which exists. Rewritten around the console samples that do, with the target frameworks, solution file and IDE version corrected, the ConsoleLds sample added to the project table, and a pointer to the samples which live in the stack repository. All four test tiers pass: 227 configuration, 115 server, 165 node manager and 61 client tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The SDK-style project migration left every file it could not place excluded by hand, under an item group labelled "Compile items now included by globbing that were not in the original project file". Those files have been dead ever since: they are not compiled, so nothing has kept them building, and several of them call APIs the 2.0 stack no longer has. They are also findable, which is the real cost - a reader grepping the samples lands in code that cannot run, and the audit that preceded this change reported two defects that turned out to live in exactly these files. Deleted 50 files across four projects: - Samples/ClientControls.Net4, 13 dialogs and controls with their designer and resource files. Five of them (BrowseNodeCtrl, SelectNodeCtrl, SelectNodeDlg, EditAnnotationDlg, EditComplexValueDlg) are superseded by the copies under Common/Client which the project does compile; the Endpoints ones still call the synchronous DiscoveryClient.Create and BindingFactory that 2.0 removed. - Samples/ReferenceServer/Aggregates, the whole folder. Its aggregate calculators predate the AggregateManager the stack now ships. Neither of the two samples that name AggregateManager or Aggregators references this project, so both already resolve to Opc.Ua.Server. - Workshop/AlarmCondition/Client/EventFieldDefinition.cs and Workshop/HistoricalEvents/Client/FilterDefinition(Field).cs, superseded by Workshop/Common. The removed types were referenced only by each other. The two Compile Remove entries that remain are not dead code and stay: the sample hosting library excludes its WinForms folder on non-Windows targets, and Opc.Ua.Sample excludes its composition root on the targets where the hosting library is not referenced. Also fixes what the last commit left behind: dropping IDisposable from AggregationReverseConnect raised CA1001, so it now implements IAsyncDisposable and StopAsync delegates to it, idempotently. With the two obsolete NodeId .Identifier and DataValue.Value reads in the node manager tests replaced, the solution builds with no CS0618 at all. All four test tiers pass unchanged: 227 configuration, 115 server, 165 node manager and 61 client tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Master #861 dropped the per-sample StandardServer subclasses for a builder-only DI surface, which touched three of the files this branch changes. The interesting resolution is Samples/Hosting/SampleApplicationHostedService.cs, where this branch is taken out entirely in favour of master. This branch had that service assign the container's ITransportBindingRegistry to the server, because a server started down the ApplicationInstance path would otherwise fall back to a private registry that only knows opc.tcp - so an AddHttpsTransport() registration never reached it. After #861 no sample starts a server that way: every one of them goes through AddSampleServer(configureServer) and the hosted server of the stack, which already assigns TransportBindings from dependency injection (OpcUaServerHostedService.cs:219). The workaround is obsolete and master's simpler service is correct. The other three keep master's shape and re-apply the transport registration on top: the two WinForms entry points move to AddSampleServer(server => server.AddUaSampleServer()) and the reference server to AddSampleServer<ReferenceServer>(...) with its node managers on the builder, each still calling services.AddOpcUa().AddHttpsTransport() so the https base address in their configuration is actually served. Everything else merged cleanly. The ITimeProviderProvider seams, the ModelChangeTracker in the NodeManagement client and the deletions all survive. All four tiers pass on the merged tree: 227 configuration, 116 server (one more than before, from master), 165 node manager and 61 client tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
An audit of the samples against the feature documentation of the stack
(
UA-.NETStandard/docs, at the2.0.0-preview.4versiontargets.props pins) turned up three kinds of
problem. This PR fixes all of them. The coverage gaps the same audit found are
filed separately as #862 and are not in scope here.
1. Every server sample advertised an endpoint it never served
All 21 server configurations declared an
httpsbase address next to theiropc.tcpone, and no sample anywhere calledAddHttpsTransport(). Thestack creates a listener only for a scheme whose factory is registered and
skips the rest without an error, while
GetDiscoveryUrls()keeps advertisingthe address — so every one of these servers published a discovery URL that
refuses connections.
Fixed by capability rather than uniformly:
ReferenceServer,Server.Net4andClient.Net4already referenceOpc.Ua.Bindings.Https, so they now callAddHttpsTransport()and keeptheir endpoint.
drop the address rather than gaining an ASP.NET Core dependency.
SampleApplicationHostedServicenow hands the server the container'sITransportBindingRegistry. Without this, a registration made on theApplicationInstancepath would never reach the server it was made for,because
ServerBasefalls back to a private registry that only knowsopc.tcp.Why this was invisible: the tier 2 harness (
SampleServerHost.KeepOpcTcpEndpointsOnly)strips every non-
opc.tcpbase address before starting a server, so no testever exercised the configurations as shipped. A new tier 0 test,
BaseAddressesOnlyUseRegisteredTransports, closes that hole: a config maydeclare a base address of another scheme only if its sample is on an explicit
allowlist of samples that register one. Verified it fails when the condition is
reintroduced (1 failure / 43 passes), then reverted the probe.
2. Dead paths a user could walk into
The
UserAuthenticationserver advertised anIssuedTokenpolicy for theWS-Security Kerberos profile with an
IssuerEndpointUrlnaming a machine thathas not existed for years, and the Kerberos tab of its client threw
NotSupportedExceptionbecause the token provider it needs does not exist onmodern .NET. Both removed, with a pointer to
docs/IdentityProviders.mdfor theflows the stack does support. The unused SAML helper beside it goes too.
Viewsclient connected with.Wait()on the UI thread; nowasync voidlikeevery sibling sample.
3. SDK functionality the samples reimplemented or ignored
TimeProviderwent from zero uses to threaded through every simulation.38 files read
DateTime.UtcNowand ten created rawTimers, so a server runagainst a fake clock kept simulating on the wall clock. The node managers now
resolve the server's clock through the documented
ITimeProviderProviderseam and create their timers from it.
PerfTestmeasures its update durationwith
GetElapsedTimeinstead of subtracting two wall-clock readings.NodeManagementclient replaces a hand-writtenEventFilterandpublish pump with the stack's
ModelChangeTracker, which also evicts changednodes from the
INodeCache— the part the hand-written version did not do,so the client could read from a stale cache after a model change.
ReverseConnectManageris disposed asynchronously fromStopAsyncinstead ofthrough the obsolete synchronous
Dispose;ArgumentListCtrlawaitsGetBuiltInTypeAsync.The solution now builds with zero
CS0618, down from 10.4. 50 dead files deleted
Everything under the item groups labelled "Compile items now included by
globbing that were not in the original project file" — SDK-style migration
leftovers that nothing has compiled since, several of them calling APIs 2.0
removed. 13 dialogs/controls in
ClientControls.Net4(5 superseded by the livecopies under
Common/Client), the wholeReferenceServer/Aggregatesfolder(predates the stack's
AggregateManager; verified neither consumer referencesthat project), and three superseded type files in the AlarmCondition and
HistoricalEvents clients.
The two remaining
Compile Removeentries are conditional TFM excludes, notdead code, and stay.
5. README drift
The console walkthrough pointed at
Samples/NetCoreConsoleServerandSamples/NetCoreConsoleClient, neither of which exists. Rewritten aroundthe console samples that do, with the target frameworks (net10/net8/net48, not
".NET Core 2.0 and UWP"), solution file (
UA Samples.slnx) and IDE versioncorrected,
ConsoleLdsadded to the project table, and a pointer to thesamples that live in the stack repository.
Related Issues
behaviour, and
docs/Transports.mddocuments the opposite ("startup throwsa clear error").
NodeBrowserhas no async seam, whichis why
Workshop/Aggregation/Server/Browser.csstill blocks on a task. Itcannot be fixed in this repository; the constraint is now documented in the
code instead of left looking like sloppiness.
Types of changes
Checklist
All four tiers pass locally, unchanged before and after the deletions:
Further comments
Three findings from the audit did not survive checking, and are worth stating
so nobody re-reports them. The samples do not have a stale security-policy
problem: the empty
<SecurityPolicyUri></SecurityPolicyUri>entries are awildcard that
ValidateSecurityPoliciesexpands to Basic256Sha256 +Aes128_Sha256_RsaOaep + Aes256_Sha256_RsaPss, and the
Basic256entries areinside a
<!-- deprecated -->block. Only ECC is genuinely missing, and thatneeds an ECC application certificate rather than a config line, so it moved to
#862.
On scope. The endpoint fix could have gone the other way — add
Bindings.Httpsto all 21 servers. I chose not to: a quickstart that exists todemonstrate one Part of the spec should not gain an ASP.NET Core dependency to
serve an endpoint nobody uses. The three samples that already carry the
dependency keep their endpoint and now actually serve it.
On
ITimeProviderProviderover constructor injection. The migration docsuggests a nullable
TimeProvideras the last constructor parameter. That doesnot work uniformly here, because nine of the sample node managers are
source-generated and their constructors are emitted. The
ITimeProviderProviderseam works identically for generated and hand-written managers and resolves the
server's clock rather than a separately injected one — which matters, because
SampleServerFactorydeliberately drops theTimeProviderthe stack hands it.🤖 Generated with Claude Code