fabric-testbed / InformationModel

FABRIC Information Model library
MIT License
7 stars 1 forks source link

Oess extension #146

Closed zlion closed 1 year ago

zlion commented 1 year ago

Please review the changes to FIM that support the OESS network service.

ibaldin commented 1 year ago

@zlion I have a couple of questions:

  1. for the cloud_account_id under Labels - what is the expected regex - I prefer to set validators for all fields in labels to avoid any surprises
  2. for the local_asn under network service sliver - there already is an asn field under labels. Is this because two ASNs need to be specified? If so, we could add that to labels, also all labels take lists in addition to single values to two ASNs can be specified like this:
    l = Labels(asn=["12345", "54321"]) 
zlion commented 1 year ago

According to OESS document, the "could_account_id" regex depends on cloud provider.

I read through related documents and pull out more details as shown below.

provider format example reference Azure UUID "3e2480b2-b4d5-424b-976a-7b0de65a1b62" GCP <random>/<vlan-attachment-region>/<edge-availability-domain> "7e51371e-72a3-40b5-b844-2e3efefaee59/us-central1/2" link AWS 12 digits Oracle ocid1.<RESOURCE TYPE>.<REALM>.[REGION][.FUTURE USE].<UNIQUE ID> "ocid1.instance.oc1.phx.abuw4ljrlsfiqw6vzzxb43vyypt4pkodawglp3wqxjqofakrwvou52gb6s5a" link

xi-yang commented 1 year ago

@ibaldin Regarding account ID, it depends, AWS has 12 digit account ID. GCP uses service account name plus project name that are strings. Then Azure uses UUID for the so called subscription ID.

We can either add different types of labels for these or just make it a string with a relaxed length limit.

zlion commented 1 year ago

@ibaldin As for the data field "asn", I agree that it is better to use asn in labels and there is no need to add new field in sliver.

One more question is about peers to be added to labels. It could be a list of dicts as shown below. Does it look ok to you?

peers= [ { "bfd": 0, "ip_version": "ipv4", "peer_ip": "192.168.1.1/24", "peer_asn": "1", "local_ip": "192.168.1.2/24", "md5_key": "" } ]

ibaldin commented 1 year ago

For acount ID seems most prudent to limit it to alphanumeric, dashes, slashes, periods and limit in length to something reasonable?

For ASNs I think this is resolved - you can always use a list of ASNs where 0th element is local and first element is remote.

Regarding the peers - this is definitely not a label. Labels are limited to strings only. If you want it to be a dictionary like you describe it has to become a separate field on some sliver (exportable as a dict, for instance). What are the fields that you want:

  1. bfd?
  2. ip_version - what does it relate to? the IP fields?
  3. md5_key what's is that for?

This sounds to me like you need a node in the graph with labels like above or similar. Can you describe how this will be used?

zlion commented 1 year ago

The account ID and ASNs look good to me.

Could you add another field peers on the NetworkServiceSliver? Here is its definition according to OESS documents.

"peers": [ { // Required. ipv4 or ipv6 "ip_version": "ipv4", // Required "local_ip": "192.168.1.3/31", // Required "peer_asn": 65650, // Required "peer_ip": "192.168.1.2/31", // Required. Null if BGP auth is disabled "md5_key": null, // Zero or One. Indicates BFD state "bfd": 0 } ]

zlion commented 1 year ago

@ibaldin Hello. Just want to touch the base on the code changes. Is it released to the public. Thanks.

ibaldin commented 1 year ago

Working on it, got sidelined today into supporting new rack type. Will work tonight to see if I can finish it.

ibaldin commented 1 year ago

This is now replaced with #148 . This one will be closed/not merged.