Feature flag MultiClusterHub and disable Web Terminal #9340

abhinandan13jan · 2021-06-24T14:18:00Z

Addresses

Commit 1 -> https://issues.redhat.com/browse/ODC-5924 // Create feature flag for MultiClusterHub CR with Running status
Commit 2 -> https://issues.redhat.com/browse/ODC-5927 // Disable Web terminal based on MCH feature flag

Description

Assumes structure of SERVER_FLAGS to be SERVER_FLAGS.clusters: string [ ] where presence of more than one entry defines multiCluster to be enabled
Creates a function isMultiClusterEnabled which reads data from SERVER_FLAGS for the presence of multiple clusters and returns accordingly.
Updates Web terminal to read the presence of multiple clusters and render accordindly.

PR Testing

Install Web-terminal operator and check for the Cloudshell masthead icon. /terminal route should be available.
Sometimes it doesn't work in localhost. You may need to manually set useCloudShellAvailable to true for testing purpose.
Manually set window.SERVER_FLAGS.clusters = [ 'clustera', 'clusterb'] . This data must exist before render.
Reload, CloudShell masthead icon should disappear.
/terminal route should be unavailable/redirected.

Screenshot

Test

Added test-suite for utility useMCHDetection in console-app
Updated test-suite for CloudShellTab

Browser Conformance

Chrome

christianvogt · 2021-06-24T14:32:28Z

frontend/packages/console-app/src/utils/useMCHDetection.ts

+  const flags = useSelector((state: RootState) => getFlagsObject(state));
+  const isMCHAvailable = flags[MCH_AVAILABILITY_FLAG];


useFlag instead?

christianvogt · 2021-06-24T14:33:24Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellMastheadButton.tsx

  const { t } = useTranslation();

-  if (!terminalAvailable) {
+  if (!terminalAvailable || (!flagLoading && isMCHAvailable)) {


Suggested change

if (!terminalAvailable || (!flagLoading && isMCHAvailable)) {

if (!terminalAvailable || flagLoading || isMCHAvailable) {

christianvogt · 2021-06-24T14:34:37Z

frontend/packages/console-app/src/components/cloud-shell/CloudShellTab.tsx


-  if (devWorkspaceFlag === false) return <Redirect to="/" />;
+  if (devWorkspaceFlag === false || mchFLag === true) return <Redirect to="/" />;


mch flag could take some time to be available but we shouldn't be showing the console while console in the meantime. We need to wait for the flag to be ready.

christianvogt · 2021-06-24T14:34:59Z

frontend/packages/console-app/src/plugin.tsx

+  {
+    type: 'FeatureFlag/Custom',
+    properties: {
+      detect: detectMCHAvailability,
+    },
+  },


Use dynamic plugin SDK because static plugins are being migrated.

spadgett · 2021-06-24T14:46:04Z

frontend/packages/console-app/src/features.ts

+
+export const detectMCHAvailability: FeatureDetector = async (dispatch) => {
+  try {
+    const multiclusterHubs = await k8sList(MultiClusterHubModel);


We probably want to key on the feature gate we plan to add in https://issues.redhat.com/browse/CONSOLE-2845. We'd only want to disable web terminal if the multicluster is enabled for console, which won't always be the case when ACM is installed.

@christianvogt

spadgett · 2021-06-24T14:47:23Z

frontend/packages/console-app/src/features.ts

+  } catch (err) {
+    err?.response?.status === 404
+      ? dispatch(setFlag(MCH_AVAILABILITY_FLAG, false))
+      : handleError(err, MCH_AVAILABILITY_FLAG, dispatch, detectMCHAvailability);


What is the access control for this resource? What happens for users who don't have authority to list MultiClusterHubs?

Yes, we would run into an undesired state for users who cannot list MCH

christianvogt · 2021-06-24T17:52:29Z

cc @TheRealJon @jhadvig
We need to ensure the correct mechanism is used to raise the flag when multi cluster is enabled.

spadgett · 2021-06-25T17:17:26Z

We need to ensure the correct mechanism is used to raise the flag when multi cluster is enabled.

The console operator will pass the backend a list of clusters, so the backend will know. Easiest might be to add something to window.SERVER_FLAGS.

TheRealJon · 2021-06-29T14:18:44Z

@spadgett Good point about the RBAC issues of using MultiClusterHub as an indication of multicluster capability. That was by my suggestion. I also suggested we look for the TechPreviewNoUpgrade config on the cluster, which will also have the same RBAC problems. I agree that window.SERVER_FLAGS might be the best way to go, in which case this story is probably blocked by https://issues.redhat.com/browse/CONSOLE-2845

abhinandan13jan · 2021-06-29T14:24:36Z

@spadgett @christianvogt @TheRealJon Keeping on hold and will update the implementation post https://issues.redhat.com/browse/CONSOLE-2845

andrewballantyne · 2021-06-29T14:25:48Z

/hold

abhinandan13jan · 2021-07-01T09:47:59Z

Updated to mock implementation using window.SERVER_FLAGS.clusters to unblock the dependant stories. Will update to reading actual data when https://issues.redhat.com/browse/CONSOLE-2845 is merged.

cc: @andrewballantyne @christianvogt @spadgett @TheRealJon

abhinandan13jan · 2021-07-02T08:41:28Z

/retest

christianvogt · 2021-07-05T21:12:26Z

@abhinandan13jan please ensure your description is up to date with the latest implementation. Including the PR testing section.

abhinandan13jan · 2021-07-06T13:32:23Z

@abhinandan13jan please ensure your description is up to date with the latest implementation. Including the PR testing section.

Srry for the delay. Updated

TheRealJon · 2021-07-06T13:37:20Z

frontend/packages/console-app/src/utils/isMultiClusterEnabled.ts

+const isMultiClusterEnabled = (): boolean => {
+  let isAvailable = false;
+  // Mock implementation. needs update
+  const mchFlagData = window.SERVER_FLAGS.clusters;
+  if (mchFlagData) {
+    isAvailable = mchFlagData.length > 1; // to-do: update logic/flag when finalised
+  }
+  return isAvailable;
+};


nit, this could be simplified with optional chaining:

Suggested change

const isMultiClusterEnabled = (): boolean => {

let isAvailable = false;

// Mock implementation. needs update

const mchFlagData = window.SERVER_FLAGS.clusters;

if (mchFlagData) {

isAvailable = mchFlagData.length > 1; // to-do: update logic/flag when finalised

}

return isAvailable;

};

const isMultiClusterEnabled = (): boolean => window.SERVER_FLAGS.clusters?.length > 1;

abhinandan13jan · 2021-07-07T08:57:42Z

/retest

TheRealJon · 2021-07-08T13:36:40Z

/lgtm

christianvogt · 2021-07-08T14:58:41Z

frontend/packages/console-app/src/utils/isMultiClusterEnabled.ts

@@ -0,0 +1,4 @@
+const isMultiClusterEnabled = (): boolean => window.SERVER_FLAGS.clusters?.length > 1;


Is this valid?
What if the hub cluster only has a one managed cluster? Won't this array contain a single item?

Should we be checking for undefined or null instead?

cc @spadgett

The cluster list will always include the hub cluster

Thanks for the clarification.

Ok so if there's one cluster it would mean that we are always targeting the hub cluster in which case the check is correct.

While maybe not correct for the web terminal which can work to access the cluster the console runs on. But that's another discussion.

christianvogt · 2021-07-09T13:56:17Z

/approve

openshift-ci · 2021-07-09T13:56:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, christianvogt, TheRealJon

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:

~~frontend/OWNERS~~ [christianvogt]

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

christianvogt · 2021-07-09T13:57:12Z

@abhinandan13jan @spadgett why the hold? Can this be merged now, it seems like it'll have no impact whether the server flag is present or not.

andrewballantyne · 2021-07-09T17:08:54Z

I think this can merge now. I added the hold because of the note Abhi made but since https://issues.redhat.com/browse/CONSOLE-2845 is done separately and we are not dependent on it... I think we can unhold.

/unhold

openshift-ci bot requested review from dtaylor113 and zherman0 June 24, 2021 14:18

openshift-ci bot added the component/core Related to console core functionality label Jun 24, 2021

feature flag acm

bbdf5a3

abhinandan13jan force-pushed the feature-flag-acm branch from 2fcb400 to aafbffd Compare June 24, 2021 14:19

christianvogt reviewed Jun 24, 2021

View reviewed changes

spadgett reviewed Jun 24, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2021

abhinandan13jan force-pushed the feature-flag-acm branch 2 times, most recently from 284ae5b to 76c3a5f Compare July 1, 2021 09:38

abhinandan13jan force-pushed the feature-flag-acm branch from 76c3a5f to 786eee8 Compare July 2, 2021 15:00

spadgett mentioned this pull request Jul 2, 2021

CONSOLE-2838: masthead changes to better align with ACM #9396

Merged

TheRealJon reviewed Jul 6, 2021

View reviewed changes

disable web terminal when MCH flag is available

94d19e1

abhinandan13jan force-pushed the feature-flag-acm branch from 786eee8 to 94d19e1 Compare July 6, 2021 14:18

openshift-ci bot assigned TheRealJon Jul 8, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2021

christianvogt reviewed Jul 8, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2021

openshift-merge-robot merged commit 6b93a16 into openshift:master Jul 9, 2021

spadgett added this to the v4.9 milestone Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature flag MultiClusterHub and disable Web Terminal #9340

Feature flag MultiClusterHub and disable Web Terminal #9340

abhinandan13jan commented Jun 24, 2021 •

edited

christianvogt Jun 24, 2021

christianvogt Jun 24, 2021

christianvogt Jun 24, 2021

abhinandan13jan Jun 29, 2021

christianvogt Jun 24, 2021

abhinandan13jan Jun 29, 2021

spadgett Jun 24, 2021

spadgett Jun 24, 2021

abhinandan13jan Jun 29, 2021

christianvogt commented Jun 24, 2021

spadgett commented Jun 25, 2021

TheRealJon commented Jun 29, 2021 •

edited

abhinandan13jan commented Jun 29, 2021

andrewballantyne commented Jun 29, 2021

abhinandan13jan commented Jul 1, 2021 •

edited

abhinandan13jan commented Jul 2, 2021

christianvogt commented Jul 5, 2021

abhinandan13jan commented Jul 6, 2021

TheRealJon Jul 6, 2021

abhinandan13jan Jul 6, 2021

abhinandan13jan commented Jul 7, 2021

TheRealJon commented Jul 8, 2021

christianvogt Jul 8, 2021 •

edited

TheRealJon Jul 8, 2021

christianvogt Jul 8, 2021

christianvogt commented Jul 9, 2021

openshift-ci bot commented Jul 9, 2021

christianvogt commented Jul 9, 2021

andrewballantyne commented Jul 9, 2021

		const flags = useSelector((state: RootState) => getFlagsObject(state));
		const isMCHAvailable = flags[MCH_AVAILABILITY_FLAG];

	if (!terminalAvailable \|\| (!flagLoading && isMCHAvailable)) {
	if (!terminalAvailable \|\| flagLoading \|\| isMCHAvailable) {


		if (devWorkspaceFlag === false) return <Redirect to="/" />;
		if (devWorkspaceFlag === false \|\| mchFLag === true) return <Redirect to="/" />;

		@@ -0,0 +1,4 @@
		const isMultiClusterEnabled = (): boolean => window.SERVER_FLAGS.clusters?.length > 1;

Feature flag MultiClusterHub and disable Web Terminal #9340

Feature flag MultiClusterHub and disable Web Terminal #9340

Conversation

abhinandan13jan commented Jun 24, 2021 • edited

Addresses

Description

PR Testing

Screenshot

Test

Browser Conformance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Jun 24, 2021

spadgett commented Jun 25, 2021

TheRealJon commented Jun 29, 2021 • edited

abhinandan13jan commented Jun 29, 2021

andrewballantyne commented Jun 29, 2021

abhinandan13jan commented Jul 1, 2021 • edited

abhinandan13jan commented Jul 2, 2021

christianvogt commented Jul 5, 2021

abhinandan13jan commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinandan13jan commented Jul 7, 2021

TheRealJon commented Jul 8, 2021

christianvogt Jul 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Jul 9, 2021

openshift-ci bot commented Jul 9, 2021

christianvogt commented Jul 9, 2021

andrewballantyne commented Jul 9, 2021

abhinandan13jan commented Jun 24, 2021 •

edited

TheRealJon commented Jun 29, 2021 •

edited

abhinandan13jan commented Jul 1, 2021 •

edited

christianvogt Jul 8, 2021 •

edited