Network policy forms: hide unavailable features for admin users #9628

mariomac · 2021-07-26T13:24:16Z

Related JIRA issue: https://issues.redhat.com/browse/NETOBSERV-27

For a user with sufficient permissions (e.g. admin) using ovn-k8s network type,
all network policy features are available and no special information is
displayed about it.

For a user with sufficient permissions (e.g. admin) using openshift-sdn network
type, egress section and IP block exceptions are hidden, and no special
information is displayed about it.

For a user with insufficient permissions to get the network type, all network
policy features are visible but an informative message is displayed about
potential unavailable features.

jotak · 2021-07-26T14:25:00Z

frontend/public/models/index.ts

+  label: 'Network',
+  labelPlural: 'Networks',
+  apiVersion: 'v1',
+  apiGroup: 'operator.openshift.io',


If I remember correctly there's also Network in "config.openshift.io" api group that can be used (it also contains CNI type info), do you know if one is more relevant than another?

I found multiple ways to fetch/guess the network type, but I used this because it's the only method I saw properly documented: https://docs.openshift.com/container-platform/4.7/networking/cluster-network-operator.html#nw-operator-cr_cluster-network-operator

So I decided to use the above documentation as a source of truth. But I might have missed the config.openshift.io api group. Does it provide any benefit with respect to operatior.openshift.io?

It's documented as well: https://docs.openshift.com/container-platform/4.7/rest_api/config_apis/network-config-openshift-io-v1.html
We might need to ask the SDN team to know more about the difference

jotak · 2021-07-27T10:13:58Z

frontend/public/module/k8s/network.ts

+ */
+export const useClusterNetworkFeatures = (): [ClusterNetworkFeatures, boolean] => {
+  const [features, setFeatures] = React.useState<ClusterNetworkFeatures>();
+  const [featuresLoaded, setFeaturesLoaded] = React.useState(false);


I wonder if we want to distinguish the "not yet loaded" state versus "unknown" state, or we could say it's the same thing. We could argue that features are unknown until they are loaded, and hence remove this featuresLoaded state. What do you think?

I'm afraid that sometimes this could cause some kind of annoying warning message that is initially shown and immediately hidden after the page is loaded.

The other way, the unsupported fields are initially hidden and immediately shown after the page is loaded. This would give a more "natural" feeling of a page being progressively loaded.

What do you think?

Yeah, I've tried locally, it generates some blinking effect that isn't very nice indeed. Ok to keep as you did

jotak · 2021-07-27T10:21:42Z

frontend/public/module/k8s/network.ts

+  const [network, networkLoaded] = useK8sGet<K8sResourceKind>(clusterNetworkModel, networkName);
+  React.useEffect(() => {
+    const cniType = ClusterNetworkType[network?.spec?.defaultNetwork?.type];
+    setFeatures(featuresDocument[cniType ? cniType : ClusterNetworkType.Unknown]);


Nitpicking: if I'm correct, cniType ? cniType : ClusterNetworkType.Unknown can be written cniType ?? ClusterNetworkType.Unknown as well (nullish coalescing) ( I'm being a smart ass but just learned this this week :-) )

aaah yeah, I actually was looking for something like that but didn't find it. As a non-native TS coder, I was looking for the "elvis operator ?:" but didn't find anything by this term. Thank you!

jotak · 2021-07-30T07:45:57Z

frontend/packages/console-app/src/components/network-policies/network-policy-form.tsx

+              </li>
+            </ul>
+            <p>
+              {t('public~More information:')}&nbsp;


I would add another sentence to help the user understand what he has to do with this message, something like: "Refer to your cluster administrator to know which network provider is used".

jotak

Just a suggestion on the info alert, apart from that LGTM

jotak

lgtm

mariomac · 2021-08-02T08:26:05Z

/assign @rohitkrai03

Related JIRA issue: https://issues.redhat.com/browse/NETOBSERV-27 For a user with sufficient permissions (e.g. admin) using ovn-k network type, all network policy feature are available and no special information is displayed about it. For a user with sufficient permissions (e.g. admin) using openshift-sdn network type, egress section and IP block exceptions are hidden, and no special information is displayed about it. For a user with insufficient permissions to get the network type, all network policy features are visible but an informative message is displayed about potential unavailable features.

The following parts have been hidden for OpenShiftSDN CNI type: - "Exceptions" label in network policy peer IP blocks. - The whole "Egress" section in the new policy form.

Previously, we used operator.openshift.io, but we moved to config because this way we could detect other network types (e.g. Calico).

Kept the alert title as a single-line message, and move the rest of the message into the alert body.

memodi · 2021-08-12T20:31:49Z

/label qe-approved

nathan-weinberg · 2021-08-20T15:33:14Z

/retest

nathan-weinberg · 2021-08-20T19:12:06Z

/lgtm

mariomac · 2021-08-23T08:22:41Z

Pinging @rohitkrai03 for a review, as indicated by the openshift-ci bot.

rohitkrai03 · 2021-08-25T15:45:51Z

Hi @mariomac, sorry I missed this one. I am not that aware about this area of the console. @spadgett can you take a look at this?

spadgett

Looks good overall, thanks

spadgett · 2021-09-09T13:23:30Z

frontend/packages/console-app/src/__tests__/network-policies/network-policy-form.spec.tsx

+import { useK8sGet } from '@console/internal/components/utils/k8s-get-hook';
+import { NetworkPolicyForm } from '../../components/network-policies/network-policy-form';
+
+jest.mock('react-i18next', () => {


We have a global mock for this now that you should be able use: https://github.com/openshift/console/blob/master/frontend/__mocks__/react-i18next.ts

spadgett · 2021-09-09T13:24:13Z

...end/packages/console-app/src/__tests__/network-policies/network-policy-peer-ipblock.spec.tsx

+import { NetworkPolicyPeerIPBlock } from '../../components/network-policies/network-policy-peer-ipblock';
+
+const i18nNS = 'public';
+jest.mock('react-i18next', () => {


Sam comment here. You should be able to use the global mock.

spadgett · 2021-09-09T13:25:27Z

frontend/packages/console-app/src/components/network-policies/create-network-policy.tsx

-  const namespace = getActiveNamespace();
-
+  const namespaceProps = {
+    namespace: getActiveNamespace(),


Prefer useActiveNamespace hook.

spadgett · 2021-09-09T13:27:02Z

frontend/packages/console-app/src/components/network-policies/create-network-policy.tsx

  return (
    <div className="co-m-pane__body co-m-pane__form">
      <h1 className="co-m-pane__heading co-m-pane__heading--baseline">
        <div className="co-m-pane__name">{t('public~Create NetworkPolicy')}</div>
        <div className="co-m-pane__heading-link">
          <Link
-            to={`/k8s/ns/${namespace}/networkpolicies/~new`}
+            to={`/k8s/ns/${namespaceProps.namespace}/networkpolicies/~new`}


I would build the base URL with resourcePathFromModel then append /~new. (Eventually we'll add a cluster path section, so better to use the utility.)

spadgett · 2021-09-09T13:27:54Z

frontend/packages/console-app/src/components/network-policies/create-network-policy.tsx

@@ -8,15 +8,16 @@ import './_create-network-policy.scss';

 export const CreateNetworkPolicy: React.FunctionComponent<{}> = () => {


nit: Other parts of the console use the shorthand React.FC. Not important to change for this PR, but it would be good to stay consistent.

spadgett · 2021-09-09T13:32:35Z

frontend/packages/console-app/src/components/network-policies/network-policy-form.tsx

+        networkFeatures?.PolicyPeerIPBlockExceptions === undefined && (
+          <Alert
+            variant="info"
+            title={<p>{t('public~When using the OpenShift SDN cluster network provider:')}</p>}


I think the alert title gets a heading element, in which case you wouldn't want to use p. Does this change the padding for the layout?

spadgett · 2021-09-09T13:35:03Z

frontend/packages/console-app/src/components/network-policies/network-policy-form.tsx

@@ -332,3 +369,5 @@ export const NetworkPolicyForm: React.FunctionComponent<NetworkPolicyFormProps>
    </Form>
  );
 };
+
+export const NetworkPolicyForm = connectToFlags(FLAGS.OPENSHIFT)(NetworkPolicyFormComponent);


Prefer useFlag

spadgett · 2021-09-09T13:36:25Z

frontend/public/models/index.ts

+  apiVersion: 'v1',
+  apiGroup: 'config.openshift.io',
+  plural: 'networks',
+  abbr: '',


You'll want to define an abbr that is used in the badge to the left of links. Even if you don't have a details page for this resource, the badge can still show up (e.g. in the search page).

…ork-type

frontend/packages/console-app/src/components/network-policies/network-policy-form.tsx

mariomac · 2021-09-10T12:40:17Z

/test e2e-gcp-console

mariomac · 2021-09-15T15:34:01Z

/cc @spadgett

spadgett

/lgtm

👍

openshift-ci · 2021-09-24T18:39:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak, mariomac, nathan-weinberg, 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:

~~frontend/OWNERS~~ [spadgett]

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

nalhadef · 2021-09-27T11:44:13Z

/docs-approved

sferich888 · 2021-09-28T15:17:37Z

/label px-approved

spadgett · 2021-09-28T18:04:15Z

/docs-approved

/label docs-approved

openshift-ci bot requested review from jcaianirh and spadgett July 26, 2021 13:24

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

jotak reviewed Jul 26, 2021

View reviewed changes

jotak reviewed Jul 27, 2021

View reviewed changes

openshift-ci bot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Jul 27, 2021

jotak reviewed Jul 30, 2021

View reviewed changes

jotak approved these changes Jul 30, 2021

View reviewed changes

openshift-ci bot assigned rohitkrai03 Aug 2, 2021

Mario Macias added 7 commits August 6, 2021 15:32

Fixed hiding of some sections

2e8cb2c

The following parts have been hidden for OpenShiftSDN CNI type: - "Exceptions" label in network policy peer IP blocks. - The whole "Egress" section in the new policy form.

Merge OpenShift SDN alerts into one single alert message

a0abdba

Fetch CNI type from config.openshift.io

83cccea

Previously, we used operator.openshift.io, but we moved to config because this way we could detect other network types (e.g. Calico).

Fix NetworkPolicyForm IntrinsicAttributes error

99c3771

Unbolded alert body

b043edb

Kept the alert title as a single-line message, and move the rest of the message into the alert body.

added new informational sentence to the alert

141871c

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 12, 2021

openshift-ci bot assigned nathan-weinberg Aug 20, 2021

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

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2021

Merge branch 'master' into fetch-network-type

3f41f3c

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2021

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2021

Mario Macias added 2 commits August 30, 2021 17:56

fixed build error

09accd8

Fixed localisation in tests

c02d527

spadgett reviewed Sep 9, 2021

View reviewed changes

Mario Macias added 3 commits September 9, 2021 16:09

Merge branch 'master' of github.com:openshift/console into fetch-netw…

1d5aee7

…ork-type

Addressing Sam comments

534a8b4

changed connectToFlags by useFlag

edb6a22

spadgett reviewed Sep 9, 2021

View reviewed changes

frontend/packages/console-app/src/components/network-policies/network-policy-form.tsx Show resolved Hide resolved

fix imports

ec3cf89

openshift-ci bot requested a review from spadgett September 15, 2021 15:34

spadgett approved these changes Sep 24, 2021

View reviewed changes

openshift-ci bot assigned spadgett Sep 24, 2021

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

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

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Sep 28, 2021

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Sep 28, 2021

openshift-merge-robot merged commit cdc08ea into openshift:master Sep 29, 2021

mariomac deleted the fetch-network-type branch September 29, 2021 07:08

spadgett added this to the v4.10 milestone Oct 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network policy forms: hide unavailable features for admin users #9628

Network policy forms: hide unavailable features for admin users #9628

mariomac commented Jul 26, 2021

jotak Jul 26, 2021

mariomac Jul 26, 2021

jotak Jul 26, 2021

jotak Jul 27, 2021

mariomac Jul 27, 2021

jotak Jul 27, 2021

jotak Jul 27, 2021

mariomac Jul 27, 2021

jotak Jul 30, 2021

jotak left a comment

jotak left a comment

mariomac commented Aug 2, 2021

memodi commented Aug 12, 2021

nathan-weinberg commented Aug 20, 2021

nathan-weinberg commented Aug 20, 2021

mariomac commented Aug 23, 2021

rohitkrai03 commented Aug 25, 2021

spadgett left a comment

spadgett Sep 9, 2021

spadgett Sep 9, 2021

spadgett Sep 9, 2021

spadgett Sep 9, 2021

spadgett Sep 9, 2021

spadgett Sep 9, 2021

spadgett Sep 9, 2021

spadgett Sep 9, 2021

mariomac commented Sep 10, 2021

mariomac commented Sep 15, 2021

spadgett left a comment

openshift-ci bot commented Sep 24, 2021

nalhadef commented Sep 27, 2021 •

edited

Loading

sferich888 commented Sep 28, 2021

spadgett commented Sep 28, 2021

		@@ -8,15 +8,16 @@ import './_create-network-policy.scss';

		export const CreateNetworkPolicy: React.FunctionComponent<{}> = () => {

Network policy forms: hide unavailable features for admin users #9628

Network policy forms: hide unavailable features for admin users #9628

Conversation

mariomac commented Jul 26, 2021

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

jotak left a comment

Choose a reason for hiding this comment

jotak left a comment

Choose a reason for hiding this comment

mariomac commented Aug 2, 2021

memodi commented Aug 12, 2021

nathan-weinberg commented Aug 20, 2021

nathan-weinberg commented Aug 20, 2021

mariomac commented Aug 23, 2021

rohitkrai03 commented Aug 25, 2021

spadgett left a 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

Choose a reason for hiding this comment

mariomac commented Sep 10, 2021

mariomac commented Sep 15, 2021

spadgett left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 24, 2021

nalhadef commented Sep 27, 2021 • edited Loading

sferich888 commented Sep 28, 2021

spadgett commented Sep 28, 2021

nalhadef commented Sep 27, 2021 •

edited

Loading