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

api: Add parsed rules to policy response #6017

Merged
merged 14 commits into from
Nov 20, 2019
Merged

api: Add parsed rules to policy response #6017

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0fae6f1
Add parsed rules to policy response
backspace Jul 25, 2019
069f595
Merge branch 'master' into f-policy-json
backspace Aug 29, 2019
837d484
Add standard error-handling for parse failure
backspace Aug 29, 2019
0d4275b
Add changelog entry
backspace Aug 29, 2019
ffb10e6
Change parsing error to set rules to nil
backspace Aug 29, 2019
3206229
Change test to use valid HCL for rules
backspace Aug 29, 2019
5356a78
Merge branch 'master' into f-policy-json
backspace Aug 29, 2019
0f860f4
Merge branch 'master' into f-policy-json
backspace Sep 3, 2019
1eb3cf8
Merge branch 'master' into f-policy-json
backspace Nov 20, 2019
7c7637c
Remove merge error
backspace Nov 20, 2019
241ed98
Remove extraneous whitespace
backspace Nov 20, 2019
e5a2d13
Remove extraneous else block
backspace Nov 20, 2019
4f1dbc3
Add explanatory comment
backspace Nov 20, 2019
8b18396
Add gofmt changes
backspace Nov 20, 2019
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 CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
IMPROVEMENTS:
* agent: allow the job GC interval to be configured [[GH-5978](https://github.com/hashicorp/nomad/issues/5978)]
* agent: add `-dev=connect` parameter to support running in dev mode with Consul Connect [[GH-6126](https://github.com/hashicorp/nomad/issues/6126)]
* api: Added JSON representation of rules to policy endpoint response [[GH-6017](https://github.com/hashicorp/nomad/pull/6017)]
* api: add follow parameter to file streaming endpoint to support older browsers [[GH-6049](https://github.com/hashicorp/nomad/issues/6049)]
* metrics: Add job status (pending, running, dead) metrics [[GH-6003](https://github.com/hashicorp/nomad/issues/6003)]
* ui: Add creation time to evaluations table [[GH-6050](https://github.com/hashicorp/nomad/pull/6050)]
Expand Down
3 changes: 1 addition & 2 deletions command/acl_policy_info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"strings"
"testing"

"github.com/hashicorp/nomad/acl"
"github.com/hashicorp/nomad/command/agent"
"github.com/hashicorp/nomad/nomad/mock"
"github.com/hashicorp/nomad/nomad/structs"
Expand All @@ -31,7 +30,7 @@ func TestACLPolicyInfoCommand(t *testing.T) {
// Create a test ACLPolicy
policy := &structs.ACLPolicy{
Name: "testPolicy",
Rules: acl.PolicyWrite,
Rules: "node { policy = \"read\" }",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I changed the parse error-handling to return err as @schmichael suggested, it caused a test failure because write, as stored here, isn’t valid HCL. Since the Rules aren’t actually being used anywhere in the test, I changed it to store valid HCL instead.

}
policy.SetHash()
assert.Nil(state.UpsertACLPolicies(1000, []*structs.ACLPolicy{policy}))
Expand Down
9 changes: 9 additions & 0 deletions nomad/acl_endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
metrics "github.com/armon/go-metrics"
log "github.com/hashicorp/go-hclog"
memdb "github.com/hashicorp/go-memdb"
policy "github.com/hashicorp/nomad/acl"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend not shadowing the name here, when reading the code it can make interpretation difficult when there are multiple names for one package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my previous attempt at this had it called somethingacl because whenever I didn’t have an override, this whole line just got deleted! I’m guessing because acl is already defined in the document…????

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - I guess so :( - That's unfortunate but makes sense 👍

"github.com/hashicorp/nomad/helper/uuid"
"github.com/hashicorp/nomad/nomad/state"
"github.com/hashicorp/nomad/nomad/structs"
Expand Down Expand Up @@ -263,6 +264,14 @@ func (a *ACL) GetPolicy(args *structs.ACLPolicySpecificRequest, reply *structs.S
reply.Policy = out
if out != nil {
reply.Index = out.ModifyIndex

rules, err := policy.Parse(out.Rules)

backspace marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
} else {
Copy link
Member

Choose a reason for hiding this comment

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

No else { needed here. The Go idiom for the if err boilerplate is to just do an early return and not worry about nesting subsequent code in an else block. nbd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it, thanks.

reply.Policy.RulesJSON = rules
}
} else {
// Use the last index that affected the policy table
index, err := state.Index("acl_policy")
Expand Down
1 change: 1 addition & 0 deletions nomad/structs/structs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9006,6 +9006,7 @@ type ACLPolicy struct {
Name string // Unique name
Description string // Human readable
Rules string // HCL or JSON format
RulesJSON *acl.Policy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no particular attachment to this name, it’s awkward, I’m open to suggestions!

Copy link
Contributor

@endocrimes endocrimes Jul 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe Policy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s just the rules portion though, right? The whole thing is the policy already. I don’t knowwwwwah haha

Copy link
Member

Choose a reason for hiding this comment

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

We should also document that this field is generated from Rules on read. It's a bit unusual to have a field only in nomad/structs/structs.go and not in its corresponding api/acl.go model, so we should let people know why that is.

As far as naming, yeah it's awkward, but I think it fits. I feel like we confuse "ACL", "Rules", "Policies", "Capabilities", etc regularly but as this field is just a specific encoding of another field, we should follow the other field's naming.

Hash []byte
CreateIndex uint64
ModifyIndex uint64
Expand Down