Remove configuration constraint for k8s_psat #5216
Conversation
Closes spiffe#5211 We already scan the map of clusters at runtime, sending an error when the cluster is not found in the hclConfig.Clusters map. So, this just removes the need for at least one cluster to be configured. When zero clusters are configured, all requests will fail. Signed-off-by: Edwin Buck <edwbuck@gmail.com>
edwbuck
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
| @@ -225,10 +226,6 @@ func (p *AttestorPlugin) Configure(_ context.Context, req *configv1.ConfigureReq | |||
| return nil, status.Error(codes.InvalidArgument, "core configuration missing trust domain") | |||
| } | |||
|
|
|||
| if len(hclConfig.Clusters) == 0 { | |||
There was a problem hiding this comment.
Not having any clusters defined would be a misconfiguration for (most?) users that aren't dynamically adjusting the list. To aid debuggability, can you change this to emit a warning log instead? The warning log should be emitted down below, just before the call to p.setConfig, so that we only emit the warning when the configuration is actually going to change.
There was a problem hiding this comment.
A WARNING log message is appropriate. Expect an update today.
| @@ -20,6 +20,7 @@ The main configuration accepts the following values: | |||
| | Configuration | Description | Default | | |||
| |---------------|-----------------------------------------------------------------------------------|---------| | |||
| | `clusters` | A map of clusters, keyed by an arbitrary ID, that are authorized for attestation. | | | |||
| | | May be empty, when no clusters are authorized for attestation. | | | |||
There was a problem hiding this comment.
This renders as a separate row in the markdown and looks a little funny. I feel like we should just add this to the end of the sentence in the description above.
There was a problem hiding this comment.
Altered to be a warning note in the next push.
1. Log a warning when the k8s psat server count is zero. 2. Fix the documentation (markdown) to present better. Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Signed-off-by: Edwin Buck <edwbuck@gmail.com>
| @@ -226,6 +226,10 @@ func (p *AttestorPlugin) Configure(_ context.Context, req *configv1.ConfigureReq | |||
| return nil, status.Error(codes.InvalidArgument, "core configuration missing trust domain") | |||
| } | |||
|
|
|||
| if len(hclConfig.Clusters) < 1 { | |||
There was a problem hiding this comment.
This warning should ideally be moved to down to line 274 just before the call to p.setConfig. The configure call can still fail at this point (i.e., line 240) leaving the configuration unchanged. Issuing the warning before the config is actually changed could be misleading.
By moving the warning to after the errors, the errors will mask a warning on a config that doesn't get applied. Signed-off-by: Edwin Buck <edwbuck@gmail.com>
Closes #5211
We already scan the map of clusters at runtime, sending an error when the cluster is not found in the hclConfig.Clusters map. So, this just removes the need for at least one cluster to be configured.
When zero clusters are configured, all requests will fail.