[bgp-config] add `networking_bgp_config_update` endpoint by nicolaskagami · Pull Request #10566 · oxidecomputer/omicron

nicolaskagami · 2026-06-08T18:53:43Z

Adds a networking_bgp_config_update endpoint with the following updatable fields:

pub struct BgpConfigUpdate {
    #[serde(flatten)]
    pub identity: IdentityMetadataUpdateParams,

    /// Update the BGP announce set associated with this configuration.
    pub bgp_announce_set_id: Option<NameOrId>,

    /// Update the maximum number of equal-cost paths.
    pub max_paths: Option<MaxPathConfig>,
}

vrf isn't added because it's to be removed.
asn isn't included because it may be used as an id internally.

closes #10066

nicolaskagami · 2026-06-12T14:41:52Z

(This is my first PR as an Oxide employee, so I'm still learning the ropes)

This was made drawing from Levon's old PR and the neighboring bgp-config methods. I changed a couple of things to improve readability (e.g. repeated code, some clone()s) and introduced the use of IdentityMetadataUpdateParams, which seems to be standard.

I also didn't attempt to remove the vrf field from the create / etc, as that might have other implications. Would we like to deprecate it first? Should we jump straight to failing vrf-containing calls?

Lastly, I wrote a little unit test mostly because it simply pains me not to do it. It was great because I found a (rather innocuous) bug on bgp_config_get (#10609) and learned some more about how things work.

jgallagher

LGTM, just a few small nits and a docs phrasing question.

jgallagher · 2026-06-18T19:50:51Z

+            let lookup_err = |e, not_found_err, msg| {
+                error!(opctx.log, "{msg}"; "error" => ?e);


Couple minor nits: (1) I don't think we're super consistent with this, but error! seems like too high of a log level for the NotFound case and probably even the internal_error case. Could we make this a warn! instead? (2) We should use the InlineErrorChain adapter instead of ?e so we know we get any underlying causes (docs on this):

Suggested change

let lookup_err = |e, not_found_err, msg| {

error!(opctx.log, "{msg}"; "error" => ?e);

let lookup_err = |e: diesel::result::Error, not_found_err, msg| {

warn!(opctx.log, "{msg}"; InlineErrorChain::new(&e));

(I think both of these also apply to the other error! logs added below)

I agree 100% but I didn't want to stray away from the current standard we had for the other bgp_config methods (e.g. bgp_config_create). Would it be alright if I tackle those as well but on a follow-up PR?

Yeah, works for me 👍

jgallagher · 2026-06-18T19:57:54Z

+                .map_err(|e| lookup_err(e, not_found_err, msg))?;
+
+            // Resolve bgp_announce_set_id if an update was requested
+            let new_bgp_announce_set_id = match &update.bgp_announce_set_id {


Instead of accessing the fields of update, can we destructure it like this:

let networking::BgpConfigUpdate { identity: update_identity, bgp_announce_set_id: update_bgp_announce_set_id, max_paths: update_max_paths, } = update;

and then refer to those bound fields here and on lines 314-327? This way if someone adds a new field to BgpConfigUpdate, they get a compilation error here and are forced to update it, instead of (potentially) forgetting to, and then we silently fail to update some fields.

jgallagher · 2026-06-18T20:00:01Z

+                    bgp_config_dsl::description.eq(new_description),
+                    bgp_config_dsl::bgp_announce_set_id
+                        .eq(new_bgp_announce_set_id),
+                    bgp_config_dsl::max_paths.eq(i16::from(new_max_paths)),


End result is the same, but it's clearer to use our unsigned integer adapters here:

Suggested change

bgp_config_dsl::max_paths.eq(i16::from(new_max_paths)),

bgp_config_dsl::max_paths.eq(SqlU8(new_max_paths)),

jgallagher · 2026-06-18T20:06:33Z

+          "system/networking"
+        ],
+        "summary": "Update BGP configuration",
+        "description": "Update the mutable fields of an existing BGP configuration. The `asn` field is intentionally not updatable; changing the autonomous system number requires creating a new BGP configuration object, since many things are keyed off the ASN.",


This may be leaking out more information than is relevant for a caller of this endpoint; I wonder if we could get away with just

Update the mutable fields of an existing BGP configuration.

or you think people might assume they can change the ASN here, we could maybe shorten this slightly:

Update the mutable fields of an existing BGP configuration. The asn field is not updatable; to change the autonomous system number, create a new BGP configuration object.

@ahl might have wordsmithing suggestions?

I liked your second suggestion better:

Update the mutable fields of an existing BGP configuration. The asn field is not updatable; to change the autonomous system number, create a new BGP configuration object.

Makes sense to me.

ahl · 2026-06-18T20:43:41Z

+          "system/networking"
+        ],
+        "summary": "Update BGP configuration",
+        "description": "Update the mutable fields of an existing BGP configuration. The `asn` field is intentionally not updatable; changing the autonomous system number requires creating a new BGP configuration object, since many things are keyed off the ASN.",


Makes sense to me.

ahl · 2026-06-18T21:06:37Z

        ]
      },
+      "BgpConfigUpdate": {
+        "description": "Parameters for updating a BGP configuration.\n\nThe `asn` field is intentionally not updatable; changing the autonomous system number requires creating a new BGP configuration object, since many things are keyed off the ASN.",


This feels redundant with the description above; how about

Suggested change

"description": "Parameters for updating a BGP configuration.\n\nThe `asn` field is intentionally not updatable; changing the autonomous system number requires creating a new BGP configuration object, since many things are keyed off the ASN.",

"description": "New values when updating a BGP configuration.",

In particular I think it's important to specify that it's all the new values--leaving a field unpopulated effectively deletes it. In some cases we've use the Nullable newtype wrapper to force users to specify values

Actually, those are not the update semantics we're following here (!). More specifically, the current implementation keeps the old values for unpopulated fields in the update object.

I tried to closely follow Levon's reference implementation and we do have a few optional update fields being updated with PUT (where a case could be made for PATCH). Example:

omicron/nexus/types/versions/src/initial/silo.rs

Lines 219 to 239 in 9fc857e

#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]

pub struct SiloQuotasCreate {

/// The amount of virtual CPUs available for running instances in the Silo

pub cpus: i64,

/// The amount of RAM (in bytes) available for running instances in the Silo

pub memory: ByteCount,

/// The amount of storage (in bytes) available for disks or snapshots

pub storage: ByteCount,

}

/// Updateable properties of a Silo's resource limits.

/// If a value is omitted it will not be updated.

#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]

pub struct SiloQuotasUpdate {

/// The amount of virtual CPUs available for running instances in the Silo

pub cpus: Option<i64>,

/// The amount of RAM (in bytes) available for running instances in the Silo

pub memory: Option<ByteCount>,

/// The amount of storage (in bytes) available for disks or snapshots

pub storage: Option<ByteCount>,

}

In the case of BgpConfig as well, the fields in the database representation are not optional, so for now they can at least be consistent in this interpretation. However, I'm unsure (and uneasy) about this strategy in the long run, where we might want to add Nullable fields and have to change update semantics.

Can you point me to some discussion/documentation on this?

nicolaskagami self-assigned this Jun 8, 2026

nicolaskagami force-pushed the nsk/bgp-config-update-endpoint branch from 73050fb to 9ca6ecf Compare June 10, 2026 13:16

add bgp_config_update endpoint

b78fcef

nicolaskagami force-pushed the nsk/bgp-config-update-endpoint branch from 9ca6ecf to b78fcef Compare June 10, 2026 19:17

nicolaskagami added 3 commits June 10, 2026 17:58

add smoke test for bgp_config_update

824b565

properly cleanup the test

c3143bd

use IdentityMetadataUpdateParams

16ae7a8

nicolaskagami mentioned this pull request Jun 11, 2026

[bgp-config] fix bgp_config_get by name #10609

Open

fix to use BgpConfigUpdate on VERIFY_ENDPOINTS integration test

dd68cb5

nicolaskagami changed the title ~~[bgp] add networking_bgp_config_update endpoint~~ [bgp-config] add networking_bgp_config_update endpoint Jun 11, 2026

nicolaskagami marked this pull request as ready for review June 11, 2026 21:56

nicolaskagami requested a review from internet-diglett June 11, 2026 21:56

nicolaskagami requested a review from jgallagher June 16, 2026 19:43

jgallagher approved these changes Jun 18, 2026

View reviewed changes

ahl reviewed Jun 18, 2026

View reviewed changes

		let lookup_err = \|e, not_found_err, msg\| {
		error!(opctx.log, "{msg}"; "error" => ?e);

	bgp_config_dsl::max_paths.eq(i16::from(new_max_paths)),
	bgp_config_dsl::max_paths.eq(SqlU8(new_max_paths)),

	"description": "Parameters for updating a BGP configuration.\n\nThe `asn` field is intentionally not updatable; changing the autonomous system number requires creating a new BGP configuration object, since many things are keyed off the ASN.",
	"description": "New values when updating a BGP configuration.",

	#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
	pub struct SiloQuotasCreate {
	/// The amount of virtual CPUs available for running instances in the Silo
	pub cpus: i64,
	/// The amount of RAM (in bytes) available for running instances in the Silo
	pub memory: ByteCount,
	/// The amount of storage (in bytes) available for disks or snapshots
	pub storage: ByteCount,
	}

	/// Updateable properties of a Silo's resource limits.
	/// If a value is omitted it will not be updated.
	#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
	pub struct SiloQuotasUpdate {
	/// The amount of virtual CPUs available for running instances in the Silo
	pub cpus: Option<i64>,
	/// The amount of RAM (in bytes) available for running instances in the Silo
	pub memory: Option<ByteCount>,
	/// The amount of storage (in bytes) available for disks or snapshots
	pub storage: Option<ByteCount>,
	}

Conversation

nicolaskagami commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolaskagami commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgallagher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nicolaskagami commented Jun 8, 2026 •

edited

Loading

nicolaskagami commented Jun 12, 2026 •

edited

Loading