search

openshift / openshift-sdn

Apache License 2.0
69 stars 63 forks source link

Support network ID allocation as annotation on namespace resource #303

Closed pravisankar closed 8 years ago

pravisankar commented 8 years ago

Trello card: https://trello.com/c/nULFSjXm/29-13-store-vnid-as-annotation-on-namespace-techdebt-supportability

danwinship commented 8 years ago

It belatedly occurs to me that this PR moves us away from the upstream NetworkPolicy proposal. In particular, the upstream model allows for multiple independently-isolated networks in a single Namespace, which doesn't seem compatible with having a single VNID annotation per Namespace.

If instead we keep NetNamespaces around, we can think of them as being proto-NetworkPolicies: having a NetNamespace object in a namespace is equivalent to having a collection of NetworkPolicy objects in the namespace that describe an OpenShift-3.2-compatible isolation policy. (Roughly: one NetworkPolicy saying that all pods in the namespace accept traffic from all other pods in the namespace, and a second NetworkPolicy listing each namespace that has the same VNID as this one, and saying that all pods in this namespace accept traffic from all of those namespaces.)

We can still use the controller/annotation changes from this PR though: just have the controller create the NetNamespace objects and assign their VNIDs, and have the WantsVNID annotation be used on the NetNamespace rather than the Namespace... (We could even keep the VNID as an annotation on the NetNamespace rather than being a "NetID" field like now, in preparation for the eventual move to NetworkPolicies, where we'd have to use an annotation for the VNID since there isn't a field.)

danwinship commented 8 years ago

@dcbw ^^^

pravisankar commented 8 years ago

On Fri, May 6, 2016 at 9:14 AM, Dan Winship notifications@github.com wrote:

It belatedly occurs to me that this PR moves us away from the upstream NetworkPolicy proposal. In particular, the upstream model allows for multiple independently-isolated networks in a single Namespace, which doesn't seem compatible with having a single VNID annotation per Namespace.

If instead we keep NetNamespaces around, we can think of them as being proto-NetworkPolicies: having a NetNamespace object in a namespace is equivalent to having a collection of NetworkPolicy objects in the namespace that describe an OpenShift-3.2-compatible isolation policy. (Roughly: one NetworkPolicy saying that all pods in the namespace accept traffic from all other pods in the namespace, and a second NetworkPolicy listing each namespace that has the same VNID as this one, and saying that all pods in this namespace accept traffic from all of those namespaces.)

We can still use the controller/annotation changes from this PR though: just have the controller create the NetNamespace objects and assign their VNIDs, and have the WantsVNID annotation be used on the NetNamespace rather than the Namespace... (We could even keep the VNID as an annotation on the NetNamespace rather than being a "NetID" field like now, in preparation for the eventual move to NetworkPolicies, where we'd have to use an annotation for the VNID since there isn't a field.)

We have decided to tackle this in a separate PR

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/openshift/openshift-sdn/pull/303#issuecomment-217487658

pravisankar commented 8 years ago

@openshift/networking Updated, PTAL

danwinship commented 8 years ago

I guess I don't have a strong sense of go code organization practices, but is there any reason for splitting it up so much rather than just have a single plugins/osdn/vnid/ directory containing all of the new code? The vnidallocator is only used by the controller, so why not have it be part of that module? And why is the migration code in a separate module from the rest of the controller when the repair code isn't?

Also, should pkg/netid/ be pkg/vnid/ ?

danwinship commented 8 years ago

It would be nice to get an LGTM on the controller parts from someone who is familiar with the existing controller stuff

pravisankar commented 8 years ago

On Wed, May 11, 2016 at 1:33 PM, Dan Winship notifications@github.com wrote:

I guess I don't have a strong sense of go code organization practices, but is there any reason for splitting it up so much rather than just have a single plugins/osdn/vnid/ directory containing all of the new code? The vnidallocator is only used by the controller, so why not have it be part of that module? And why is the migration code in a separate module from the rest of the controller when the repair code isn't?

Code was organized similar to other existing allocators in k8s and origin. Controller dir contains both controllers (vnid and repair) and migration is not included because it's for netnamespace resource.

Also, should pkg/netid/ be pkg/vnid/ ?

I chose netid (network ID) as the top level dir (since vnid is used for vnidRange interface). So, I kept common stuff in CLI and OVS in pkg/netid and OVS specific stuff under plugins/osdn/netid.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/openshift/openshift-sdn/pull/303#issuecomment-218581506

pravisankar commented 8 years ago

@openshift/networking Updated, PTAL

knobunc commented 8 years ago

LGTM

@liggitt @smarterclayton Have your concerns been addressed by the latest version of the PR?

pravisankar commented 8 years ago

@smarterclayton @liggitt updated, can you please take one more look?

pravisankar commented 8 years ago

Closing this PR in favor of https://github.com/openshift/origin/pull/10365