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

Export Generated BGP Configuration Code #2688

Closed
wants to merge 5 commits into from

Conversation

wenovus
Copy link
Contributor

@wenovus wenovus commented Aug 17, 2023

Move internal/pkg/config -> pkg/config/gobgp

Background: #2593

Previously I opened #2599 but I think this approach is likely the best since it preserves backwards-compatibility with minimal changes.

@wenovus
Copy link
Contributor Author

wenovus commented Sep 19, 2023

@fujita ping on this as the preferred way to address #2593, do you have any concerns?

Move internal/pkg/config -> pkg/config/gobgp

Background: osrg#2593
@fujita
Copy link
Member

fujita commented Sep 27, 2023

renaming config to gobgp is really confusing; lost the context. I think that we needs a better name; openconfig? model?

@wenovus
Copy link
Contributor Author

wenovus commented Sep 27, 2023

renaming config to gobgp is really confusing; lost the context. I think that we needs a better name; openconfig? model?

Thanks, here is a list of options I've seen/thought of:

  • I think openconfig might not appropriate since GoBGP has augmented the original models, essentially creating its own "vendor models".
  • In keeping with "I don't think using bgp prefix for gobgp packages is a good idea. Just verbose.", then model sounds better to me. (preferred)
  • I'd like to propose gobgpoc, where oc is a common abbreviation for "openconfig", and gobgp qualifies it as being a vendor-augmented form of OpenConfig rather than the standard, pure OpenConfig from https://github.com/openconfig/public
  • There is another option described here: Make generated BGP configuration code public #2599 (comment)
    pkg/internal/config -> pkg/config
    pkg/config -> pkg/configop, or pkg/gobgp
    
    the downside is the renaming of an already-exported package which may necessitate a major version bump. This is because it's possible for a user to be using config.ReadConfigFile and config.InitialConfig to write their own gobgpd.

@fujita
Copy link
Member

fujita commented Sep 27, 2023

OpenConfig is supposed to be used to build vendor models?
What vendor uses pure OpenConfig model?

@wenovus
Copy link
Contributor Author

wenovus commented Sep 28, 2023

OpenConfig is supposed to be used to build vendor models? What vendor uses pure OpenConfig model?

Yes OpenConfig is the model used to access vendor device features. Ideally no vendor deviations/augments are required but they do happen. For example in openconfig/featureprofiles, tests are written using the "pure" OpenConfig models, but there are if conditions in the tests that check deviations from the pure model that different vendors may have. The pure model itself is also constantly changing to meet new requirements or for other improvements, which is why it's now starting to be versioned.

Therefore as a user of GoBGP, I see pkg/config/? as a non-pure OpenConfig catered to GoBGP since the YANG file has been augmented with new features in multiple places, whereas featureprofiles's API is pure OpenConfig since its YANG is non-altered: https://github.com/openconfig/ondatra/blob/main/gnmi/generate.sh, hence why it's named oc.

@fujita
Copy link
Member

fujita commented Sep 28, 2023

Using gobgp twice in package path is confusing, so model or oc works for me.

@wenovus
Copy link
Contributor Author

wenovus commented Sep 30, 2023

Using gobgp twice in package path is confusing, so model or oc works for me.

Actually now I prefer oc a bit more because the model is largely based off of OpenConfig and therefore is a pretty representative name. The fact that it exists in GoBGP should warn users that it may deviate from the pure OpenConfig models.

PTAL, thanks.

@fujita
Copy link
Member

fujita commented Sep 30, 2023

pushed, thanks!

@fujita fujita closed this Sep 30, 2023
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.

2 participants