NETOBSERV-4 Create NetworkPolicy dialog #8655
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
component/core
openshift-ci-robot
added
the
kind/i18n
|
Hi @jotak. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-robot
added
the
needs-ok-to-test
|
Hey @andrew-ronaldson , @astoycos
Currently it's: I'm suggesting this because, especially the second option "Allow traffic from another namespace in the cluster" doesn't sound good to me, first because it can be from more than just one namespace, and secondly because it's not necessarily other namespaces, it could very well include the present namespace. What do you think? Also, something else: since network policies with openshift-sdn work sometimes differently, what do you think about having a warning when a field might not be supported by some network types? E.g. the whole egress section could have such a warning note: "Egress network policies aren't supported with OpenShiftSDN network type." in the explanation text. Ideally I'd like to detect automatically which network type is in use, but afaik this is a cluster configuration that requires admin role, hence normal users wouldn't have access to it (unless you see another way to get that than fetching the cluster Network config?) |
|
Hey Joel! For starters I think we should change "Add from source'" to "Add NetworkPolicyPeer" Apart from that I agree the way the things are worded now is a bit confusing. Lets take a step back, The options we must be able to express when defining a single network policy peer are A Peer defined with....
based on the design When we add a New "From Source" we just need to narrow down to
So Regarding
"namespace-wide" is a bit confusing to me here
I think its mostly there but i'd rather see -- Allow traffic from/to same namespace
This is a REALLY good point and I definitely think if we don't have a dynamic warning we need to have at least a static warning somewhere on the form.
So its a cluster configuration yes, but it can't be RE-configured after cluster installation, so I don't really see any point in hiding it from Developers... but yeah I'm not sure what the default is there Regardless if we can't fetch the config from the CC, you can always just look to see what namespace is there.... In OCP-SDN's case I think it's just |
I'm fine with renaming this as well (tbh i was thinking about something like "Add allowed source" to make it more visible that it's about allowing, not restricting - although it's also mentioned elsewhere). Anyway we can discuss about all these points in the next meeting.
Well, it doesn't really address my point, because the second option may still include the policy's namespace, so it's not 'different' namespace. I don't think I'm nitpicking, take the example of an application than spans over several namespaces, and you want to restrict the traffic to pods belonging to that application. If I read the options that you're suggesting, my first guess would be that I have to create 2 network policies, one for the same namespace, and another for the other namespaces. But in reality I could perfectly just have 1 network policy, which IMO is more obvious to understand with a wording like "cluster-wide traffic". Said differently, I'm trying to make apparent the different scopes that are proposed, given those scopes are not exclusive, but inclusive. Unless I'm wrong, it looks like that: What about something like:
That's a good trick, thanks! (I just need also to check if the console can get these names despite logged in user not seeing them) |
|
Sorry if I'm not understanding this Joel but if you want to use the Policy namespace would you just add source "Allow traffic from same namespace" then the dropdown option "All pods in the namespace"? That's how I see it but maybe that's just me. I like "Add allowed source" instead of from sources. In the description area below the ingress rule heading we could explain the Peers so it is clearer. I agree the wording needs work and I will work with writers on the team. |
openshift-ci
bot
added
the
needs-rebase
We can discuss this in our meeting if needed. My point is if you want to create a rule that is both for the same namespace and other namespaces (since a rule can target multiple namespaces at once) |
openshift-ci
bot
removed
the
needs-rebase
|
Hey Joel, thanks for the descriptive feedback! And no worries it's good to nitpick here since it's pretty confusing, I see what you mean...
This scope diagram is definitely correct, and well done btw! It's counter intuitive but you're 100% right you could allow/deny traffic from pods in the SAME namespace and DIFFERENT namespaces with a single rule. I like this suggestion
The only thing I would advocate against is using the word Whatever word there would work ^ |
jotak
force-pushed
the
netpolicy-form
branch
2 times, most recently
from
284ea65
to
b1dcc4b
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
jotak
changed the title
|
It's ready for review. (Side note: I also did some tries to have multiple views (form / yaml / split) as discussed with @andrew-ronaldson , enabling to switch between views without loosing context, this is pretty promising IMO, but leaving it aside for the moment so that it can be discussed in a more global context) see preview
branch: https://github.com/jotak/console/tree/netpolicy-form-edit
|
|
/retest |
openshift-ci
bot
added
the
kind/cypress
jotak
force-pushed
the
netpolicy-form
branch
2 times, most recently
from
f2935b6
to
8ac8f28
Compare
jotak
force-pushed
the
netpolicy-form
branch
3 times, most recently
from
aa7dfe1
to
4563c2e
Compare
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
| } | ||
| } | ||
|
|
||
| .co-create-networkpolicy__deny .checkbox { |
There was a problem hiding this comment.
When possible, give the checkbox element itself a classname rather than nesting selectors. We've found this to be more maintainable and in the spirit of BEM. (Here and below.)
There was a problem hiding this comment.
Ok, I'm adding a surrounding div with className
|
|
||
| /* Style override for FormFieldGroupExpandable: bigger font (~h2), move toggle icon a little bit lower */ | ||
| .co-create-networkpolicy__expandable-xl > .pf-c-form__field-group-header .pf-c-form__field-group-header-title-text { | ||
| font-size: var(--pf-global--FontSize--xl); |
There was a problem hiding this comment.
nit: we try to alphabetize rules. (We have a backlog story to enforce this with a linter.)
| @@ -0,0 +1,56 @@ | |||
| .co-create-networkpolicy { | |||
There was a problem hiding this comment.
New code generally should go under the console-app package. We will eventually move everything out of public into packages
| } | ||
|
|
||
| .co-create-networkpolicy__expandable-xl > .pf-c-form__field-group-toggle { | ||
| margin-top: 0.3rem; |
There was a problem hiding this comment.
It seems odd to mix rem and px styles (although I don't know if there is a best practice here).
|
|
||
| .co-create-networkpolicy__rule-header .pf-m-secondary { | ||
| margin: 8px; | ||
| top: -8px; |
There was a problem hiding this comment.
Can you explain why these overrides are needed? We try to use the PF components without overrides as much as possible.
There was a problem hiding this comment.
I can't remember, and it doesn't seem necessary anymore, I'm removing it.
| id={`exception-${idx}`} | ||
| value={exc} | ||
| /> | ||
| <Button |
There was a problem hiding this comment.
This button needs an aria-label for screen readers.
| const helpTextPodSelector = | ||
| direction === 'ingress' | ||
| ? namespaceSelector | ||
| ? t( |
There was a problem hiding this comment.
Avoid nested ternaries, they're hard to follow.
| id={`${key}-port`} | ||
| value={port.port} | ||
| /> | ||
| <Button |
| }; | ||
|
|
||
| return ( | ||
| <Card style={{ marginBottom: 15 }}> |
| id: `peer-header-${idx}`, | ||
| }} | ||
| actions={ | ||
| <Button |
| <Alert | ||
| variant="info" | ||
| title={t( | ||
| 'public~When using the OpenShift SDN cluster network provider, egress network policy is not supported.', | ||
| )} | ||
| actionClose={<AlertActionCloseButton onClose={() => setShowSDNAlert(false)} />} | ||
| /> |
There was a problem hiding this comment.
Seems this should be a warning alert?
In other forms we display <Alert> inline so they aren't floating.
| <Alert | |
| variant="info" | |
| title={t( | |
| 'public~When using the OpenShift SDN cluster network provider, egress network policy is not supported.', | |
| )} | |
| actionClose={<AlertActionCloseButton onClose={() => setShowSDNAlert(false)} />} | |
| /> | |
| <Alert | |
| variant="warning" | |
| isInline | |
| className="co-alert" | |
| title={t( | |
| 'public~When using the OpenShift SDN cluster network provider, egress network policy is not supported.', | |
| )} | |
| actionClose={<AlertActionCloseButton onClose={() => setShowSDNAlert(false)} />} | |
| /> |
There was a problem hiding this comment.
I was using a warning at first but changed to info. The reason is that this banner is always displayed, even when there's no problem because the user uses ovn-k cni, and we don't want to sound too aggressive or worrying while there are chances that there's no issue at all. Also, having it as a floating box allows the user to dismiss it if desired (if it's perceived as annoying).
This situation isn't perfect for sure, but it's temporary, there's a task with the SDN team to expose information about the CNI plugin in use; when it's done, we will be able to remove this message and adapt the display to whatever is relevant.
There was a problem hiding this comment.
BTW I could add something to the message like "If your network provider is OVN-Kubernetes, please ignore this message"
There was a problem hiding this comment.
I agree it should be an info message for the reasons you state.
We could try to get the configuration for users who have authority, but if the SDN team is working to expose it to normal users, I'm fine leaving it as-is.
|
/retest |
|
/ok-to-test |
openshift-ci
bot
added
needs-rebase
New form to create network policies as an alternative to yaml cf https://issues.redhat.com/browse/NETOBSERV-1 UX feedbacks - Restore h2 style for ingress/egress section titles - Full-width dropdowns - Replace openshiftSDN related warnings with pf Alerts (info) - Hide pods selector unless explicitly wanted - Adding items to list puts them in first position - Remove rules numbering - Show description text along with pod selector (hence remove the tooltip) - Show description text along with peers in rules Hide/show selectors mechanism ... allowing to get rid of some intermediate options (subtype dropdown) New component NetworkPolicyConditionalSelector IPBlock: hide exceptions until one is added Also use NetworkPolicyKind in network-policy-model.ts, and have more strict typing Fix code review comments - Move files from public to packages/console-app - Move networkPolicy state to inner level - Remove form pre-validation (validation entirely performed from backend) - Couple of CSS fix (remove unneeded, avoid inline styles...), prefer rem to px - Add keys to model (cf rule react/no-array-index-key) - Add several aria-labels
openshift-ci
bot
removed
the
needs-rebase
|
PR rebased, @spadgett @andrew-ronaldson I removed the namespace field |
| ]; | ||
| return ( | ||
| <div className="form-group co-create-networkpolicy__add-peer"> | ||
| <Dropdown |
There was a problem hiding this comment.
Note: For new dropdowns, you should prefer the PatternFly dropdown. We're trying to move away from the internal one.
OK to leave as-is for this PR, though.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak, 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
New form to create network policies as an alternative to yaml
cf https://issues.redhat.com/browse/NETOBSERV-4
Checklist: