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 API to create user settings #7095

Merged
merged 2 commits into from Dec 1, 2020

Conversation

jerolimov
Copy link
Member

@jerolimov jerolimov commented Nov 4, 2020

Fixes:
https://issues.redhat.com/browse/ODC-4755

Analysis / Root cause:
Based on the User Settings for OpenShift Console enhancement proposal we want to create a ConfigMap for each individual user in the namespace openshift-console-user-settings

Currently you need create this namespace yourself, see Test Setup below. This namespace will be automatically created by the console-operator, see PR openshift/console-operator#479

Solution Description:
Because the user could not create a ConfigMap here, we create a new ConfigMap, Role and RoleBinding with the service account role over the bridge. This code provides 3 new endpoints to create, get and delete the user settings. (The second and third API calls primary for debugging / development.) The console will fetch the ConfigMap over the known k8sWatch API.

API
GET /api/console/user-settings
Get the current user settings (returns a ConfigMap or 404)

POST /api/console/user-settings
Create a user setting ConfigMap for the current user. It returns the existing ConfigMap if it is already exist. This method does not accept an initial content for the ConfigMap.

DELETE /api/console/user-settings
Delete the user setting ConfigMap for the current user. Returns 204 No Content if the ConfigMap was deleted, also if it was not created yet.

Screen shots / Gifs for design review:
Irrelevant

Unit test coverage report:

➜  console git:([odc-4755](https://issues.redhat.com/browse/odc-4755)) go test -coverprofile=coverage.out github.com/openshift/console/pkg/usersettings  
ok  	github.com/openshift/console/pkg/usersettings	0.006s	coverage: 16.2% of statements

➜  console git:([odc-4755](https://issues.redhat.com/browse/odc-4755)) ✗ go tool cover -func=coverage.out
github.com/openshift/console/pkg/usersettings/handlers.go:37:	HandleUserSettings		0.0%
github.com/openshift/console/pkg/usersettings/handlers.go:86:	getUserSettings			0.0%
github.com/openshift/console/pkg/usersettings/handlers.go:92:	createUserSettings		0.0%
github.com/openshift/console/pkg/usersettings/handlers.go:121:	deleteUserSettings		0.0%
github.com/openshift/console/pkg/usersettings/handlers.go:140:	createServiceAccountClient	0.0%
github.com/openshift/console/pkg/usersettings/handlers.go:149:	createUserProxyClient		0.0%
github.com/openshift/console/pkg/usersettings/handlers.go:158:	getUserSettingMeta		0.0%
github.com/openshift/console/pkg/usersettings/helpers.go:11:	getUserSettingMeta		100.0%
github.com/openshift/console/pkg/usersettings/helpers.go:29:	createRole			100.0%
github.com/openshift/console/pkg/usersettings/helpers.go:61:	createRoleBinding		100.0%
github.com/openshift/console/pkg/usersettings/helpers.go:85:	createConfigMap			100.0%
github.com/openshift/console/pkg/usersettings/types.go:10:	getConfigMapName		100.0%
github.com/openshift/console/pkg/usersettings/types.go:14:	getRoleName			100.0%
github.com/openshift/console/pkg/usersettings/types.go:18:	getRoleBindingName		100.0%
total:								(statements)			16.2%

Test setup:
This PR require some additional RBAC rules.

  1. If PR Add namespace and role resources for user settings console-operator#479 is not part of your cluster you need to create a namespace:

1a Create namespace:

apiVersion: v1
kind: Namespace
metadata:
  name: openshift-console-user-settings
  annotations:
    include.release.openshift.io/self-managed-high-availability: "true"
    openshift.io/node-selector: ""

Until https://github.com/openshift/console-operator/pull/486/files is merged and available on your cluster you need to add this as well:

1b Create Role:

kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: console-user-settings-admin-fixed
  namespace: openshift-console-user-settings
  annotations:
    include.release.openshift.io/self-managed-high-availability: "true"
rules:
- apiGroups:
  - rbac.authorization.k8s.io
  resources:
  - roles
  - rolebindings
  verbs:
  - get
  - list
  - watch
  - create
  - update
  - delete
  - patch
- apiGroups:
  - ""
  resources:
  - configmaps
  verbs:
  - get
  - list
  - watch
  - create
  - update
  - delete
  - patch

1c Create RoleBinding:

kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: console-user-settings-admin-fixed
  namespace: openshift-console-user-settings
  annotations:
    include.release.openshift.io/self-managed-high-availability: "true"
roleRef:
  kind: Role
  name: console-user-settings-admin-fixed
  apiGroup: rbac.authorization.k8s.io
subjects:
  - kind: ServiceAccount
    name: console
    namespace: openshift-console
  1. Because the API calls needs a authentication and CSRF token, you can test the API calls from a logged in browser session with this JavaScript snippet:

In frontend code this is automatically done by coFetch, see co-fetch.js. But to test this without additional frontend code or postman I extracted the code for you:

2a Extract crcf token:

let cookiePrefix = 'csrf-token=';
let getCSRFToken = () =>
  document &&
  document.cookie &&
  document.cookie
    .split(';')
    .map((c) => c.trim())
    .filter((c) => c.startsWith(cookiePrefix))
    .map((c) => c.slice(cookiePrefix.length))
    .pop();
getCSRFToken();

2b Get user-settings:

fetch('/api/console/user-settings', { method: 'GET', headers: { 'X-CSRFToken': getCSRFToken() } }).then(
    (res) => {
        console.log('res', res);
        res.text().then(
            (text) => {
                console.log('text', text);
            },
            (parseErr) => console.warn('parseErr', parseErr)
        );
    },
    (requestErr) => {
        console.warn('requestErr', requestErr);
    }
);

2c Create user-settings:

fetch('/api/console/user-settings', { method: 'POST', headers: { 'X-CSRFToken': getCSRFToken() } }).then(
    (res) => {
        console.log('res', res);
        res.text().then(
            (text) => {
                console.log('text', text);
            },
            (parseErr) => console.warn('parseErr', parseErr)
        );
    },
    (requestErr) => {
        console.warn('requestErr', requestErr);
    }
);

Browser conformance:
Irrelevant

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/backend Related to backend labels Nov 4, 2020
@jerolimov jerolimov force-pushed the odc-4755 branch 6 times, most recently from 5aba1a8 to 3b16941 Compare November 5, 2020 10:05
@jerolimov jerolimov force-pushed the odc-4755 branch 3 times, most recently from 8a50fca to d9675c1 Compare November 12, 2020 08:08
@jerolimov
Copy link
Member Author

jerolimov commented Nov 13, 2020

Note for myself, we can fetch the current user uid with this command:

oc get users.user.openshift.io/~ -o template={{.metadata.uid}}

@jerolimov jerolimov force-pushed the odc-4755 branch 2 times, most recently from 519d7f2 to 3c44a04 Compare November 17, 2020 10:24
@jerolimov jerolimov changed the title [WIP] Add API to create user settings Add API to create user settings Nov 17, 2020
@openshift-ci-robot openshift-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 Nov 17, 2020
@jerolimov jerolimov changed the title Add API to create user settings [WIP] Add API to create user settings Nov 17, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2020
@jerolimov jerolimov force-pushed the odc-4755 branch 9 times, most recently from 93763b1 to 643e34b Compare November 17, 2020 18:14
@jerolimov
Copy link
Member Author

/retest

pkg/usersettings/handlers.go Outdated Show resolved Hide resolved

// Fetch the current user with auth.User token and returning the user-setting ConfigMap.
func (h *UserSettingsHandler) getUserSettings(user *auth.User) (*core.ConfigMap, error) {
context := context.TODO()
Copy link
Member

Choose a reason for hiding this comment

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

lets just move the context initialization to the HandleUserSettings() and pass it as parameter. That way if we decide in the future to replace it by a single context there would be less changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the context, createServiceAccountClient and getResourceData now at the top of HandleUserSettings so that there is just one place for it

// Fetch the current user with auth.User token and returning the user-setting ConfigMap.
func (h *UserSettingsHandler) getUserSettings(user *auth.User) (*core.ConfigMap, error) {
context := context.TODO()
resourceData, err := h.getResourceData(context, user)
Copy link
Member

Choose a reason for hiding this comment

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

Also this call can be moved to the HandleUserSettings() and pas as param to the appropriate UserSettingsHandler method

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought this also, but are unsure how an OPTION request would handle this. Should I add extra if for an OPTION request before that or should we check if this works locally and on a cluster. I will change this and we see what happen :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved context, createServiceAccountClient and getResourceData to HandleUserSettings

},
}

client, err := h.createServiceAccountClient()
Copy link
Member

Choose a reason for hiding this comment

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

Also this call can be moved to the HandleUserSettings() and pas as param to the appropriate UserSettingsHandler method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

pkg/usersettings/handlers.go Outdated Show resolved Hide resolved
ResourceSuffix string
}

func (r *ResourceData) getConfigMapName() string {
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to add unit tests for these methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to move this from "name getters / helpers" to the moment when creating the ResourceNames. See comment above.

But for the moment I added tests for the "name getters / helpers" here as well.

},
}

roleBinding := &rbac.RoleBinding{
Copy link
Member

Choose a reason for hiding this comment

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

Lets create a helper for this, so we can add unit-test.

Copy link
Member Author

@jerolimov jerolimov Nov 25, 2020

Choose a reason for hiding this comment

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

Done, see helpers.go and helpers_test.go

return nil, err
}

role := &rbac.Role{
Copy link
Member

Choose a reason for hiding this comment

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

Lets create a helper for this, so we can add unit-test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, see helpers.go and helpers_test.go

},
}

configMap := &core.ConfigMap{
Copy link
Member

Choose a reason for hiding this comment

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

Lets create a helper for this, so we can add unit-test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, see helpers.go and helpers_test.go

@@ -543,29 +543,28 @@ func main() {
bridge.FlagFatalf("user-auth", "must be one of: oidc, openshift, disabled")
}

var resourceListerToken string
Copy link
Member

Choose a reason for hiding this comment

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

👍

@jerolimov
Copy link
Member Author

@jhadvig All done, I had one small idea for another cleanup. And I can squash all commits leter. Thanks for reviewing this.

Comment on lines 14 to 15
if name == "kube:admin" {
resourceIdentifier = "kubeadmin"
} else if uid != "" {
resourceIdentifier = uid
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure there is no UID for the kubeadmin case:

Suggested change
if name == "kube:admin" {
resourceIdentifier = "kubeadmin"
} else if uid != "" {
resourceIdentifier = uid
if uid != "" {
resourceIdentifier = uid
} else if name == "kube:admin" {
resourceIdentifier = "kubeadmin"

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

pkg/usersettings/handlers.go Outdated Show resolved Hide resolved
pkg/usersettings/handlers.go Outdated Show resolved Hide resolved
pkg/usersettings/handlers.go Outdated Show resolved Hide resolved
}
case http.MethodPost:
configMap, err := h.createUserSettings(context, serviceAccountClient, userSettingMeta)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

The preferred idiomatic way for go, how to structure the response should be:

if err != nil {
        errMsg := fmt.Sprintf("Failed to create user settings: %v", err)
	klog.Errorf(errMsg)
	serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: errMsg})
	return
}
serverutils.SendResponse(w, http.StatusOK, configMap)

so we return immediatelly

Copy link
Member Author

Choose a reason for hiding this comment

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

done

pkg/usersettings/handlers.go Outdated Show resolved Hide resolved
return nil, err
}

userInfo, err := client.Resource(USER_RESOURCE).Get(context, "~", meta.GetOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

yeah, thats probably ok

pkg/usersettings/helpers.go Outdated Show resolved Hide resolved

_, err = client.RbacV1().RoleBindings(namespace).Create(ctx, roleBinding, meta.CreateOptions{})
if err != nil && !apierrors.IsAlreadyExists(err) {
return nil, err
Copy link
Member

Choose a reason for hiding this comment

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

we should call deleteUserSettings() method upon error. So we dont end up in an inconsistent state, where Role and RoleBinding was created but the CM for some reason was not.
Either that or we rely that the request would be repeated until all the resources are created.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed in slack, I added deletedUserSettings before returning the create error.

Copy link
Member

@jhadvig jhadvig left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2020
@jerolimov
Copy link
Member Author

/assign @spadgett

@spadgett
Copy link
Member

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, jhadvig, spadgett

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2020
@openshift-merge-robot
Copy link
Contributor

@jerolimov: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp-console d023732 link /test e2e-gcp-console

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit b54fdd5 into openshift:master Dec 1, 2020
@spadgett spadgett added this to the v4.7 milestone Dec 9, 2020
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. component/backend Related to backend component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants