Deployment Status Command Does Not Respect -namespace Wildcard #16792
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
Juanadelacuesta
force-pushed
the
b-gh-13660
branch
3 times, most recently
from
44d2894
to
0c9ac1e
Compare
Juanadelacuesta
changed the title
tgross
approved these changes
Apr 11, 2023
Comment on lines
+470
to
+477
| allowableNamespaces, err := allowedNSes(aclObj, store, allow) | ||
| if err != nil { | ||
| if err == structs.ErrPermissionDenied { | ||
| reply.Deployments = make([]*structs.Deployment, 0) | ||
| return nil | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
Not blocking but something we might want to get agreement on with the team and then document in our RPC checklist...
There's a small inconsistency here between the behavior of the first permissions check (on line 458) and this permissions check in the blocking query:
- If you have no permission to any namespace when you make the query, you get
ErrPermissionDenied - If you have permissions and make a blocking query, and while you're waiting on the blocking query the permissions are removed, you get an empty list.
I spot-checked some of the other List RPCs and it looks like we're inconsistent with this across the code base; some RPCs have both checks (ex. Eval.List, Job.List) while others only have the second check (ex. Namespaces.ListNamespace, Variables.List).
jrasell
approved these changes
Apr 11, 2023
There was a problem hiding this comment.
LGTM, minor inline suggestion which is not blocking. The PR will also need a changelog entry, so everyone can bask in the fix!
Something that isn't needed before merge would be to use must for all the new test additions, rather than a mix of must and assert/require.
nomad/deployment_endpoint.go
Outdated
| var deploys []*structs.Deployment | ||
| paginator, err := paginator.NewPaginator(iter, tokenizer, nil, args.QueryOptions, | ||
| paginator, err := paginator.NewPaginator(iter, tokenizer, filters, args.QueryOptions, |
There was a problem hiding this comment.
While we are in this area, could we change the name of the paginator variable as it clashes with an import.
Juanadelacuesta
force-pushed
the
b-gh-13660
branch
from
ca50773
to
25e4c7c
Compare
Juanadelacuesta
added
backport/1.3.x
Juanadelacuesta
added a commit
that referenced
this pull request
Apr 12, 2023
* func: add namespace support for list deployment * func: add wildcard to namespace filter for deployments * Update deployment_endpoint.go * style: use must instead of require or asseert * style: rename paginator to avoid clash with import * style: add changelog entry * fix: add missing parameter for upsert jobs
Juanadelacuesta
added a commit
that referenced
this pull request
Apr 12, 2023
* func: add namespace support for list deployment * func: add wildcard to namespace filter for deployments * Update deployment_endpoint.go * style: use must instead of require or asseert * style: rename paginator to avoid clash with import * style: add changelog entry * fix: add missing parameter for upsert jobs
Juanadelacuesta
added a commit
that referenced
this pull request
Apr 13, 2023
…ace Wildcard (#16865) * Deployment Status Command Does Not Respect -namespace Wildcard (#16792) * func: add namespace support for list deployment * func: add wildcard to namespace filter for deployments * Update deployment_endpoint.go * style: use must instead of require or asseert * style: rename paginator to avoid clash with import * style: add changelog entry * fix: add missing parameter for upsert jobs * fix: remove extra parameter from upsert job
Juanadelacuesta
added a commit
that referenced
this pull request
Apr 13, 2023
* func: add namespace support for list deployment * func: add wildcard to namespace filter for deployments * Update deployment_endpoint.go * style: use must instead of require or asseert * style: rename paginator to avoid clash with import * style: add changelog entry * fix: add missing parameter for upsert jobs
Juanadelacuesta
added a commit
that referenced
this pull request
Apr 13, 2023
… (#16866) * func: add namespace support for list deployment * func: add wildcard to namespace filter for deployments * Update deployment_endpoint.go * style: use must instead of require or asseert * style: rename paginator to avoid clash with import * style: add changelog entry * fix: add missing parameter for upsert jobs
Juanadelacuesta
added a commit
that referenced
this pull request
Apr 13, 2023
… (#16876) * func: add namespace support for list deployment * func: add wildcard to namespace filter for deployments * Update deployment_endpoint.go * style: use must instead of require or asseert * style: rename paginator to avoid clash with import * style: add changelog entry * fix: add missing parameter for upsert jobs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR addresses the bug reported here
The deployments Namespace filter did not take into account the namespace wildcard, resulting on an empty response every time it was used in conjunction with a prefix. This PR adds the missing wildcard check as well as filtering by the allowed namespaces according to the ACL token.