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

Web: implement requesting for kube namespace selector #47345

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Oct 8, 2024

part of #46742

requires:

this is ready for review, i'm still trying to figure out how to make the updated react select less wonky (it works but wonky)

if you want to test, i have a staging cluster with all the UI changes, let me know and i'll invite

when request.allow.kubernetes_resources requires namespace

  ... rest of role
  allow:
    request:
      kubernetes_resources:
      - kind: namespace   # wildcard '*' enforces the same namespace requirement on the web UI (since web UI only supports namespace selection)
image

when request.kubernetes_resources is a kind that web UI doesn't support, it disables the checkout regardless of other resources (it is enabled once user removes the kubernetes):

  ... rest of role
  allow:
    request:
      kubernetes_resources:
      - kind: pod   # only namespace is supported on the web UI, tsh supports all
image

demo (when no config is specified, allows requesting for a kube_cluster or a kube_clusters namespace):

  ... rest of role
  allow:
    request:
    # no kubernetes_resources defined
Screen.Recording.2024-11-04.at.6.14.54.PM.mov

@github-actions github-actions bot requested review from avatus and rudream October 8, 2024 15:59
Copy link

github-actions bot commented Oct 8, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Copy link

github-actions bot commented Oct 8, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@kimlisa kimlisa added the no-changelog Indicates that a PR does not require a changelog entry label Oct 8, 2024
@kimlisa kimlisa force-pushed the lisa/kube-namespace-3 branch from 768b594 to 3140a5c Compare October 9, 2024 06:13
@avatus
Copy link
Contributor

avatus commented Oct 9, 2024

Haven't looked at all the code yet. In general, its SUPER slick and nice. a couple comments of UX and couple questions.

  1. Anytime i add a resource to the "cart", the entire checkout page disappears while its loading the dryrun and then comes back. it causes the cart to flicker a bit too much. This also happens anytime I change the selected namespaces. I think adding/removing resources can maybe be justified (it'd be better if not tho), but while selecting namespaces is probably too jarring. heres a vid.
dad8591a20128f4aba5409885d630d8a.mp4
  1. if i add a resource, all the suggested reviewers are there, but if i pick a namespace, after the flicker all the suggested reviewers are gone

  2. i like that we load the namespaces adhoc only when we decide to select them the first time, thats great! However, i think the flicker causes whatever loaded data to go away which causes me to have to reload the namespaces again. here is an example flow to test

  • add a kube
  • select the namespace dropdown (this causes the load)
  • select the namespace dropdown again (no load)
  • add a new resource to the cart
  • flicker
  • select original namespace, it has to load again

is this because of the flicker or is this one of the wonky things you're talking about in the react-select?

@kimlisa kimlisa force-pushed the lisa/kube-namespace-3 branch from 3140a5c to a7f49c2 Compare October 17, 2024 20:15
@kimlisa
Copy link
Contributor Author

kimlisa commented Oct 17, 2024

  1. Anytime i add a resource to the "cart", the entire checkout page disappears while its loading the dryrun and then comes back. it causes the cart to flicker a bit too much. This also happens anytime I change the selected namespaces. I think adding/removing resources can maybe be justified (it'd be better if not tho), but while selecting namespaces is probably too jarring. heres a vid.

namespaces are resources and is behaving the same as if you were to add/remove resources to the checklist.
i'm not entirely sure how to handle this other than to re-design how checkout works (like allow user to select any resources including namespaces, then click a "next" button that'll run a single dry run and display other info like reviewers/role etc)

  1. if i add a resource, all the suggested reviewers are there, but if i pick a namespace, after the flicker all the suggested reviewers are gone

good catch, this is currently not supported when kube resources are present, and should be handled in another PR

  1. i like that we load the namespaces adhoc only when we decide to select them the first time, thats great! However, i think the flicker causes whatever loaded data to go away which causes me to have to reload the namespaces again. here is an example flow to test

i might be misunderstanding, but the flicker has always been there? (it's the dry run + get resource role attempt per added resource change)

i think it's fine to reload again, if the user for some reason wants to change the namespace again. before the adhoc method, it was calling this api unnecessarily few times per selector on renders (and the user may not even want to select a namespace)

Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

Just leaving a small comment about excludeKubeClusterWithNamespaces that I had when reviewing #47347.

{hasUnsupportedKubeRequestModes && (
<Alert kind="danger">
You can only request Kubernetes resource kind{' '}
{unsupportedKubeRequestModes} for cluster{' '}
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to get rid of those square brackets around resource kinds? How is the message going to look like if unsupportedKubeRequestModes includes multiple kinds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it'll look like this [secret pod namespace], opting not to change anything, since it follows error messaging in tsh

Copy link
Member

Choose a reason for hiding this comment

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

That's fine for now, but long term I think we should strive to not leak too many CLI-specific conventions leak to GUIs. I had a similar discussion with Grzegorz recently. #47652 (comment)

I'm not sure why tsh shows the resource kinds in square brackets in the first place, but in the UI it looks kind of weird IMHO – we have much more freedom there with regards to styling when it comes showing data like this. TBH I think just a regular string would be fine here, e.g. "You can only request Kubernetes resource kinds pod and secret", where "resource kinds" gets pluralized depending on the count and the array with allowed kinds uses something like Array#to_sentence from Ruby on Rails.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I keep forgetting that there's now an equivalent of Array#to_sentence in JS. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat

const vehicles = ['Motorcycle', 'Bus', 'Car'];

const formatter = new Intl.ListFormat('en', {
  style: 'long',
  type: 'conjunction',
});
console.log(formatter.format(vehicles));
// Expected output: "Motorcycle, Bus, and Car"

Though I'm not sure if this is something we could use without a polyfill. caniuse.com says that it's not available at all in Safari on Macs that are not running Big Sur or better. https://caniuse.com/mdn-javascript_builtins_intl_listformat

Copy link
Contributor Author

@kimlisa kimlisa Oct 24, 2024

Choose a reason for hiding this comment

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

i hear you, i created a function for it here: a751204

result:

image image image

Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

I'm submitting some misc issues I found while reviewing the teleterm PR, but I don't plan on reviewing this PR in its entirety. @rudream could you take a look at it?

@kimlisa kimlisa force-pushed the lisa/kube-namespace-3 branch 2 times, most recently from 638a391 to 1a979b2 Compare October 24, 2024 01:52
@kimlisa kimlisa force-pushed the lisa/kube-namespace-3 branch from 1a979b2 to d10cd23 Compare October 28, 2024 15:34
@kimlisa kimlisa added this pull request to the merge queue Oct 28, 2024
Merged via the queue into master with commit 8044b98 Oct 28, 2024
41 checks passed
@kimlisa kimlisa deleted the lisa/kube-namespace-3 branch October 28, 2024 16:06
@public-teleport-github-review-bot

@kimlisa See the table below for backport results.

Branch Result
branch/v16 Failed
branch/v17 Create PR

github-merge-queue bot pushed a commit that referenced this pull request Nov 5, 2024
…ces during access requests (#48413)

* Web: add support for requesting for kube namespaces (#47345)

* Teleterm: add support for access requesting kube namespaces (#47347)

* WebShared: Update how request checkout handles kube resource related errors (#48168)

* WebShared: Update how request checkout handles kube resource related errors

* Fix bug where after create/cancel, specifiable fields were retained

* Remove single toggler for kube resources

* Address CR

* Update snaps

* Backport fixes

- teleport version v16 and less uses react select version 3
  which required to add manual support for initially fetching
  namespaces on select dropdown
- hover tooltip design diffs
- field select design diffs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v16 backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/md ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants