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

Console: expose network features #875

Merged
merged 3 commits into from
Sep 29, 2021
Merged

Console: expose network features #875

merged 3 commits into from
Sep 29, 2021

Conversation

mariomac
Copy link

The Console may need to show some fields that are exclusive to a given CNI type, and hide the fields that are not
provided by it. Then the console needs visibility about which CNI (OpenShiftSDN or OVNKubernetes) features
are available, depending on each customer's configuration.

This information should be available to all users (or, at this moment, to any user able to create
a Network Policy).

Operator (CNO). For example, the CNO would write a Config Map with a list of features, e.g.:

```properties
network_policy_egress=true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the full list of features you want to be exposed per CNI currently?

Copy link
Author

Choose a reason for hiding this comment

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

yes, at the moment.


### Risks and Mitigations

To be evaluated.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any risk.

Copy link
Contributor

@jotak jotak left a comment

Choose a reason for hiding this comment

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

+1

@jotak
Copy link
Contributor

jotak commented Sep 13, 2021

/assign @LalatenduMohanty

@jotak
Copy link
Contributor

jotak commented Sep 21, 2021

Hi @LalatenduMohanty is there anything blocking the approval?

Copy link
Contributor

@martinkennelly martinkennelly left a comment

Choose a reason for hiding this comment

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

+1 from me.


[In the new network policy creation forms, some fields may depend on the type
of cluster network type](https://issues.redhat.com/browse/NETOBSERV-16).
For example, `OpenShiftSDN` neither supports egress
Copy link
Contributor

Choose a reason for hiding this comment

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

** nor ingress ipblock exceptions

## Proposal

The Console UI should be able to read the set of features that the SDN component supports. These
features are accessed via a ConfigMap (read-only permissions) that is written by the Cloud Network
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Cluster Network Operator


* If `features[F] === true`, then CNI type supports the feature.
* If `features[F] === false`, then the CNI type does not support the feature.
* If `features[F] === undefined`, then the feature' provider does not know
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't happen as we're not really asking the CNI to provide this information, the CNO (if it's deploying one of these CNIs should always know what features they support)

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless it's a 3rd party plugin, maybe we don't care as much about those..... i.e https://access.redhat.com/articles/5436171

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking on version skew between the CNO and the Console. E.g. a new version of the console that asks for a new network feature but the old version of the CNO does not return anything about its support status.

@astoycos
Copy link
Contributor

This lgtm for an initial merge, barring the small nit's I had (we can continue to work out the implementation details moving forward)

@astoycos
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2021
@LalatenduMohanty
Copy link
Member

/unassign @LalatenduMohanty

@jotak
Copy link
Contributor

jotak commented Sep 27, 2021

/assign @knobunc

@knobunc
Copy link
Contributor

knobunc commented Sep 29, 2021

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 29, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak, knobunc, martinkennelly

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2021
@openshift-merge-robot openshift-merge-robot merged commit c626516 into openshift:master Sep 29, 2021
@mariomac mariomac deleted the expose-cni-type branch September 29, 2021 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants