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

Network policy form design 4.9 #545

Merged
merged 4 commits into from
Aug 20, 2021

Conversation

andrew-ronaldson
Copy link
Contributor

Setting Network policies is a critical part of the security of applications on a cluster. In OCP 4.7 the only way to manage Network policy in the console is to edit the yaml of these resources. It's a common and important enough operation that we should make this easier for administrators to work with. The goal of this work is to add the ability to create/update/delete NetworkPolicy resources through the OpenShift console in a way that is easier than directly editing the yaml representation of these resources.

@openshift/team-ux-leads

@openshift/team-ux-review (Administrator perspective)
@openshift/team-devconsole-ux (Developer perspective)
@openshift/team-kni-ux (KNI & Virtualization)

Copy link
Contributor

@bmignano bmignano left a comment

Choose a reason for hiding this comment

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

Looking good, @andrew-ronaldson ! Just left a few questions and comments.

Copy link
Contributor Author

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

Changes made as per feedback

After review with Observability team we decided to remove the dropdown menus that started each "Allowed source" section. More helper text was added to simplify the process.
Copy link
Contributor

@bmignano bmignano left a comment

Choose a reason for hiding this comment

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

Great work on this design! LGTM!

Copy link
Contributor

@matthewcarleton matthewcarleton left a comment

Choose a reason for hiding this comment

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

I had a few small questions - great work on a complex problem!

Copy link
Contributor Author

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback @matthewcarleton

Fixed inconsistency in working of delete actions and changes to Ports fields
Copy link
Contributor

@itsptk itsptk left a comment

Choose a reason for hiding this comment

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

Very nice design! I know a lot of time and effort went into it and it looks great. 👍 Added a few random thoughts.


![Adding allowed sources from peers dropdown](img/5-peers-dropdown.png "Adding allowed sources from peers dropdown menu")

The ingress/egress rules are blocked into three categories of allowed peers:
Copy link
Contributor

Choose a reason for hiding this comment

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

It obviously seems like a deliberate decision but I was curious about the reason for using the term "peers" through the UI instead of the resource, like "pods?"


The screenshot below shows all ingress and egress options in one view.

![Full field list for ingress/egress](img/0-network-policy-full-view.png "Full field list for ingress/egress")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Based on above images I think the link buttons would be "Add pod selector"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating screenshots with a few other changes so I'll include the pod selector naming as well. thanks!

Copy link
Contributor

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

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

LGTM

@beanh66 beanh66 merged commit db9b966 into openshift:master Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants