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 2006329: Allow web terminal to be installed in any namespace #10045

Merged
merged 7 commits into from Sep 23, 2021
Merged

Bug 2006329: Allow web terminal to be installed in any namespace #10045

merged 7 commits into from Sep 23, 2021

Conversation

JPinkney
Copy link
Contributor

@JPinkney JPinkney commented Sep 10, 2021

As of OpenShift 4.8 the console hardcodes the namespace where the devworkspacetemplates live as openshift-operators. In order to support fully support OSD (and devsandbox), we need the ability to have the devworkspacetemplates and the web terminal operator to be installed in other namespaces then openshift-operators and have those devworkspacetemplates still be picked up by the console. This PR is in conjuction with another PR on the web terminal operator which makes it so that devworkspacetemplates are installed in the namespace where the operator is installed.

In order for the console to correctly reference the namespace where the devworkspacetemplates live it follows this flow when the terminal button is clicked:

  • call the new backend endpoint that resolves the terminals namespace from the frontend
    • If the endpoint was successfully received then the console will create a devworkspace that references devworkspacestemplates installed in that namespace and start the workspace/exec into the workspace as expected
    • If the endpoint was not successfully received then put the cloud shell UI into an unrecoverable state (since we cannot continue without having the namespace) and don't start/initialize the workspace

Testing instructions:

  1. Deploy the console changes to your cluster following https://github.com/openshift/console#steps with the image set as: quay.io/jpinkney/console:installable-anywhere4
  2. Install web terminal catalogsource with the web terminal changes:
cat <<EOF | oc apply -f -
apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: custom-web-terminal-catalog
  namespace: openshift-marketplace
spec:
  displayName: Web Terminal Operator Catalog
  image: quay.io/jpinkney/web-terminal-operator-index@sha256:70e4f05ef522f1fd197db6a2cc364c36b1facf0af41d0303af6e7fbdc0bff0ba
  publisher: Red Hat
  sourceType: grpc
EOF
  1. Create a subscription and operator group in a non openshift-operators namespace
# Create the test namespace
oc new-project web-terminal-test

# Create the operator group
cat <<EOF | oc apply -f -
apiVersion: operators.coreos.com/v1
kind: OperatorGroup
metadata:
  name: web-terminal-test
  namespace: web-terminal-test
EOF

# Create the subscription
cat <<EOF | oc apply -f -
apiVersion: operators.coreos.com/v1alpha1
kind: Subscription
metadata:
  name: web-terminal
  namespace: web-terminal-test
spec:
  channel: fast
  installPlanApproval: Automatic
  name: web-terminal
  source: custom-web-terminal-catalog
  sourceNamespace: openshift-marketplace
  startingCSV: web-terminal.v1.3.1
EOF
  1. Make sure that the console operator has the correct permissions. It can be done easily by checking out this pr and then using:
watch -n 1 oc apply -f manifests/03-rbac-role-cluster.yaml

To actually verify that everything is working:

  1. Login as normal user or admin and create a web terminal. Verify that everything is working as expected
  2. Create a new subscription in a different namespace. Verify that the console will fail to open the next time with an error saying: found multiple subscriptions for web-terminal when only one should be found
  3. Remove the subscription and verify that opening the terminal will work again

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2006329

…inal is installed

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
…mplates in the namespace where the web terminal operators is installed

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
@JPinkney
Copy link
Contributor Author

cc @amisevsk @sleshchenko

pkg/terminal/operator.go Outdated Show resolved Hide resolved
@@ -25,6 +25,8 @@ const (
ProxyEndpoint = "/api/terminal/proxy/"
// AvailableEndpoint path used to check if functionality is enabled
AvailableEndpoint = "/api/terminal/available/"
// InstalledNamespaceEndpoint path used to get the namespace where the controller is installed
InstalledNamespaceEndpoint = "/api/terminal/installedNamespace"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need two of AvailableEndpoint and InstalledNamespaceEndpoint.
Since backend uses exactly the same logic, maybe client should call something like /api/terminal/available, /api/terminal/operator, /api/terminal/operator-namespace (resolves the operator's namespace or return 404 if it's not installed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm OK with doing either, the only reason why I'm hesitant to make the change is that we would have to modify useCloudShellAvailable.ts and that's called in multiple other non web terminal packages so it would require additional testing/review in those packages

Copy link
Contributor

Choose a reason for hiding this comment

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

then maybe let's move it out of this PR.
Anyway someday we need to rename cloud-shell stuff to terminal, because it's really outdated terms )

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.

Found one nit, otherwise the backend changes look good 👍

pkg/terminal/proxy.go Show resolved Hide resolved
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Copy link
Contributor

@sahil143 sahil143 left a comment

Choose a reason for hiding this comment

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

verified locally by following the steps in the description, works as expected

cc @christianvogt

@JPinkney
Copy link
Contributor Author

/retest

@JPinkney JPinkney changed the title Allow web terminal to be installed in any namespace Bug 2006329: Allow web terminal to be installed in any namespace Sep 22, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2021

@JPinkney: This pull request references Bugzilla bug 2006329, which is invalid:

  • expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2006329: Allow web terminal to be installed in any namespace

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.

@openshift-ci openshift-ci bot added bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Sep 22, 2021
@christianvogt
Copy link
Contributor

/bugzilla refresh

@openshift-ci openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Sep 23, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 23, 2021

@christianvogt: This pull request references Bugzilla bug 2006329, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

/bugzilla refresh

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.

@christianvogt
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 23, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, JPinkney, sahil143

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2021
@openshift-merge-robot openshift-merge-robot merged commit 3aa670f into openshift:master Sep 23, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 23, 2021

@JPinkney: All pull requests linked via external trackers have merged:

Bugzilla bug 2006329 has been moved to the MODIFIED state.

In response to this:

Bug 2006329: Allow web terminal to be installed in any namespace

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. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/backend Related to backend component/core Related to console core functionality 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