openconfig / gnsi

Apache License 2.0
20 stars 16 forks source link

Certz does not have counters per-profile #111

Closed haussli closed 1 year ago

haussli commented 1 year ago

certz supports multiple ssl profiles (RotateCertificateRequest.ssl_profile_id). It also maintains counters exposed as augment /oc-sys:system/oc-sys-grpc:grpc-servers/oc-sys-grpc:grpc-server/oc-sys-grpc:state:

This structure does not appear to be per-ssl_profile_id. So, 1) it is global, which is not logical given its fields, or 2) it needs a ssl_profile_id field and a list keyed by that id, in yang parlance.

2 seems to be the answer, but I'm not sure what implementation to suggest here.

morrowc commented 1 year ago

Sadly I keep avoiding learning much of anything about YANG.. but like a ton of other things on a network device where there are multiple instances of a thing, yea .. we want to count the relevant / critical bits related to those instances and not attempt to interpolate / infer what one instance of the thing is doing based on jiggery-pokery from remote.

so yea, please do 2 ... the path you point to doesn't look directly like it's 'ssl profile' related...

brianneville commented 1 year ago

I think the counter leafs make sense where they are as an augment to the grpc-server/state container, for a number of reasons:

  1. Accepting or rejecting connections from the system is something that occurs on a per-gRPC-server basis, and representing this relationship in YANG makes more sense. True, a gRPC server is using an SSL profile but we should view the gRPC server as the target which we are attempting to connect to and which will accept/reject connections - not the SSL profile, which is just another constituent part of that connection (like port number, VRF, or hostname).

  2. An SSL profile may be in-use by multiple gRPC servers (e.g. gNMI, gNOI, gRIBI etc may not necessarily be hosted on the same gRPC server), and I think there's more value in having counters per-gRPC-server than based on the SSL profile ID. The increased granularity from per-gRPC-server counters provides more potential for monitoring connection attempts. I.e. from a per-ssl_profile_id counter you can't determine how many people actually attempted to access any particular gRPC server. But with per-gRPC-server counters you can determine this, while also being able to also monitor connection attempts based on SSL profile ID as long as you know the SSL profile ID of the server (see my suggestion at the end of this message) and sum the per-gRPC-server counters. For example, having a per-ssl_profile_id access-reject counter increment only tells you that "among all the gRPC servers using this SSL profile, one of them had a client which is misconfigured". Compare this to the current per-gRPC-server counters where if an access-reject counter increments, the YANG indicates which exact server had a misconfigured client attempt a connection.

we want to count the relevant / critical bits related to those instances and not attempt to interpolate / infer what one instance of the thing is doing based on jiggery-pokery from remote.

For the version/created-on information I agree it could be reasonable to have that be in its own list, keyed by ssl_profile_id, since this information is attached to the SSL profile when it is rotated through the certz service.

However, the solution which would minimise changes/disruption to the YANG model, maintain the relationship between the counters and the gRPC server, but still convey the information we want to capture here (i.e. association between ssl_profile_id and version/created-on and counters) would be to just add a new leaf under /oc-sys:system/oc-sys-grpc:grpc-servers/oc-sys-grpc:grpc-server/oc-sys-grpc:state containing the value of the ssl_profile_id. i.e. add this leaf under the grpc-server-credentials-state grouping:

    grouping grpc-server-credentials-state {
        description
            "gRPC server credentials freshness-related data.";

        leaf ssl-profile-id {
            type string;
            description
                "The ID of this gRPC server's SSL profile
                 as used by the gNSI Certz service";
        }

Please let me know what you think about this before remodeling the YANG :)

haussli commented 1 year ago

I think that having an ssl-profile-id associated with each grpc-server would solve this. Would you create a MR for that?

brianneville commented 1 year ago

opened https://github.com/openconfig/gnsi/pull/126, please take a look