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

Add namespace related options to global settings #5682

Merged
merged 12 commits into from
Feb 18, 2021

Conversation

floreks
Copy link
Member

@floreks floreks commented Nov 12, 2020

The goal of this PR is to give admin better control over default namespace and namespaces presented to users without proper privileges.

I am currently testing out different ways of implementing this feature. I'd like to offer a way to either validate user input in case of a raw input field or offer a Combobox with a list of available namespaces to select from. I am not sure if we can assume that a user with privileges to update global dashboard settings has privileges to list all namespaces.

UI

Zrzut ekranu z 2021-02-18 14-19-24

User without privileges to list namespaces

Zrzut ekranu z 2021-02-18 14-23-17

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 12, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floreks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the language/de Updates or issues for German translations. label Nov 12, 2020
@k8s-ci-robot k8s-ci-robot added language/fr Updates or issues for French translations. approved Indicates a PR has been approved by an approver from all required OWNERS files. language/ja Updates or issues for Japanese translations. language/ko Updates or issues for Korean translations. language/zh Updates or issues for Chinese translations. labels Nov 12, 2020
@codecov
Copy link

codecov bot commented Nov 12, 2020

Codecov Report

Merging #5682 (d3bd377) into master (f86b89e) will increase coverage by 0.02%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master    #5682      +/-   ##
==========================================
+ Coverage   43.82%   43.84%   +0.02%     
==========================================
  Files         215      215              
  Lines        9167     9174       +7     
  Branches      112      113       +1     
==========================================
+ Hits         4017     4022       +5     
- Misses       4884     4886       +2     
  Partials      266      266              

@FrenchBen FrenchBen mentioned this pull request Nov 12, 2020
@FrenchBen
Copy link
Contributor

To provide some insights (point of view), I'll provide a few use cases.

Goal

Upon logging in to the Dashboard, derived from the settings, a User will have access to a pre-define set of NS within the dropdown.

I am a user with Admin role

As an admin, I am setting up a restricted dashboard for my users.

  • I can lookup all Namespaces, thus can validate them, and have a selection (predictive, or dropdown selection)

I am a user, part of a team, with Limited NS access

As a user, I would like to customize the dashboard to focus on the namespaces for my team (settings shared across all user roles)

  • I cannot lookup all Namespaces, but can validate the NS input given (user has access)

I am a user with Limited NS access

As a user, I would like to customize the dashboard to focus on my namespaces (settings for the user only)

  • I cannot lookup all Namespaces, but can validate the NS input given (user has access)

From the above, I can see the following paths:

  • Global Settings
    • Set by Admin
    • Set by user (affects all users)
  • User Settings
    • Set by user (localStorage setting?)

@floreks
Copy link
Member Author

floreks commented Nov 26, 2020

Global settings do not provide a distinction between users. As the name suggests they are global and enforced for everyone. Users with edit privileges are considered to be an admin here.

We do not have user settings right now and local settings are only stored locally, so there is no way to share them between devices.

The first step is to add global settings options for namespace management. As a next step, we can think about local settings that could override global settings.

Small remark regarding the second user role. We always act on behalf of a user, but we can't actually recognize which user is logged in. The logged-in user can have any privileges however from the Dashboard perspective it is still an anonymous user.

We'd like to change that in the new API and be able to recognize logged in user. This way we could store per-user settings.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2020
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 17, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2020
@omesser
Copy link

omesser commented Jan 15, 2021

ping @floreks
Any news / plans to get this in and released ?
A properly namespaced version (which can be limited via chart values/global settings) is something many (well, us at iguazio at least) crave for :)

@floreks
Copy link
Member Author

floreks commented Jan 15, 2021

Ye, I really wanted to finish it this week but other important issues took over and I didn't have time to finish. I'd like to finish ASAP. Most probably next Thursday I'll have time to push this and then push the release.

@omesser
Copy link

omesser commented Jan 15, 2021

@floreks no worries of course, there are other things happening at the world atm :] just checking and wanted to mention there are clients for this change
Thanks for the quick reply!

@floreks floreks force-pushed the feature/namespace branch 2 times, most recently from 3fed4e1 to 3e539ad Compare January 21, 2021 14:41
@floreks
Copy link
Member Author

floreks commented Feb 11, 2021

Mostly finished now. Namespace selector uses values from settings also. Only some minor styling tweaks left to do and it should be good to go. I will finish this next Thursday and push a new release.

@floreks floreks changed the title [WIP] Add namespace related options to global settings Add namespace related options to global settings Feb 18, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2021
@floreks floreks added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 18, 2021
@floreks floreks added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2021
@k8s-ci-robot k8s-ci-robot merged commit e95bcdb into kubernetes:master Feb 18, 2021
@floreks floreks deleted the feature/namespace branch February 18, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/de Updates or issues for German translations. language/fr Updates or issues for French translations. language/ja Updates or issues for Japanese translations. language/ko Updates or issues for Korean translations. language/zh Updates or issues for Chinese translations. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants