CNTRLPLANE-3363: Add KMS plugin health reporter design

[ibihim]

What

A health reporter sidecar runs alongside every API server pod replica when KMS is enabled. It probes the colocated KMS plugin(s) and writes a single advisory KMSHealthReporter_<nodeName> condition per node on the apiserver operator CR.

Why

Exposes plugin health state through the operator CRs and onward into the ClusterOperator's Degraded condition, so a misbehaving KMS plugin is visible in oc get co rather than silently waiting until KAS encryption fails.

Supports future key rotation: per-plugin keyID in the reporter's Message lets a rotation controller verify all nodes agree on the active key before initiating rotation.

[openshift-ci-robot]

@ibihim: This pull request references CNTRLPLANE-3363 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set.

Details

In response to this:

What

A health reporter sidecar runs alongside every API server pod replica when KMS is enabled. It probes the colocated KMS plugin(s) and writes a single advisory KMSHealthReporter_<nodeName> condition per node on the apiserver operator CR. A separate aggregator controller reads those conditions and emits a single KMSPluginsDegraded rollup; library-go's StatusSyncer propagates the _Degraded suffix into the ClusterOperator's Degraded condition. The Message field on each per-node condition is structured input for the aggregator: one key=value line per probed plugin, carrying keyID, status, lastChecked, and an optional trailing detail.

Why

Exposes plugin health state through the operator CRs and onward into the ClusterOperator's Degraded condition, so a misbehaving KMS plugin is visible in oc get co rather than silently waiting until KAS encryption fails.

Supports future key rotation: per-plugin keyID in the reporter's Message lets a rotation controller verify all nodes agree on the active key before initiating rotation.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign benluddy for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ardaguclu]

I was thinking there will be one health reporter sidecar per each kms plugin (i.e. health reporter a for kms plugin a, health reporter b for kms plugin b, etc.) not single health reporter plugin for all kms plugins. Is that correct?.

[ardaguclu]

Plugin lifecycle is supposed to inject kms plugins. That means it will additionally inject one health monitor side car?.

[tjungblu]

yeah it's a bit contradicting with the sections below. I was also under the assumption we had a 1:1 monitor:plugin relation.

[ibihim]

What

One reporter sidecar per API server pod, not one per plugin. It probes every colocated KMS socket and emits a single per-node condition whose Message carries one line per plugin (see Message format below).

Why

• One monitor per node keeps condition cardinality constant. With one monitor per plugin, it won't (because of SSA / field ownership).
• I'm assuming the injector can run one reporter per pod with all plugin sockets exposed to it, by passing socket paths as flags at startup.

If the above turns out to be harder than 1:1 injection in practice, I will change it.

[ardaguclu]

by checking the current EncryptionConfiguration?

[ibihim]

The health reporter is given the UDS paths as a flag argument. Technically it could also scan for mounted UDS sockets.

[ardaguclu]

Can you give an example about how kms-2.sock and kms-1.sock will be represented in operator CR?

[ardaguclu]

I saw example below.

[ardaguclu]

From where the details of RPC error will be seen?

[ibihim]

Every call to the KMS plugin can create an error as per interface:

	Status(ctx context.Context) (*StatusResponse, error) // error = RPC error.

[ardaguclu]

How can cluster admin see the RPC error details from the operator conditions reported by health monitor?. cluster admin can not send request to Status endpoint of kms plugin.

[ibihim]

The cluster admin will see the aggregated status created by the condition controller.

Is this a CTA, to add this notion to the EP?

[ibihim]

Added a paragraph for this: 771b367

[ardaguclu]

How can this format be parsed from other controllers (i.e. rotation controller)

[ibihim]

I tried to replicate

      message: |-
        Available: The registry is ready
        NodeCADaemonAvailable: The daemon set node-ca has available replicas
        ImagePrunerAvailable: Pruner CronJob has been created

If you prefer, I can dump json encoded data into it.

[ibihim]

Agreed to use minified JSON

[ardaguclu]

This data probably should be in decodable format instead of one liner. So that it can be usable.

[ibihim]

Agreed to use minified JSON

[ardaguclu]

TBH, I didn't understand anything from this paragraph :). Can we update it better clarify.

[ibihim]

I tried to explain the SSA dynamic. I can drop the section completely.

operator.openshift.io/v1.KubeAPIServer has a status that, where the health reporter would write conditions to. So there is a potential conflict that can be resolved by SSA with +listType=map, but this is a thing we should be cautious of.

[ardaguclu]

Could you please mention about who is going to inject this side car.

[ibihim]

https://github.com/ibihim/enhancements/blob/2bb10c537d05893622c0340a2b2322d6d0765abd/enhancements/kube-apiserver/kms-encryption-foundations.md#health-reporter-sidecar now mentions the pluginlifecycle package.

[ardaguclu]

Is this "aggregator controller" referring to existing controller or will be implemented?

[ibihim]

Honestly, I was not sure. I would have hoped to let it handle by the conditionController.

[ardaguclu]

Will there be a change in condition controller?. That means there is a change in encryption controllers. I think we should reflect this what changes are expected?.

[ibihim]

As we discussed, we will see if it works to put it into the condition controller and if not, we will create a new one.

[p0lyn0mial]

LastTransitionTime, which records state flips only

what does it mean ? does changing the msg counts as changing the state ?

[ibihim]

Timestamp means, whenever the response from the KMS plugin arrived to the health reporter.
It explicitly is not a LTT, based on the suggestion you made earlier.

This means you can derive staleness from now > timestamp + probe interval + skew

[p0lyn0mial]

I think that in general we miss:

1. Specifying the probe interval
2. Specifying the creds used to publish the condition
3. Explaining what happens with orphaned conditions
4. A plugin identity, when multiple plugins are specified - otherwise we cannot map status to a specific plugin.
5. Listing the alternatives we discussed
6. Specifying how the reporter connects to the apiserver. Perhaps for KAS we could go via localhost

[ibihim]

1. Yes, this is worthy to elaborate to balance detection and hammering KAS with SSA request.
2. I assumed we would have a SA with RBAC.
3. https://github.com/openshift/enhancements/pull/2005/changes#diff-c0e8034f61b2841cee9b2b1d07403961148c3a30f096b0d35720a57ead8d3697R660
4. In the status of the condition each health report has its own line. I assumed that the keyID would make it unique. Does it not?
5. Ok.
6. See 2.

[ibihim]

1. Set to 30s.
2. legacy SA token with RBAC for now.
3. Node + keyID
4. oh damn. I will add that.
5. service is slightly better, but localhost is worth mentioning.

[ibihim]

Added a section in alternatives to cover the options discussed: b411a09

[p0lyn0mial]

State or Status?

[p0lyn0mial]

I would drop it for now. We don't know how rotation will work yet.

/cc @tjungblu

[tjungblu]

no we should include this :) at least that makes my life easier to reference this mechanism in the other enhancement.

[ibihim]

@tjungblu work on the key rotation was the motivation to include it.

[p0lyn0mial]

We talked about the possibility of using the mechanism produced by the reporter to publish information about the keys. But we still don’t know the detail, so for now I’d prefer to remove the information about the keyID until then.

I also spoke with @benluddy yesterday, and he would like to add an API instead of using conditions.

We will probably also change the health reporting later, but he agreed that initially we can use conditions for health reporting.

[ibihim]

I think we agreed, to show keyID and kekID for now as minified JSON in the message. Correct me if I am wrong.

[p0lyn0mial]

not sure if we should change this filed. The msg could be interpreted by the controller

[ibihim]

OK, this is a good point that would need investigation.

[p0lyn0mial]

not sure if we should change this filed.

[p0lyn0mial]

is the only field whose value may contain spaces; it is always last on its line

does it mean we need to escape spaces in the detail field ?

[ibihim]

not directly, but as I mentioned to @ardaguclu, I tried to replicate the existing format. I could use JSON if this is preferred.

[ardaguclu]

However, this states each sidecar probes its colocated KMS Plugins

[tjungblu]

... and is it the keyID though? I would assume we would allocate a unique socket path on each node like:

/var/run/kmsplugin/kms-kube-apiserver/{encryptionconfig.Name (?) }/plugin.sock

not sure whether we need to separate via folders here though

there's also no sidecar-injection section either 😄

[ibihim]

@ardaguclu, is this a question? Each sidecar probes its colocated KMS Plugins. Yes, this is correct.

I added a "Naming caveat" section.

nodeName + keyID should be unique.

There is a sidecar injection section: https://github.com/ibihim/enhancements/blob/2bb10c537d05893622c0340a2b2322d6d0765abd/enhancements/kube-apiserver/kms-encryption-foundations.md#sidecar-injection

[ardaguclu]

Why are we trying to skip StatusSyncer?

[ibihim]

Based on a discussion with @p0lyn0mial, a dedicated controller would create a condition that will be read by the StatusSyncer. This will reduce the amount of conditions.

So the "aggregator controller" will create a condition that will be read by the StatusSyncer.

[tjungblu]

heh there's no multiple-concurrent-sidecars section :)

[ibihim]

There is?

https://github.com/ibihim/enhancements/blob/2bb10c537d05893622c0340a2b2322d6d0765abd/enhancements/kube-apiserver/kms-encryption-foundations.md#multiple-concurrent-sidecars

[tjungblu]

hah, that was two weeks ago. I think you just forgot to push a few commits back then.

[tjungblu]

what's the difference between RPC error and unhealthy?

[ibihim]

The KMS Plugin can report healthy / unhealthy. An error would indicate that something is wrong with the connection itself.

[ardaguclu]

Errors are usually very long (not short like connection refused). Would this formatting still work for very long error traces?

[ardaguclu]

I agree with the justification here.

[tjungblu]

yep, same here

[ardaguclu]

In SSA, will there be a fight between health reporter that tries to update the condition and aggregator controller that tries to cleanup the condition?.

[tjungblu]

I don't think it's going to be a big problem when they stomp on each other, we might have some flapping during non-ready/deleted states though.

I wonder if we rather have top level condition of the KMSHealthReporter_<nodeName> owned by the aggregator controller.

Kinda similar to how the node controller works:
https://github.com/openshift/library-go/blob/3a6f949c22c3ffc0a2b28ee50e78173086e7adae/pkg/operator/staticpod/controller/node/node_controller.go

the sidecar would only ever write to it when the condition for said node exists. Sounds like too much coordination for a few conflicts between node state transitions...

for the later API, we could just use a map and key it off the nodename directly https://github.com/openshift/api/pull/2850/changes#diff-616d67895c3421c2d091662d30ed47b4b6f0b57db9411e29618d22b964ddb9efR362

[ardaguclu]

You know better than me those conditions. In SSA, it is always better a controller fully owns the field.

[ibihim]

The *Degraded, *Available and so on are owned by the aggregator controller.
The aggregator controller itself would remove KMSHealthReporter_ that are owned by the KMS health reporters, not write into them. So there is some flapping, but it shouldn't be for too long / it shouldn't be a problem. Because the removal happens during garbage collection.

We could tear down the health reporter down before we tear down the KMS plugin, so we don't have wrong error conditions.

[ardaguclu]

I have just one question. Other than that changes look good to me.

[tjungblu]

Suggested change

	**Risk: Orphaned Conditions on Mode Switch**
	**Risk: Orphaned Conditions on Mode Switch**
	**Risk: Orphaned Conditions on encryption type change and node replacement**

Just wanted to ask who is cleaning up the old orphaned nodes when a node goes away, seems it is just buried in the Mitigation here :) Let's make the title clearer.

[ibihim]

Encryption type change is more accurate, but "node replacement", is this something that is a major concern? I mean we can add it, it is correct.

[tjungblu]

we can scale the control plane up and down anytime, so yes, this is kinda important

[ibihim]

I will change it. Can't apply your commit as it contains the previous line as well.

[openshift-ci]

@ibihim: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.