Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove configuration constraint for k8s_psat #5216

Merged
merged 6 commits into from
Jun 14, 2024
Merged

Remove configuration constraint for k8s_psat #5216

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
82249d9
Remove configuration constraint.
edwbuck Jun 11, 2024
1acda0b
Updates requested in reveiw.
edwbuck Jun 14, 2024
d4d70ad
Add in the block markers to make the Warning work.
edwbuck Jun 14, 2024
36113e8
Clarify the setting value with code highlighting.
edwbuck Jun 14, 2024
dcd9d16
Moved warning about zero configured clusters after errors.
edwbuck Jun 14, 2024
dc96106
Merge branch 'main' into zero_cluster
azdagron Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/plugin_server_nodeattestor_k8s_psat.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. | |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Altered to be a warning note in the next push.


Each cluster in the main configuration requires the following configuration:

Expand Down
5 changes: 1 addition & 4 deletions pkg/server/plugin/nodeattestor/k8spsat/psat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ func (p *AttestorPlugin) Attest(stream nodeattestorv1.NodeAttestor_AttestServer)

func (p *AttestorPlugin) Configure(_ context.Context, req *configv1.ConfigureRequest) (*configv1.ConfigureResponse, error) {
hclConfig := new(AttestorConfig)

if err := hcl.Decode(hclConfig, req.HclConfiguration); err != nil {
return nil, status.Errorf(codes.InvalidArgument, "unable to decode configuration: %v", err)
}
Expand All @@ -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 {
Copy link
Member

@azdagron azdagron Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A WARNING log message is appropriate. Expect an update today.

return nil, status.Error(codes.InvalidArgument, "configuration must have at least one cluster")
}

config := &attestorConfig{
trustDomain: req.CoreConfiguration.TrustDomain,
clusters: make(map[string]*clusterConfig),
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/plugin/nodeattestor/k8spsat/psat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ func (s *AttestorSuite) TestConfigure() {

// missing clusters
err = doConfig(coreConfig, "")
s.RequireGRPCStatus(err, codes.InvalidArgument, "configuration must have at least one cluster")
s.Require().NoError(err)

// cluster missing service account allow list
err = doConfig(coreConfig, `clusters = {
Expand Down
Loading