This repository has been archived by the owner on Dec 20, 2022. It is now read-only.
fix policy cache missing #26
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WindzCUHK
reviewed
Jul 22, 2019
| @@ -330,7 +310,7 @@ func (p *policyd) fetchPolicy(ctx context.Context, domain string) (*SignedPolicy | |||
| return sp, true, nil | |||
| } | |||
|
|
|||
| func simplifyAndCachePolicy(ctx context.Context, rp gache.Gache, sp *SignedPolicy) error { | |||
| func simplifyAndCachePolicy(ctx context.Context, dom string, sp *SignedPolicy) (map[string][]*Assertion, error) { | |||
There was a problem hiding this comment.
Suggested change
| func simplifyAndCachePolicy(ctx context.Context, dom string, sp *SignedPolicy) (map[string][]*Assertion, error) { | |
| func simplifyPolicy(ctx context.Context, dom string, sp *SignedPolicy) (map[string][]*Assertion, error) { |
| continue | ||
| } | ||
|
|
||
| // remove duplication of `role+action+reource' assertion |
There was a problem hiding this comment.
Suggested change
| // remove duplication of `role+action+reource' assertion | |
| // remove duplication of `role+action+resource' assertion |
| @@ -343,6 +323,12 @@ func simplifyAndCachePolicy(ctx context.Context, rp gache.Gache, sp *SignedPolic | |||
| case <-ctx.Done(): | |||
| return ctx.Err() | |||
| default: | |||
| if strings.SplitN(ass.Role, ":role.", 2)[0] != dom { | |||
| glg.Errorf("role is not matches with the domain, role: %v, domain: %v", ass.Role, dom) | |||
There was a problem hiding this comment.
Suggested change
| glg.Errorf("role is not matches with the domain, role: %v, domain: %v", ass.Role, dom) | |
| glg.Debugf("role is not matches with the domain, role: %v, domain: %v", ass.Role, dom) |
| // cache | ||
| var retErr error | ||
| assm.Range(func(k interface{}, val interface{}) bool { | ||
| ass := val.(*util.Assertion) | ||
| a, err := NewAssertion(ass.Action, ass.Resource, ass.Effect) | ||
| if err != nil { | ||
| glg.Debugf("error adding assertion to the cache, err: %v", err) | ||
| glg.Errorf("error adding assertion to the cache, assertion: %+v, err: %v", ass, err) |
There was a problem hiding this comment.
Suggested change
| glg.Errorf("error adding assertion to the cache, assertion: %+v, err: %v", ass, err) | |
| glg.Errorf("error creating policy.Assertion, assertion: %+v, err: %v", ass, err) |
| SetExpiredHook(func(ctx context.Context, key string) { | ||
| //key = <domain>:role.<role> | ||
| p.fetchAndCachePolicy(ctx, strings.Split(key, ":role.")[0]) | ||
| }) |
There was a problem hiding this comment.
- ExpiredHook is called after delete, checking policy between time gap with cause error
- retry on ExpiredHook fail?
- fetchAndCachePolicy() error is not print inside the ExpiredHook
| if roleAsss, ok := p.rolePolicies.Get(domain); ok { | ||
| if asss, ok := roleAsss.(map[string][]*Assertion)[r]; ok { | ||
| for _, ass := range asss { | ||
| glg.Debugf("Checking policy domain: %s, role: %v, action: %s, resource: %s, assertion: %v", domain, roles, action, resource, ass) | ||
| if strings.EqualFold(ass.ResourceDomain, domain) && ass.Reg.MatchString(strings.ToLower(action+"-"+resource)) { | ||
| ch <- ass.Effect |
There was a problem hiding this comment.
I think we will have deadlock here, when multiple deny policy match.
- ech size = 1
- read from ech only 1 times
- when, write more than 2, the go func cannot return, wg.Wait() will block forever
I think the old logic may still have the same problem,
but the old logic handles ctx cancel in a better way,
better to keep the old logic first.
|
|
||
| go func() { | ||
| defer close(ech) |
There was a problem hiding this comment.
better to close the channel explicitly
|
closed by #27 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.