From 8ba760a2fc2cdf69dfc939c36dcb7eb8d7dd2f28 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 15 Oct 2021 13:33:31 -0400 Subject: [PATCH] acl: remove id and revision from Policy constructors The fields were removed in a previous commit. Also remove an unused constructor for PolicyMerger --- acl/acl_test.go | 2 +- acl/authorizer.go | 4 +-- acl/policy.go | 3 +- acl/policy_merger.go | 6 ---- acl/policy_test.go | 4 +-- agent/acl_test.go | 2 +- agent/consul/acl_endpoint.go | 2 +- agent/consul/acl_test.go | 34 +++++++++---------- agent/consul/catalog_endpoint_test.go | 14 ++++---- agent/consul/filter_test.go | 4 +-- agent/rpc/subscribe/subscribe_test.go | 7 ++-- agent/structs/acl.go | 2 +- .../config_entry_discoverychain_test.go | 4 +-- agent/structs/config_entry_test.go | 2 +- agent/structs/intention_test.go | 2 +- agent/xds/delta_test.go | 6 ++-- 16 files changed, 44 insertions(+), 54 deletions(-) diff --git a/acl/acl_test.go b/acl/acl_test.go index c99c564475..3bbfed25eb 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -2549,7 +2549,7 @@ func TestACL_ReadAll(t *testing.T) { body := func(t *testing.T, rules string, defaultPolicy Authorizer, check func(t *testing.T, authz Authorizer, prefix string, entCtx *AuthorizerContext)) { t.Helper() - policy, err := NewPolicyFromSource("", 0, rules, SyntaxCurrent, nil, nil) + policy, err := NewPolicyFromSource(rules, SyntaxCurrent, nil, nil) require.NoError(t, err) acl, err := NewPolicyAuthorizerWithDefaults(defaultPolicy, []*Policy{policy}, nil) diff --git a/acl/authorizer.go b/acl/authorizer.go index 43d50544bf..8c4f7aa926 100644 --- a/acl/authorizer.go +++ b/acl/authorizer.go @@ -267,8 +267,8 @@ func Enforce(authz Authorizer, rsc Resource, segment string, access string, ctx // NewAuthorizerFromRules is a convenience function to invoke NewPolicyFromSource followed by NewPolicyAuthorizer with // the parse policy. -func NewAuthorizerFromRules(id string, revision uint64, rules string, syntax SyntaxVersion, conf *Config, meta *EnterprisePolicyMeta) (Authorizer, error) { - policy, err := NewPolicyFromSource(id, revision, rules, syntax, conf, meta) +func NewAuthorizerFromRules(rules string, syntax SyntaxVersion, conf *Config, meta *EnterprisePolicyMeta) (Authorizer, error) { + policy, err := NewPolicyFromSource(rules, syntax, conf, meta) if err != nil { return nil, err } diff --git a/acl/policy.go b/acl/policy.go index f19e7a6be4..d4ebd5976a 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -427,8 +427,7 @@ func parseLegacy(rules string, conf *Config) (*Policy, error) { // NewPolicyFromSource is used to parse the specified ACL rules into an // intermediary set of policies, before being compiled into // the ACL -// TODO: remove id and revision args -func NewPolicyFromSource(id string, revision uint64, rules string, syntax SyntaxVersion, conf *Config, meta *EnterprisePolicyMeta) (*Policy, error) { +func NewPolicyFromSource(rules string, syntax SyntaxVersion, conf *Config, meta *EnterprisePolicyMeta) (*Policy, error) { if rules == "" { // Hot path for empty source return &Policy{}, nil diff --git a/acl/policy_merger.go b/acl/policy_merger.go index 681772d04f..cceb62dd9c 100644 --- a/acl/policy_merger.go +++ b/acl/policy_merger.go @@ -313,12 +313,6 @@ type PolicyMerger struct { enterprisePolicyRulesMergeContext } -func NewPolicyMerger() *PolicyMerger { - merger := &PolicyMerger{} - merger.init() - return merger -} - func (m *PolicyMerger) init() { m.policyRulesMergeContext.init() m.enterprisePolicyRulesMergeContext.init() diff --git a/acl/policy_test.go b/acl/policy_test.go index 46f8fb24ef..fd0e9dba50 100644 --- a/acl/policy_test.go +++ b/acl/policy_test.go @@ -771,7 +771,7 @@ func TestPolicySourceParse(t *testing.T) { require.True(t, tc.Rules != "" || tc.RulesJSON != "") if tc.Rules != "" { t.Run("hcl", func(t *testing.T) { - actual, err := NewPolicyFromSource("", 0, tc.Rules, tc.Syntax, nil, nil) + actual, err := NewPolicyFromSource(tc.Rules, tc.Syntax, nil, nil) if tc.Err != "" { errStartsWith(t, err, tc.Err) } else { @@ -781,7 +781,7 @@ func TestPolicySourceParse(t *testing.T) { } if tc.RulesJSON != "" { t.Run("json", func(t *testing.T) { - actual, err := NewPolicyFromSource("", 0, tc.RulesJSON, tc.Syntax, nil, nil) + actual, err := NewPolicyFromSource(tc.RulesJSON, tc.Syntax, nil, nil) if tc.Err != "" { errStartsWith(t, err, tc.Err) } else { diff --git a/agent/acl_test.go b/agent/acl_test.go index e3b3f5ca5f..d979fd5bd1 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -237,7 +237,7 @@ func catalogPolicy(token string) (structs.ACLIdentity, acl.Authorizer, error) { return nil, nil, acl.ErrNotFound } - policy, err := acl.NewPolicyFromSource("", 0, tok.rules, acl.SyntaxCurrent, nil, nil) + policy, err := acl.NewPolicyFromSource(tok.rules, acl.SyntaxCurrent, nil, nil) if err != nil { return nil, nil, err } diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go index 6370924016..f3e7f39809 100644 --- a/agent/consul/acl_endpoint.go +++ b/agent/consul/acl_endpoint.go @@ -1176,7 +1176,7 @@ func (a *ACL) PolicySet(args *structs.ACLPolicySetRequest, reply *structs.ACLPol } // validate the rules - _, err = acl.NewPolicyFromSource("", 0, policy.Rules, policy.Syntax, a.srv.aclConfig, policy.EnterprisePolicyMeta()) + _, err = acl.NewPolicyFromSource(policy.Rules, policy.Syntax, a.srv.aclConfig, policy.EnterprisePolicyMeta()) if err != nil { return err } diff --git a/agent/consul/acl_test.go b/agent/consul/acl_test.go index 9ebf93bc27..61b4df204b 100644 --- a/agent/consul/acl_test.go +++ b/agent/consul/acl_test.go @@ -2172,7 +2172,7 @@ func TestACL_filterHealthChecks(t *testing.T) { } // Allowed to see the service but not the node. - policy, err := acl.NewPolicyFromSource("", 0, ` + policy, err := acl.NewPolicyFromSource(` service "foo" { policy = "read" } @@ -2195,7 +2195,7 @@ service "foo" { } // Chain on access to the node. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` node "node1" { policy = "read" } @@ -2253,7 +2253,7 @@ func TestACL_filterIntentions(t *testing.T) { } // Policy to see one - policy, err := acl.NewPolicyFromSource("", 0, ` + policy, err := acl.NewPolicyFromSource(` service "foo" { policy = "read" } @@ -2328,7 +2328,7 @@ func TestACL_filterServiceNodes(t *testing.T) { } // Allowed to see the service but not the node. - policy, err := acl.NewPolicyFromSource("", 0, ` + policy, err := acl.NewPolicyFromSource(` service "foo" { policy = "read" } @@ -2352,7 +2352,7 @@ service "foo" { } // Chain on access to the node. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` node "node1" { policy = "read" } @@ -2424,7 +2424,7 @@ func TestACL_filterNodeServices(t *testing.T) { } // Allowed to see the service but not the node. - policy, err := acl.NewPolicyFromSource("", 0, ` + policy, err := acl.NewPolicyFromSource(` service "foo" { policy = "read" } @@ -2448,7 +2448,7 @@ service "foo" { } // Chain on access to the node. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` node "node1" { policy = "read" } @@ -2520,7 +2520,7 @@ func TestACL_filterCheckServiceNodes(t *testing.T) { } // Allowed to see the service but not the node. - policy, err := acl.NewPolicyFromSource("", 0, ` + policy, err := acl.NewPolicyFromSource(` service "foo" { policy = "read" } @@ -2543,7 +2543,7 @@ service "foo" { } // Chain on access to the node. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` node "node1" { policy = "read" } @@ -2646,7 +2646,7 @@ node "node1" { service "foo" { policy = "read" }` - policy, err := acl.NewPolicyFromSource("", 0, rules, acl.SyntaxLegacy, nil, nil) + policy, err := acl.NewPolicyFromSource(rules, acl.SyntaxLegacy, nil, nil) if err != nil { t.Fatalf("err %v", err) } @@ -2674,7 +2674,7 @@ node "node2" { service "bar" { policy = "read" }` - policy, err := acl.NewPolicyFromSource("", 0, rules, acl.SyntaxLegacy, nil, nil) + policy, err := acl.NewPolicyFromSource(rules, acl.SyntaxLegacy, nil, nil) if err != nil { t.Fatalf("err %v", err) } @@ -2708,7 +2708,7 @@ node "node2" { service "bar" { policy = "read" }` - policy, err := acl.NewPolicyFromSource("", 0, rules, acl.SyntaxLegacy, nil, nil) + policy, err := acl.NewPolicyFromSource(rules, acl.SyntaxLegacy, nil, nil) if err != nil { t.Fatalf("err %v", err) } @@ -2837,7 +2837,7 @@ func TestACL_filterNodeDump(t *testing.T) { } // Allowed to see the service but not the node. - policy, err := acl.NewPolicyFromSource("", 0, ` + policy, err := acl.NewPolicyFromSource(` service "foo" { policy = "read" } @@ -2861,7 +2861,7 @@ service "foo" { } // Chain on access to the node. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` node "node1" { policy = "read" } @@ -2970,7 +2970,7 @@ func TestACL_filterDatacenterCheckServiceNodes(t *testing.T) { perms acl.Authorizer ) // Allowed to see the service but not the node. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` service_prefix "" { policy = "read" } `, acl.SyntaxCurrent, nil, nil) require.NoError(t, err) @@ -2985,7 +2985,7 @@ func TestACL_filterDatacenterCheckServiceNodes(t *testing.T) { } // Allowed to see the node but not the service. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` node_prefix "" { policy = "read" } `, acl.SyntaxCurrent, nil, nil) require.NoError(t, err) @@ -3000,7 +3000,7 @@ func TestACL_filterDatacenterCheckServiceNodes(t *testing.T) { } // Allowed to see the service AND the node - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` service_prefix "" { policy = "read" } node_prefix "" { policy = "read" } `, acl.SyntaxCurrent, nil, nil) diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index a4626facae..4433274ec7 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/agent/consul/catalog_endpoint_test.go @@ -3437,7 +3437,7 @@ func TestVetRegisterWithACL(t *testing.T) { } // Create a basic node policy. - policy, err := acl.NewPolicyFromSource("", 0, ` + policy, err := acl.NewPolicyFromSource(` node "node" { policy = "write" } @@ -3482,7 +3482,7 @@ node "node" { } // Chain on a basic service policy. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` service "service" { policy = "write" } @@ -3512,7 +3512,7 @@ service "service" { } // Chain on a policy that allows them to write to the other service. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` service "other" { policy = "write" } @@ -3586,7 +3586,7 @@ service "other" { } // Chain on a policy that forbids them to write to the other service. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` service "other" { policy = "deny" } @@ -3616,7 +3616,7 @@ service "other" { } // Chain on a policy that forbids them to write to the node. - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` node "node" { policy = "deny" } @@ -3663,7 +3663,7 @@ func TestVetDeregisterWithACL(t *testing.T) { } // Create a basic node policy. - policy, err := acl.NewPolicyFromSource("", 0, ` + policy, err := acl.NewPolicyFromSource(` node "node" { policy = "write" } @@ -3676,7 +3676,7 @@ node "node" { t.Fatalf("err: %v", err) } - policy, err = acl.NewPolicyFromSource("", 0, ` + policy, err = acl.NewPolicyFromSource(` service "my-service" { policy = "write" } diff --git a/agent/consul/filter_test.go b/agent/consul/filter_test.go index 396ebbbb0b..415ac762ab 100644 --- a/agent/consul/filter_test.go +++ b/agent/consul/filter_test.go @@ -10,7 +10,7 @@ import ( func TestFilter_DirEnt(t *testing.T) { t.Parallel() - policy, _ := acl.NewPolicyFromSource("", 0, testFilterRules, acl.SyntaxLegacy, nil, nil) + policy, _ := acl.NewPolicyFromSource(testFilterRules, acl.SyntaxLegacy, nil, nil) aclR, _ := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) type tcase struct { @@ -52,7 +52,7 @@ func TestFilter_DirEnt(t *testing.T) { func TestFilter_TxnResults(t *testing.T) { t.Parallel() - policy, _ := acl.NewPolicyFromSource("", 0, testFilterRules, acl.SyntaxLegacy, nil, nil) + policy, _ := acl.NewPolicyFromSource(testFilterRules, acl.SyntaxLegacy, nil, nil) aclR, _ := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) type tcase struct { diff --git a/agent/rpc/subscribe/subscribe_test.go b/agent/rpc/subscribe/subscribe_test.go index 531d8ec820..a4a5e24177 100644 --- a/agent/rpc/subscribe/subscribe_test.go +++ b/agent/rpc/subscribe/subscribe_test.go @@ -612,7 +612,7 @@ node "node1" { } ` cfg := &acl.Config{WildcardName: structs.WildcardSpecifier} - authorizer, err := acl.NewAuthorizerFromRules("1", 0, rules, acl.SyntaxCurrent, cfg, nil) + authorizer, err := acl.NewAuthorizerFromRules(rules, acl.SyntaxCurrent, cfg, nil) require.NoError(t, err) authorizer = acl.NewChainedAuthorizer([]acl.Authorizer{authorizer, acl.DenyAll()}) require.Equal(t, acl.Deny, authorizer.NodeRead("denied", nil)) @@ -808,10 +808,7 @@ node "node1" { policy = "write" } ` - authorizer, err := acl.NewAuthorizerFromRules( - "1", 0, rules, acl.SyntaxCurrent, - &acl.Config{WildcardName: structs.WildcardSpecifier}, - nil) + authorizer, err := acl.NewAuthorizerFromRules(rules, acl.SyntaxCurrent, &acl.Config{WildcardName: structs.WildcardSpecifier}, nil) require.NoError(t, err) authorizer = acl.NewChainedAuthorizer([]acl.Authorizer{authorizer, acl.DenyAll()}) require.Equal(t, acl.Deny, authorizer.NodeRead("denied", nil)) diff --git a/agent/structs/acl.go b/agent/structs/acl.go index 8f5e23c028..ae63878bc7 100644 --- a/agent/structs/acl.go +++ b/agent/structs/acl.go @@ -725,7 +725,7 @@ func (policies ACLPolicies) resolveWithCache(cache *ACLCaches, entConf *acl.Conf continue } - p, err := acl.NewPolicyFromSource(policy.ID, policy.ModifyIndex, policy.Rules, policy.Syntax, entConf, policy.EnterprisePolicyMeta()) + p, err := acl.NewPolicyFromSource(policy.Rules, policy.Syntax, entConf, policy.EnterprisePolicyMeta()) if err != nil { return nil, fmt.Errorf("failed to parse %q: %v", policy.Name, err) } diff --git a/agent/structs/config_entry_discoverychain_test.go b/agent/structs/config_entry_discoverychain_test.go index 37f41e925b..a3fb49b4ae 100644 --- a/agent/structs/config_entry_discoverychain_test.go +++ b/agent/structs/config_entry_discoverychain_test.go @@ -17,7 +17,7 @@ func TestConfigEntries_ListRelatedServices_AndACLs(t *testing.T) { // This test tests both of these because they are related functions. newAuthz := func(t *testing.T, src string) acl.Authorizer { - policy, err := acl.NewPolicyFromSource("", 0, src, acl.SyntaxCurrent, nil, nil) + policy, err := acl.NewPolicyFromSource(src, acl.SyntaxCurrent, nil, nil) require.NoError(t, err) authorizer, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) @@ -34,7 +34,7 @@ func TestConfigEntries_ListRelatedServices_AndACLs(t *testing.T) { buf.WriteString(fmt.Sprintf("service %q { policy = %q }\n", s, "write")) } - policy, err := acl.NewPolicyFromSource("", 0, buf.String(), acl.SyntaxCurrent, nil, nil) + policy, err := acl.NewPolicyFromSource(buf.String(), acl.SyntaxCurrent, nil, nil) require.NoError(t, err) authorizer, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) diff --git a/agent/structs/config_entry_test.go b/agent/structs/config_entry_test.go index 1711e8306b..294c9e40bd 100644 --- a/agent/structs/config_entry_test.go +++ b/agent/structs/config_entry_test.go @@ -23,7 +23,7 @@ func TestConfigEntries_ACLs(t *testing.T) { type testcase = configEntryACLTestCase newAuthz := func(t *testing.T, src string) acl.Authorizer { - policy, err := acl.NewPolicyFromSource("", 0, src, acl.SyntaxCurrent, nil, nil) + policy, err := acl.NewPolicyFromSource(src, acl.SyntaxCurrent, nil, nil) require.NoError(t, err) authorizer, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) diff --git a/agent/structs/intention_test.go b/agent/structs/intention_test.go index 6ca822c509..9b733e7f7b 100644 --- a/agent/structs/intention_test.go +++ b/agent/structs/intention_test.go @@ -117,7 +117,7 @@ func TestIntention_ACLs(t *testing.T) { for name, tcase := range cases { t.Run(name, func(t *testing.T) { - authz, err := acl.NewAuthorizerFromRules("", 0, tcase.rules, acl.SyntaxCurrent, &config, nil) + authz, err := acl.NewAuthorizerFromRules(tcase.rules, acl.SyntaxCurrent, &config, nil) require.NoError(t, err) require.Equal(t, tcase.read, tcase.intention.CanRead(authz)) diff --git a/agent/xds/delta_test.go b/agent/xds/delta_test.go index 56d1c93ec3..99a1f0393b 100644 --- a/agent/xds/delta_test.go +++ b/agent/xds/delta_test.go @@ -950,7 +950,7 @@ func TestServer_DeltaAggregatedResources_v3_ACLEnforcement(t *testing.T) { // Ensure the correct token was passed require.Equal(t, tt.token, id) // Parse the ACL and enforce it - policy, err := acl.NewPolicyFromSource("", 0, tt.acl, acl.SyntaxLegacy, nil, nil) + policy, err := acl.NewPolicyFromSource(tt.acl, acl.SyntaxLegacy, nil, nil) require.NoError(t, err) return acl.NewPolicyAuthorizerWithDefaults(acl.RootAuthorizer("deny"), []*acl.Policy{policy}, nil) } @@ -1014,7 +1014,7 @@ func TestServer_DeltaAggregatedResources_v3_ACLTokenDeleted_StreamTerminatedDuri aclRules := `service "web" { policy = "write" }` token := "service-write-on-web" - policy, err := acl.NewPolicyFromSource("", 0, aclRules, acl.SyntaxLegacy, nil, nil) + policy, err := acl.NewPolicyFromSource(aclRules, acl.SyntaxLegacy, nil, nil) require.NoError(t, err) var validToken atomic.Value @@ -1112,7 +1112,7 @@ func TestServer_DeltaAggregatedResources_v3_ACLTokenDeleted_StreamTerminatedInBa aclRules := `service "web" { policy = "write" }` token := "service-write-on-web" - policy, err := acl.NewPolicyFromSource("", 0, aclRules, acl.SyntaxLegacy, nil, nil) + policy, err := acl.NewPolicyFromSource(aclRules, acl.SyntaxLegacy, nil, nil) require.NoError(t, err) var validToken atomic.Value