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

Bug 1758628: Console workload show restricted acccess if knative serverless TP1 operator is installed and logged in as non admin #2905

Merged

Conversation

invincibleJai
Copy link
Member

@invincibleJai invincibleJai commented Oct 3, 2019

  • fix console sidebar resources when knative installed
  • fix workloads not shown up as non-admin user if knative is installed

Tracks: https://jira.coreos.com/browse/ODC-1979

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 3, 2019
@openshift-ci-robot openshift-ci-robot added component/knative Related to knative-plugin component/sdk Related to console-plugin-sdk labels Oct 3, 2019
@invincibleJai
Copy link
Member Author

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 4, 2019
return {
revisions,
};
const revsionsRes = revisions.length > 0 ? { revisions } : revisions;
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused. It looks like getRevisions always returns K8sResourceKind[]. So we're returning a KnativeItem is the array is not empty, but an empty array otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

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

@spadgett In case of empty array updated to return undefined as currently, it was confusing. In Overview, we are consuming this via extension utils and first check is if CRDs are present then check is based on key as in knative specific resources "configurations" and in current code issue was even if knative resources were not present but utils returned

{ configurations: [] }

https://github.com/openshift/console/blob/master/frontend/public/components/overview/index.tsx#L832

in case of normal deployments and as in sidebar it identifies configurations and knative operator is installed so treated it as knative resource.

with added check in case of no configurations found will return undefined, so will not add any knative specific resources in overviewitems.

cc @christianvogt

@invincibleJai invincibleJai force-pushed the fix-console-sidebar branch 2 times, most recently from 1f72af6 to c4886d2 Compare October 4, 2019 14:56
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 4, 2019
@gijohn
Copy link
Contributor

gijohn commented Oct 4, 2019

Tested and it works fine.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2019
@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2019
@invincibleJai
Copy link
Member Author

/test e2e-aws-console
/test e2e-aws-console-olm

@invincibleJai
Copy link
Member Author

/test e2e-aws

@@ -111,6 +111,7 @@ export const knativeServingResourcesRevision = (namespace: string): FirehoseReso
kind: referenceForModel(RevisionModel),
namespace,
prop: 'revisions',
optional: true,
Copy link
Member

Choose a reason for hiding this comment

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

Do you no longer see the problem switching projects? #2862 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it happens only in topology if we switch namespace and it's not associated with this change optional:true. I mean even in current master it will be observed in case non-admin user tries to view projects not created by them and on the first load will see "Restricted access" and on change of namespace will show loading if knative serverless is installed and as we are showing knative specific resources in sidebar as well so in workloads user will be shown with "restricted access". As you suggested optional: true is working as expected here i.e user will be shown workloads and deployments in topology.

@invincibleJai
Copy link
Member Author

/test e2e-aws

@openshift-bot
Copy link
Contributor

/retest

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

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2019
@invincibleJai
Copy link
Member Author

/test e2e-aws

@invincibleJai
Copy link
Member Author

/test e2e-aws
/test e2e-aws-console-olm

@christianvogt
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, gijohn, invincibleJai

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-merge-robot openshift-merge-robot merged commit 2163b27 into openshift:master Oct 6, 2019
@invincibleJai invincibleJai changed the title fix console sidebar resources when knative installed Bug 1758628 - Console workload show restricted acccess if knative serverless TP1 operator is installed and logged in as non admin Oct 9, 2019
@invincibleJai
Copy link
Member Author

/bugzilla refresh

@spadgett spadgett changed the title Bug 1758628 - Console workload show restricted acccess if knative serverless TP1 operator is installed and logged in as non admin Bug 1758628: Console workload show restricted acccess if knative serverless TP1 operator is installed and logged in as non admin Oct 9, 2019
@openshift-ci-robot
Copy link
Contributor

@invincibleJai: All pull requests linked via external trackers have merged. Bugzilla bug 1758628 has been moved to the MODIFIED state.

In response to this:

Bug 1758628: Console workload show restricted acccess if knative serverless TP1 operator is installed and logged in as non admin

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.

@spadgett spadgett added this to the v4.3 milestone Oct 9, 2019
@invincibleJai
Copy link
Member Author

/cherrypick release-4.2

@openshift-cherrypick-robot

@invincibleJai: new pull request created: #2942

In response to this:

/cherrypick release-4.2

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.

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/knative Related to knative-plugin component/sdk Related to console-plugin-sdk kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants