server: Return permission error if one occurs for Events endpoint

[nathanstilwell]

Events in DB Console (/#/events) would show an internal server error
to the user if they did not have admin permissions to view events. The
error logged in the server is indeed a permissions error, but this is
confusing to the user. To accurately show a permission error on the
FrontEnd, I added an AND statement to the Events endpoint to only return
a server error if an error exists and is not a permission error.

Release note (ui change): Node events will now display a permission
error rather than an internal server error when user does not have admin
privileges to view events.

[cockroach-teamcity]

This change is 

[dhartunian]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh and @Santamaura)

[koorosh]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nathanstilwell and @Santamaura)

---

pkg/server/admin.go, line 1222 at r2 (raw file):

	// just do it here.
	defer func() {
		if retErr != nil && error.Is(retErh, errRequiresAdmin) {

Just wondering if we need to handle the same permission error with other endpoints?
If yes, then it might be better to move this extra validation into serverError func.

[koorosh]

LGTM, only a question to make sure if we need to make this more generic

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nathanstilwell and @Santamaura)

---

pkg/server/admin.go, line 1222 at r2 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

Just wondering if we need to handle the same permission error with other endpoints?
If yes, then it might be better to move this extra validation into serverError func.

func (s *adminServer) serverError(err error) error {
	log.ErrorfDepth(context.TODO(), 1, "%s", err)
        if error.Is(err, errRequiresAdmin {
                return err
        }
	return errAdminAPIError
}

[nathanstilwell]

---

pkg/server/admin.go, line 1222 at r2 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

func (s *adminServer) serverError(err error) error {
	log.ErrorfDepth(context.TODO(), 1, "%s", err)
        if error.Is(err, errRequiresAdmin {
                return err
        }
	return errAdminAPIError
}

David and I talked about that, but serverError is used in so many places I fear unintended consequences. If we find ourselves doing this in more places that might be something we consider in the future.

[koorosh]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Santamaura)

---

pkg/server/admin.go, line 1222 at r2 (raw file):

Previously, nathanstilwell (Nathan Stilwell) wrote…

David and I talked about that, but serverError is used in so many places I fear unintended consequences. If we find ourselves doing this in more places that might be something we consider in the future.

Got it!

[nathanstilwell]

---

pkg/server/admin.go, line 1222 at r2 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

Got it!

Good call out though. I'm glad we all had the same thought. 😄

[nathanstilwell]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from f085778 to blathers/backport-release-20.2-72416: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 20.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[dhartunian]

blathers backport 21.2