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.

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

fix(authz)!: prunning: unable to execute authorization #10633

Closed
wants to merge 8 commits into from

Conversation

aleem1314
Copy link
Contributor

Description

Closes: #10611


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added the C:CLI label Nov 29, 2021
x/authz/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/authz/keeper/keeper.go Outdated Show resolved Hide resolved
Copy link
Contributor

@atheeshp atheeshp left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

We still need to be able to remove expired grants.

x/authz/keeper/keeper.go Outdated Show resolved Hide resolved
x/authz/keeper/grpc_query.go Show resolved Hide resolved
if req.MsgTypeUrl != "" {
authorization, expiration := k.GetCleanAuthorization(ctx, grantee, granter, req.MsgTypeUrl)
Copy link
Collaborator

Choose a reason for hiding this comment

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

so why we don't want to clean authorization if it's expired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expired authorizations are not pruned from the state. see 8311#issuecomment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. I totally forgot about that issue.

@robert-zaremba
Copy link
Collaborator

So, let's either decide here what shall we do with expired authorization or create a new issue.
CC: @cmwaters

@robert-zaremba
Copy link
Collaborator

One solution I have in mind is following:

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Let's decide if we really want to remove the grant pruning or fix it. I would prefer to fix it.

@robert-zaremba
Copy link
Collaborator

I'm blocking this PR only to clarify what we want to achieve, it's totally fine to schedule a next task and re-enable pruning, or do it here (to be honest, I think we should do it here, because this PR does only a temporal solutions).

@robert-zaremba robert-zaremba changed the title fix(authz)!: unable to execute authorization fix(authz)!: prunning: unable to execute authorization Dec 6, 2021
@aleem1314
Copy link
Contributor Author

@robert-zaremba let's merge this PR. I've raised a follow-up PR(#10714) for pruning expired authorizations.

Copy link
Contributor

@likhita-809 likhita-809 left a comment

Choose a reason for hiding this comment

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

lgtm

@robert-zaremba
Copy link
Collaborator

let's merge this PR. I've raised a follow-up PR(#10714) for pruning expired authorizations.

Why you didn't do that update here? This PR doesn't make much sense without #10714

@aleem1314
Copy link
Contributor Author

Closing in favor of #10714

@aleem1314 aleem1314 closed this Jan 3, 2022
@julienrbrt julienrbrt deleted the aleem/10611-authz-exec branch October 10, 2022 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

authz: unable to execute authorization because it doesn't exist
5 participants