jobs: add VIEWJOB role option #96382
Merged
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
|
This change is |
jayshrivastava
force-pushed
the
viewjob-2
branch
2 times, most recently
from
af11ebd to
7605bfd
Compare
jayshrivastava
commented
Feb 2, 2023
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/jobs/jobsauth/authorization.go line 40 at r1 (raw file):
// ViewAccess. In other words: if a user with a given set of privileges is // authorized to modify a job using ControlAccess, they will be authorized to // view it using ViewAccess.
This is unnecessary because job-type specific checks may not obey this, depending on the implementation. This restriction is also not that important imo.
jayshrivastava
force-pushed
the
viewjob-2
branch
from
7605bfd to
7b43613
Compare
jayshrivastava
changed the title
jayshrivastava
force-pushed
the
viewjob-2
branch
from
7b43613 to
c090ba3
Compare
|
It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
jayshrivastava
force-pushed
the
viewjob-2
branch
from
6d30436 to
773484e
Compare
jayshrivastava
force-pushed
the
viewjob-2
branch
from
773484e to
78fdb40
Compare
jayshrivastava
changed the title
HonoreDB
approved these changes
Feb 14, 2023
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 2 of 2 files at r2.
Reviewable status:
|
bors r=HonoreDB TYFR! |
|
bors r=HonoreDB |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
bors r- |
|
Canceled. |
jayshrivastava
force-pushed
the
viewjob-2
branch
from
78fdb40 to
eca16c9
Compare
This is a preliminary change before adding the `VIEWJOB` role option. The purpose of the `VIEWJOB` role option will be to allow TSEs to view jobs in admin clusters without modifying them. This requires access to view all jobs, including ones owned by admins. Adding `VIEWJOB` will conflict with the present `CONTROLJOB` implementation. It will be stronger than `CONTROLJOB` because it lets one view admin jobs when `CONTROLJOB` doesn't, yet it will be weaker because it only allows viewing jobs and not pause/cancel/resume-ing them. This change is introduced to make `CONTROLJOB` at least as strong as `VIEWJOB`: it now allows for all jobs to be viewed. In other words, `VIEWJOB` lets you do a subset of things `CONTROLJOB` lets you do. Also, the reason that `CONTROLJOB` prevents access to admin-owned jobs is not well defined. This is tracked in cockroachdb#96432. Release note (general change): Previously, users with the `CONTROLJOB` role option could not view owned by admins (ex. via `SHOW JOBS`). Now, they can. Epic: none
jayshrivastava
force-pushed
the
viewjob-2
branch
2 times, most recently
from
e28b98b to
9870321
Compare
Release note (general change): Now, users can be granted the `VIEWJOB` role option to be able to view all jobs (ex. via `SHOW JOBS`). This role can be revoked using `NOVIEWJOB`.
jayshrivastava
force-pushed
the
viewjob-2
branch
from
9870321 to
2473fe1
Compare
|
bors r=HonoreDB |
|
Build succeeded: |
This was referenced Mar 1, 2023
jayshrivastava
added a commit
to jayshrivastava/cockroach
that referenced
this pull request
Mar 1, 2023
This change updates `VIEWJOB` to be a global privilege instead of a role option so that it can be inherited from roles to their members. Release note (general change): Previously, `VIEWJOB` was a role option which could be granted to users. Now, `VIEWJOB` is a global privilege. Granting this privilege to a user or role has the syntax `GRANT SYSTEM VIEWJOB TO user`. Using `VIEWJOB` as a role option is deprecated. Note that the `VIEWJOB` role option was not included in any release so far. It was queued up to be released in 23.1, but was not. This change is also being queued for 23.1, so there should not be any backwards compatibility issues. Informs: cockroachdb#96382 Epic: none
jayshrivastava
added a commit
to jayshrivastava/cockroach
that referenced
this pull request
Mar 9, 2023
This change updates `VIEWJOB` to be a global privilege instead of a role option so that it can be inherited from roles to their members. Previously, `VIEWJOB` was a role option which could be granted to users. Now, `VIEWJOB` is a global privilege. Granting this privilege to a user or role has the syntax `GRANT SYSTEM VIEWJOB TO user`. Using `VIEWJOB` as a role option is deprecated. Note that the `VIEWJOB` role option was not included in any release so far. It was queued up to be released in 23.1, but was not. This change is also being queued for 23.1, so there should not be any backwards compatibility issues. Informs: cockroachdb#96382 Epic: none
jayshrivastava
added a commit
to jayshrivastava/cockroach
that referenced
this pull request
Mar 9, 2023
This change updates `VIEWJOB` to be a global privilege instead of a role option so that it can be inherited from roles to their members. Previously, `VIEWJOB` was a role option which could be granted to users. Now, `VIEWJOB` is a global privilege. Granting this privilege to a user or role has the syntax `GRANT SYSTEM VIEWJOB TO user`. Using `VIEWJOB` as a role option is deprecated. Note that the `VIEWJOB` role option was not included in any release so far. It was queued up to be released in 23.1, but was not. This change is also being queued for 23.1, so there should not be any backwards compatibility issues. Informs: cockroachdb#96382 Epic: none Release note: None
jayshrivastava
added a commit
to jayshrivastava/cockroach
that referenced
this pull request
Mar 9, 2023
This change updates `VIEWJOB` to be a global privilege instead of a role option so that it can be inherited from roles to their members. Previously, `VIEWJOB` was a role option which could be granted to users. Now, `VIEWJOB` is a global privilege. Granting this privilege to a user or role has the syntax `GRANT SYSTEM VIEWJOB TO user`. Using `VIEWJOB` as a role option is deprecated. Note that the `VIEWJOB` role option was not included in any release so far. It was queued up to be released in 23.1, but was not. This change is also being queued for 23.1, so there should not be any backwards compatibility issues. Informs: cockroachdb#96382 Epic: none Release note: None
craig bot
pushed a commit
that referenced
this pull request
Mar 9, 2023
96967: changefeedccl: skip testing queries that are too slow as regular SQL r=[samiskin] a=HonoreDB TestChangefeedRandomExpressions was occasionally timing out when doing the regular SELECT query--it's tricky to get sqlsmith not to generate complex expressions that are likely to not be valid for changefeeds anyway, so this PR just skips predicates that take more than a second to process. Informs #96532. Release note: None 97860: jobs: add VIEWJOB global privilege, remove role option r=jayshrivastava a=jayshrivastava This change updates `VIEWJOB` to be a global privilege instead of a role option so that it can be inherited from roles to their members. Previously, `VIEWJOB` was a role option which could be granted to users. Now, `VIEWJOB` is a global privilege. Granting this privilege to a user or role has the syntax `GRANT SYSTEM VIEWJOB TO user`. Using `VIEWJOB` as a role option is deprecated. Note that the `VIEWJOB` role option was not included in any release so far. It was queued up to be released in 23.1, but was not. This change is also being queued for 23.1, so there should not be any backwards compatibility issues. Informs: #96382 Epic: None Release Note: None 98135: cdc: copy request body when registering schemas r=jayshrivastava a=jayshrivastava cdc: copy request body when registering schemas Previously, when the schema registry encountered an error when registering a schema, it would retry the request. The problem is that upon hitting an error, we clean the body before retrying. Retrying with an empty body results in a obscure error message. With this change, we now retry with the original request body so the original error is sustained. This change also adds the metric `changefeed.schema_registry.retry_count` which is a counter for the number of retries performed by the schema registry. Seeing nonzero values indicates that there is an issue with contacting the schema registry and/or registering schemas. Release note (ops change): A new metric `changefeed.schema_registry.retry_count` is added. This measures the number of request retries performed when sending requests to the schema registry. Observing a nonzero value may indicate improper configuration of the schema registry or changefeed parameters. Epic: None 98212: authors: add Mira Radeva to authors r=miraradeva a=miraradeva Release note: None Epic: None 98249: backupccl: incremental schedules always wait on_previous_running r=benbardin a=adityamaru An incremental backup schedule must always wait if there is a running job that was previously scheduled by this incremental schedule. This is because until the previous incremental backup job completes, all future incremental jobs will attempt to backup data from the same `StartTime` corresponding to the `EndTime` of the last incremental layer. In this case only the first incremental job to complete will succeed, while the remaining jobs will either be rejected or worse corrupt the chain of backups. This change overrides the Wait behaviour for an incremental schedule to always default to `wait` during schedule creation or in an alter statement. Note the user specified value will still be applied to the full backup schedule. Ideally we'd have a way to configure options for both the full and incremental schedule separately, in which case we could reject the `on_previous_running` configuration for incremental schedules. Until then this workaround will have to do and we should call out this known limitation. Fixes: #96110 Release note (enterprise change): backup schedules created or altered to have the option `on_previous_running` will have the full backup schedule created with the user specified option, but will override the incremental backup schedule to always default to `on_previous_running = wait`. This ensures correctness of the backup chains created by the incremental schedule by preventing duplicate incremental jobs from racing against each other. 98307: ui: fix txn aggregations in txns fingerprints page r=xinhaoz a=xinhaoz This commit addresses 2 issues on the txns overview page: 1. We were previously grouping txns by txn fingerprint id, agg time, agg interval, and app name. This is from a time when we wanted all these fields, but recently we only want to aggregate on txn fingerprint id. This commit changes the grouping to only the txn id. 2. Stats aggregation causing undesired data mutations: We were seeing that in the txns fingerprint page, stats columns would seemingly randomly continue to increase while on the page (e.g. exec count, bytes read). During stats aggregation after grouping by the fields mentioned above, we were using the first txn in the grouping as the base object for stats aggregation, meaning we inherited and mutated the stats object of that txn. Since we aggregate on every re-render, This meant that we were using the result of any previous aggregations as the base for our current aggregation in the re-render. This explains the never-ending incrementing stats. This commit addresses this bug by ensuring we don't re-use the stats object between re-renders by creating a new copy of the stats for every aggregation. Fixes: #96186 Fixes: #68375 Release note (bug fix): stats columns in txns fingerprint overview page does not continuously increment BEFORE https://www.loom.com/share/d9bbd98ced2742dd899031fbc16df6af AFTER https://www.loom.com/share/5407fbbad086404c8d9d63e7f5ef15dd 98321: backupccl: add restore/pause/tpce/80GB/aws/nodes=4/cpus=8 to aws nightlies r=lidorcarmel a=msbutler Epic: none Release note: None Co-authored-by: Aaron Zinger <[email protected]> Co-authored-by: Jayant Shrivastava <[email protected]> Co-authored-by: Mira Radeva <[email protected]> Co-authored-by: adityamaru <[email protected]> Co-authored-by: Xin Hao Zhang <[email protected]> Co-authored-by: Michael Butler <[email protected]>
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.
jobs: allow CONTROLJOB users to view all jobs
This is a preliminary change before adding the
VIEWJOBroleoption. The purpose of the
VIEWJOBrole optionwill be to allow TSEs to view jobs in admin clusters without
modifying them. This requires access to view all jobs, including ones
owned by admins.
Adding
VIEWJOBwill conflict with the presentCONTROLJOBimplementation.It will be stronger than
CONTROLJOBbecause it lets one view admin jobs whenCONTROLJOBdoesn't, yet it will be weaker because it only allows viewingjobs and not pause/cancel/resume-ing them.
This change is introduced to make
CONTROLJOBat least as strongas
VIEWJOB: it now allows for all jobs to be viewed. In other words,VIEWJOBlets you do a subset of thingsCONTROLJOBlets you do.Also, the reason that
CONTROLJOBprevents access to admin-ownedjobs is not well defined. This is tracked in #96432.
Release note (general change):
Previously, users with the
CONTROLJOBrole option could not viewowned by admins (ex. via
SHOW JOBS). Now, they can.Epic: none
jobs: add VIEWJOB role option
Release note (general change):
Users can now be granted the
VIEWJOBrole optionto be able to view all jobs (ex. via
SHOW JOBS).This role can be revoked using
NOVIEWJOB.Epic: none