NETOBSERV-15: list pods affected by a network policy #9953

mariomac · 2021-08-31T16:16:57Z

For a better/faster identification of pods impacted by a policy, show pods
targeted by the combined namespace and pod selectors in peer definition and/or
rule definition and/or whole ingress/egress section.

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

Demo (updated after @andrew-ronaldson redesign request):

demo.mp4

mariomac · 2021-09-01T08:57:02Z

/retest

jotak · 2021-09-02T16:07:57Z

/cc

For a better/faster identification of pods impacted by a policy, show pods targeted by the combined namespace and pod selectors in peer definition and/or rule definition and/or whole ingress/egress section. JIRA issue: https://issues.redhat.com/browse/NETOBSERV-15

jotak · 2021-09-13T09:43:55Z

There's an error breaking the UI when an invalid label is entered and the pods are watched. For example, in pod selector enter label key: "app" and value "test-"

Value ending with a dash is not accepted.

We should show an empty list on error, with perhaps (even better) an error icon somewhere ?

More info: https://issues.redhat.com/browse/NETOBSERV-15?focusedCommentId=18972231#comment-18972231

mariomac · 2021-09-16T13:48:16Z

frontend/package.json

    "@patternfly/react-catalog-view-extension": "4.12.38",
    "@patternfly/react-charts": "6.15.14",
-    "@patternfly/react-core": "4.147.2",
+    "@patternfly/react-core": "4.152.4",


required update, as the TreeView component did not provide the hasGuides property in the previous version (even if it's part of the Patternfly specification)

mariomac · 2021-09-20T07:16:51Z

/retest

jotak · 2021-09-27T08:50:07Z

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

+          <Button ref={podsPreviewPopoverRef} variant="link" isInline>
+            {t('public~affected pods')}
+          </Button>{' '}
+          {t('that this policy will apply to')}


Text namespace is missing here, no? t('console app~blah blah')

Also, to mix an element within translated text, afaik we should use the Trans component. Should be something like that:

<Trans ns="public"> Show a preview of the{' '} <Button ref={podsPreviewPopoverRef} variant="link" isInline> affected pods </Button>{' '} that this policy will apply to </Trans>

Though not sure if affected pods can be left as is or has to be explicitly translated as {t('public~affected pods')}

jotak · 2021-09-27T09:31:17Z

frontend/packages/console-app/src/components/network-policies/_create-network-policy.scss

  margin-right: 0;
 }
+
+.pf-c-tree-view__list-item {


The TreeView component should get a className, for instance <TreeView className="co-create-networkpolicy__selector-preview" ... so that this style is scoped and doesn't risk to have side effects in other parts; (hence this rule would be rewritten .co-create-networkpolicy__selector-preview .pf-c-tree-view__list-item)

(or alternatively, use a surrounding class related to networkpolicy to ensure a narrow scope is used)

In this case, I decided to use only a class selector, as I consider that showing the bullets at the left margin is a bug that should not be visible on any Tree View. E.g. without this entry:

Anyway this issue has been already fixed in PatternFly so I'll remove this code.

jotak · 2021-09-27T09:35:14Z

frontend/packages/console-app/src/components/network-policies/_create-network-policy.scss

+
+.pf-c-tree-view__list-item {
+  list-style: none;
+}


I did some tests to fix the pointer issue (so that the cursor doesn't suggest clicking on a pod trigger any effect), this CSS would do the trick:

.co-create-networkpolicy__selector-preview button.pf-c-tree-view__node[role="treeitem"], .co-create-networkpolicy__selector-preview button.pf-c-tree-view__node[role="treeitem"] > .pf-c-tree-view__node-container { cursor: default; }

Alternatively, patternfly provides a variant named "compactNoBackground" in TreeView that also removes the pointer cursor, but weirdly this variant is less compact than the normal one and IMO doesn't look good in our use case, so I'd stick with a CSS override.

jotak · 2021-09-27T13:11:58Z

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

        />
      </div>
+      <p>
+        {t('public~Show a preview of the')}{' '}


Same as above, perhaps a Trans component makes sense here

jotak · 2021-09-27T13:19:04Z

...end/packages/console-app/src/components/network-policies/network-policy-selector-preview.tsx

+          {preview.total && preview.total > maxPreviewPods && (
+            <>
+              {_.size(safeNsSelector.matchLabels) === 0 ? (
+                <a href={`${resourceListPathFromModel(PodModel, namespace)}${podsFilterQuery}`}>


About this "View all N results" link: I wonder if this link should always open in a new window, wdyt? (@mariomac and @andrew-ronaldson ) I don't imagine anyone while creating a new network policy would like to quit that page to view these search results...
Or alternatively, when asked to view more results, we show all the results within that popup (which then would be scrollable).

Do you mean the size of the modal stays the same but it is scrollable to show the list? I have a few other questions if that's alright:

What is the benefit the user gets from having this feature in the form?

Is there value in seeing the total count of pods or just list them out?

Could we in future show the status of the pods? (Example: Failed to connect)

Another compromise option could be that, instead of opening the table in a new window, to visualize an embedded table below, like in the original commit. Maybe limited in height and scrollable.

To clarify: first option is to show the popup tree, as now. If the customer clicks into the "View all" link, instead opening a new window, it closes the popup and attaches the preview table below the label selector.

This would also fix an "issue" I noticed with the popup: if I want to update the label selector, the popup is closed while I change the labels, and then I have to open again the popup. I felt more confortable getting an updated view of the affected pods as long as I typed.

Do you mean the size of the modal stays the same but it is scrollable to show the list? I have a few other questions if that's alright:

1. What is the benefit the user gets from having this feature in the form?

IMO, to avoid having to go back and forth between separate views when setting up the labels, as the user tries to make sure the selector targets strictly the desired pods, no more, no less.
If we think showing all pods is to risky (could slow down the page too much), we can keep a limit, but should be much more than 5 IMO
The solution proposed by @mariomac would also work.

2. Is there value in seeing the total count of pods or just list them out?

I would say yes, especially if not everything is displayed. But it still has less value that showing them out as the user would still have to list them to make sure everything is correct.

3. Could we in future show the status of the pods? (Example: Failed to connect)

Not in the near future, but yes maybe; there's a RFE for checking pods connectivity (RFE-1874)

To clarify: first option is to show the popup tree, as now. If the customer clicks into the "View all" link, instead opening a new window, it closes the popup and attaches the preview table below the label selector.

This would also fix an "issue" I noticed with the popup: if I want to update the label selector, the popup is closed while I change the labels, and then I have to open again the popup. I felt more confortable getting an updated view of the affected pods as long as I typed.

+1 I also like when it's updated as you type

As agreed offline, I have submitted a new PR with the following changes:

"View all xx results" link now opens in a new tab

Increased pod limit to 10

…w-pods

mariomac · 2021-10-07T11:09:04Z

Added the following data-test labels:

pods-preview-title: the list of pods matching ... message before the pods tree, or the No pods matching the provided labels message when there aren't matches.
pods-preview-footer: the showing 10 from XX results footer message shown after the pods tree when there is a namespace selector.
pods-preview-tree: the TreeView containing the pods preview inside the Popover
pods-preview-footer-link: the View all XX results link shown after the pods tree when it fetches data from all namespaces or from the current namespace.
pods-preview-alert: the Alert message shown when there is an input error or a K8s API unexpected error.
show-affected-pods: the Button to pop-up the preview of the pods that are affected by the Network Policy.
policy-pods-preview-popover: the Popover instance that contains the preview of the pods that are affected by the Network Policy.
show-affected-pods-ingress: the Button to pop-up the previews of the pods that are affected by an ingress rule.
pods-preview-ingress-popover: the Popover instance that contains the preview of the pods that are affected by an ingress rule.
show-affected-pods-egress and pods-preview-ingress-egress are analogous to the two previous items, but for egress rules.

memodi · 2021-10-12T15:01:24Z

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

          Show a preview of the{' '}
-          <Button ref={podsPreviewPopoverRef} variant="link" isInline>
+          <Button
+            data-test={`show-affected-pods-${props.direction}}`}


there seems to be extra closing brace bracket here }

Good catch! thank you

…w-pods

jotak · 2021-10-18T15:02:36Z

frontend/packages/console-app/locales/en/console-app.json

+  "ingress rule": "ingress rule",
+  "egress rule": "egress rule",
  "If no namespace selector is provided, pods from all namespaces will be eligible.": "If no namespace selector is provided, pods from all namespaces will be eligible.",
+  "netpolicy_peers_preview_affected_pods": "Show a preview of the <2>affected pods</2> that this <5></5> will apply to",


is that expected? First time I'm seeing this in the locale file

I mean, the key != the value ; running yarn i18n won't break that?

yes, it won't break. The key is automatically fetched from the i18nKey property of the Trans element. Before this, I was having errors because the Trans element had too much conditional logic inside and the key became malformed. Maybe now we can remove it as I simplified the Trans element.

Maybe we haven't seen this more often because there are no more Trans instances in the console.

memodi · 2021-10-18T15:35:13Z

/label qe-approved

jotak · 2021-10-18T16:36:26Z

/lgtm

mariomac · 2021-10-19T06:54:32Z

/assign @invincibleJai

invincibleJai · 2021-10-19T15:45:48Z

/unassign
/assign @spadgett

spadgett · 2021-11-08T17:55:11Z

/approve

openshift-ci · 2021-11-08T17:55:47Z

[APPROVALNOTIFIER] This PR is APPROVED

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

sferich888 · 2021-11-09T13:11:07Z

/label px-approved

mariomac · 2021-11-09T14:32:49Z

/retest ci/prow/e2e-gcp

openshift-ci · 2021-11-09T14:32:50Z

@mariomac: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test analyze
/test backend
/test ceph-storage-plugin
/test e2e-gcp
/test e2e-gcp-console
/test frontend
/test images
/test kubevirt-plugin

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-console-master-analyze
pull-ci-openshift-console-master-backend
pull-ci-openshift-console-master-e2e-gcp
pull-ci-openshift-console-master-e2e-gcp-console
pull-ci-openshift-console-master-frontend
pull-ci-openshift-console-master-images

In response to this:

/retest ci/prow/e2e-gcp

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.

mariomac · 2021-11-09T14:33:08Z

/test e2e-gcp

openshift-ci bot requested review from rhamilto and TheRealJon August 31, 2021 16:17

openshift-ci bot added component/core Related to console core functionality kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Aug 31, 2021

openshift-ci bot requested a review from jotak September 2, 2021 16:07

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

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

Addressed Andrew Ronaldson's redesign request

64928e3

More info: https://issues.redhat.com/browse/NETOBSERV-15?focusedCommentId=18972231#comment-18972231

mariomac commented Sep 16, 2021

View reviewed changes

cleaned test and updated i18n files

8f5e770

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

Merge branch 'master' into netpol-show-pods

b335e82

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

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

Merge branch 'master' into netpol-show-pods

0acb1bc

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

jotak reviewed Sep 27, 2021

View reviewed changes

Mario Macias added 4 commits September 29, 2021 15:40

Merge branch 'master' of github.com:openshift/console into netpol-sho…

0470177

…w-pods

Merge branch 'master' of github.com:openshift/console into netpol-sho…

d511a0a

…w-pods

Fixed compiler errors

00d0a4f

fix merge errors

a563128

added data-test labels

33c6943

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

memodi reviewed Oct 12, 2021

View reviewed changes

Mario Macias added 5 commits October 13, 2021 10:17

Merge branch 'master' of github.com:openshift/console into netpol-sho…

be15847

…w-pods

Fixed extra } in pod preview label

7f11f81

Fix i18n missing key

f6413eb

Merge branch 'master' of github.com:openshift/console into netpol-sho…

b21a0d7

…w-pods

Avoid issues with Trans and string interpolation

693932f

jotak reviewed Oct 18, 2021

View reviewed changes

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

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

openshift-ci bot assigned invincibleJai Oct 19, 2021

openshift-ci bot assigned spadgett and unassigned invincibleJai Oct 19, 2021

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

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

openshift-merge-robot merged commit b16cd57 into openshift:master Nov 9, 2021

mariomac deleted the netpol-show-pods branch December 2, 2021 15:32

spadgett added this to the v4.10 milestone Dec 8, 2021

NETOBSERV-15: list pods affected by a network policy #9953

NETOBSERV-15: list pods affected by a network policy #9953

Uh oh!

Conversation

mariomac commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariomac commented Sep 1, 2021

Uh oh!

jotak commented Sep 2, 2021

Uh oh!

jotak commented Sep 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariomac commented Sep 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jotak Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariomac Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariomac commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

memodi commented Oct 18, 2021

Uh oh!

jotak commented Oct 18, 2021

Uh oh!

mariomac commented Oct 19, 2021

Uh oh!

invincibleJai commented Oct 19, 2021

Uh oh!

spadgett commented Nov 8, 2021

Uh oh!

openshift-ci bot commented Nov 8, 2021

Uh oh!

sferich888 commented Nov 9, 2021

Uh oh!

mariomac commented Nov 9, 2021

Uh oh!

mariomac commented Aug 31, 2021 •

edited

Loading

jotak Sep 27, 2021 •

edited

Loading

mariomac Oct 1, 2021 •

edited

Loading

mariomac commented Oct 7, 2021 •

edited

Loading