Skip to content

keystone: add v3 limits create operation#2464

Merged
mandre merged 5 commits intogophercloud:masterfrom
OpenSource-THG:keystone-limits-create
Mar 17, 2023
Merged

keystone: add v3 limits create operation#2464
mandre merged 5 commits intogophercloud:masterfrom
OpenSource-THG:keystone-limits-create

Conversation

@emilmaruszczak
Copy link
Copy Markdown
Contributor

@emilmaruszczak emilmaruszczak commented Sep 6, 2022 •
edited by mandre
Loading

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 6, 2022 •
edited
Loading

Coverage Status

Coverage: 79.124% (-1.0%) from 80.086% when pulling a50a6e5 on OpenSource-THG:keystone-limits-create into 9809b6a on gophercloud:master.

@mandre
Copy link
Copy Markdown
Contributor

mandre commented Sep 28, 2022

Thanks. Would love to see acceptance tests here as well.

@emilmaruszczak emilmaruszczak force-pushed the keystone-limits-create branch 2 times, most recently from 2a3be28 to 391f915 Compare October 4, 2022 08:52
@emilmaruszczak emilmaruszczak force-pushed the keystone-limits-create branch from 391f915 to fe20aa5 Compare October 4, 2022 16:02
@emilmaruszczak
Copy link
Copy Markdown
Contributor Author

emilmaruszczak commented Oct 4, 2022 •
edited
Loading

Thanks. Would love to see acceptance tests here as well.

@mandre
Could you help me fix this test? I'm unsure on how to set permission or use different account.

=== RUN   TestCreateLimits
    identity.go:26: Attempting to create project: ACPTTESTKaXHAJrJ
    identity.go:43: Successfully created project ACPTTESTKaXHAJrJ with ID a4e9c74349004110bbd6de52e1891c0e
    identity.go:207: Attempting to create service: ACPTTESTZpfWWHKO
    identity.go:223: Successfully created service 8e7c6eb812b3493f87c23cf4c90c9d63
    convenience.go:35: Failure in limits_test.go, line 75: unexpected error "Request forbidden: [POST http://10.1.1.215/identity/v3/limits], error message: {\"error\":{\"code\":403,\"message\":\"You are not authorized to perform the requested action: identity:create_limits.\",\"title\":\"Forbidden\"}}\n"
--- FAIL: TestCreateLimits (0.11s)

@EmilienM
Copy link
Copy Markdown
Contributor

EmilienM commented Oct 7, 2022

Thanks. Would love to see acceptance tests here as well.

@mandre Could you help me fix this test? I'm unsure on how to set permission or use different account.

=== RUN   TestCreateLimits
    identity.go:26: Attempting to create project: ACPTTESTKaXHAJrJ
    identity.go:43: Successfully created project ACPTTESTKaXHAJrJ with ID a4e9c74349004110bbd6de52e1891c0e
    identity.go:207: Attempting to create service: ACPTTESTZpfWWHKO
    identity.go:223: Successfully created service 8e7c6eb812b3493f87c23cf4c90c9d63
    convenience.go:35: Failure in limits_test.go, line 75: unexpected error "Request forbidden: [POST http://10.1.1.215/identity/v3/limits], error message: {\"error\":{\"code\":403,\"message\":\"You are not authorized to perform the requested action: identity:create_limits.\",\"title\":\"Forbidden\"}}\n"
--- FAIL: TestCreateLimits (0.11s)

check for clients.RequireAdmin(t) in current tests and you'll be admin in your tests :)

@mandre
Copy link
Copy Markdown
Contributor

mandre commented Oct 13, 2022

Thanks. Would love to see acceptance tests here as well.

@mandre Could you help me fix this test? I'm unsure on how to set permission or use different account.

=== RUN   TestCreateLimits
    identity.go:26: Attempting to create project: ACPTTESTKaXHAJrJ
    identity.go:43: Successfully created project ACPTTESTKaXHAJrJ with ID a4e9c74349004110bbd6de52e1891c0e
    identity.go:207: Attempting to create service: ACPTTESTZpfWWHKO
    identity.go:223: Successfully created service 8e7c6eb812b3493f87c23cf4c90c9d63
    convenience.go:35: Failure in limits_test.go, line 75: unexpected error "Request forbidden: [POST http://10.1.1.215/identity/v3/limits], error message: {\"error\":{\"code\":403,\"message\":\"You are not authorized to perform the requested action: identity:create_limits.\",\"title\":\"Forbidden\"}}\n"
--- FAIL: TestCreateLimits (0.11s)

check for clients.RequireAdmin(t) in current tests and you'll be admin in your tests :)

clients.RequireAdmin(t) skips the test unless we're running as admin. There must be something else, possibly in the way we install keystone, that causes it to return a 403 when trying to list limits.

@emilmaruszczak
Copy link
Copy Markdown
Contributor Author

emilmaruszczak commented Dec 21, 2022

FYI: I'm working on finding why are the permissions failing when admin account is used (which theoretically should have all the powers).

@emilmaruszczak
Copy link
Copy Markdown
Contributor Author

emilmaruszczak commented Dec 22, 2022 •
edited
Loading

@mandre @EmilienM I managed to fix the test (partially). There were two issues:

  1. The default policy for identity:create_limits is role:admin and system_scope:all. The issue was that the system_scope was empty and thus enforcer returned 403. Solution is in this commit.
  2. Limit cannot be created if there is no corresponding registered limit. I did a workaround of using a pre-created registered limit from glance (ref). This doesn't work on older versions of Openstack. Solution would be to create registered limit programatically (using API), but that is not supported by gophercloud and I don't intend to work on it right now.

mandre
mandre reviewed Mar 1, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

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

Hey @emilmaruszczak, sorry for the time it took to get back to your PR. Nice find about the missing system scope (on a related note, we've just merged the ability to set the system scope in gophercloud-utils with gophercloud/utils#181 in case you need it).
Do you mind creating an issue for implementing the registered limits API calls, so that we have better context for why we skip tests for some releases?

acceptance/openstack/identity/v3/limits_test.go Outdated
th.AssertNoErr(t, err)

// Find image service (glance on Devstack) which has precreated registered limits.
// TODO: Use registered limits API to create global limit and then overwrite it with limit.
Copy link
Copy Markdown
Contributor

@mandre mandre Mar 1, 2023

Choose a reason for hiding this comment

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

I'm fine skipping these tests for versions of openstack for which our workaround doesn't work. Just make sure that we log an issue for implementing the required registered limits API calls, and we reference it here in a comment.

You can skip the tests with something like:

clients.SkipReleasesBelow(t, "stable/xena")

Copy link
Copy Markdown
Contributor Author

@emilmaruszczak emilmaruszczak Mar 13, 2023

Choose a reason for hiding this comment

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

Did that, mentioned the todo in the existing task.
The utils clientconfig has too much logic, it's impossible to get envs that will set the system scope and domain properly. I'll write an issue on it when I have the time.

@emilmaruszczak emilmaruszczak mentioned this pull request Mar 10, 2023
Closed
@emilmaruszczak emilmaruszczak requested a review from mandre March 13, 2023 10:26
mandre
mandre approved these changes Mar 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

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

Thanks @emilmaruszczak.

@mandre mandre merged commit 51f2469 into gophercloud:master Mar 17, 2023
@mandre mandre added this to the v1.3.0 milestone Mar 17, 2023
@mandre mandre mentioned this pull request May 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mandre mandre mandre approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.3.0

Development

Successfully merging this pull request may close these issues.

4 participants

@emilmaruszczak @coveralls @mandre @EmilienM