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

Add initial vnet-peering proposal #652

Closed
wants to merge 5 commits into from
Closed

Add initial vnet-peering proposal #652

wants to merge 5 commits into from

Conversation

ManuStoessel
Copy link

Proposed Changes

  • add initial proposal for the VNet peering feature

@ManuStoessel ManuStoessel requested a review from a team as a code owner February 24, 2023 15:17
@github-actions github-actions bot added documentation Improvements or additions to documentation size/L labels Feb 24, 2023
@@ -0,0 +1,168 @@
---
title: VNet Peering
Copy link
Contributor

Choose a reason for hiding this comment

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

VNet is implementation detail and should be renamed.


---

# OEP-NNNN: Your short, descriptive title
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove placeholder


## Summary

Network peering is a crucial feature to allow users to route traffic between two onmetal networks. This OEP introduces a new API that allows the owner of both networks to express the consent to peering the networks explicitely and extends the existing `Network` API to reflect an active peering.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no 'owner' in a Kubernetes sense. If a resource representing peering exists, there should not be consent necessary.

Comment on lines 37 to 38
- peering connects whole networks together
(vf get both network route set announced)
Copy link
Contributor

Choose a reason for hiding this comment

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

vf is implementation detail and should be removed.


- peering connects whole networks together
(vf get both network route set announced)
- clear separation from transit gateways
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the separation from transit gateway and in which factors do we differentiate? What are the characteristics we expect that are different?

namespace: customer-andre #optional could be another network of the same namespace
status:
ttl: 2023-02-15T19:00:00Z # omit if empty
state: -> pending because missing match
Copy link
Contributor

Choose a reason for hiding this comment

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


### Example

Let's suppose Manuel wants to peer his `Network` `manuels-network` with André's `Network` `mysupernetwork`. Let's also assume that Manuel's namespace is `customer-manuel` and André's namespace is called `customer-andre`. With that we get the following two `NetworkPeeringRequest`objects reflecting a successful peering of the two `Network`objects presented at the end.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove your own names to keep this generic. Also, remove the usage of the word 'customer' - this is about the API and namespaces, not about customers (that will eventually be there for sure, but customer / IAM can only exist if we respect k8s namespaces and boundaries correctly).

docs/proposals/08-peering.md Show resolved Hide resolved

## Proposal

We propose using a new API called `NetworkPeeringRequests` and extending the `Network` API with a `status` reflecting the peering. For a peering to succeed, both owners of the two `Networks` that should peer, have to create a matching `NetworkPeeringRequest`. On successful peering, the `status` of the corresponding `Network` objects should be updated with the other `Network`s details. A `NetworkPeeringRequest` will also have a set time-to-live (ttl), so the system will be able to automatically clean up vacant or unmatched `NetworkPeeringRequest`objects. Always two matching `NetworkPeeringRequests` are required to enable peering. Important: `localNetworkRef` must match to other `remoteNetworkRef` and vice versa.
Copy link
Contributor

Choose a reason for hiding this comment

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

The type should be called NetworkPeering as it represents the peering. The Request suffix does not make sense here.

namespace: customer-manuel
name: manuels-network
status:
peeredNetworks:
Copy link
Contributor

Choose a reason for hiding this comment

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

One resource should not modify another resource. We should have a separate resource to embody the peering. See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#do-not-modify-the-referred-object

Copy link
Author

Choose a reason for hiding this comment

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

we should probably enhance the network controller to check for matching NetworkPeering resources

@adracus
Copy link
Contributor

adracus commented Apr 5, 2023

Superseded by #676

@adracus adracus closed this Apr 5, 2023
@adracus adracus deleted the vnet-peering branch April 14, 2023 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants