oxidecomputer/omicron

500 error returned on BGP config create.

Closed this issue · 4 comments

rcgoodfellow commented

Further details will be provided as they become available.

rcgoodfellow commented

Ok. Don't really need more info, can just read the code. This call stack.

  • omicron/nexus/src/external_api/http_entrypoints.rs

    Lines 3902 to 3919 in 95f1842

    async fn networking_bgp_config_create(
    rqctx: RequestContext<ApiContext>,
    config: TypedBody<params::BgpConfigCreate>,
    ) -> Result<HttpResponseCreated<BgpConfig>, HttpError> {
    let apictx = rqctx.context();
    let handler = async {
    let nexus = &apictx.context.nexus;
    let config = config.into_inner();
    let opctx = crate::context::op_context_for_external_api(&rqctx).await?;
    let result = nexus.bgp_config_set(&opctx, &config).await?;
    Ok(HttpResponseCreated::<BgpConfig>(result.into()))
    };
    apictx
    .context
    .external_latencies
    .instrument_dropshot_handler(&rqctx, handler)
    .await
    }

  • omicron/nexus/src/app/bgp.rs

    Lines 18 to 26 in 95f1842

    pub async fn bgp_config_set(
    &self,
    opctx: &OpContext,
    config: &params::BgpConfigCreate,
    ) -> CreateResult<BgpConfig> {
    opctx.authorize(authz::Action::Modify, &authz::FLEET).await?;
    let result = self.db_datastore.bgp_config_set(opctx, config).await?;
    Ok(result)
    }

  • omicron/nexus/db-queries/src/db/datastore/bgp.rs

    Lines 31 to 114 in 95f1842

    pub async fn bgp_config_set(
    &self,
    opctx: &OpContext,
    config: &params::BgpConfigCreate,
    ) -> CreateResult<BgpConfig> {
    use db::schema::bgp_config::dsl;
    use db::schema::{
    bgp_announce_set, bgp_announce_set::dsl as announce_set_dsl,
    };
    use diesel::sql_types;
    use diesel::IntoSql;
    let conn = self.pool_connection_authorized(opctx).await?;
    self.transaction_retry_wrapper("bgp_config_set")
    .transaction(&conn, |conn| async move {
    let announce_set_id: Uuid = match &config.bgp_announce_set_id {
    NameOrId::Name(name) => {
    announce_set_dsl::bgp_announce_set
    .filter(bgp_announce_set::time_deleted.is_null())
    .filter(bgp_announce_set::name.eq(name.to_string()))
    .select(bgp_announce_set::id)
    .limit(1)
    .first_async::<Uuid>(&conn)
    .await?
    }
    NameOrId::Id(id) => *id,
    };
    let config =
    BgpConfig::from_config_create(config, announce_set_id);
    let matching_entry_subquery = dsl::bgp_config
    .filter(dsl::name.eq(Name::from(config.name().clone())))
    .filter(dsl::time_deleted.is_null())
    .select(dsl::name);
    // SELECT exactly the values we're trying to INSERT, but only
    // if it does not already exist.
    let new_entry_subquery = diesel::dsl::select((
    config.id().into_sql::<sql_types::Uuid>(),
    config.name().to_string().into_sql::<sql_types::Text>(),
    config
    .description()
    .to_string()
    .into_sql::<sql_types::Text>(),
    config.asn.into_sql::<sql_types::BigInt>(),
    config.bgp_announce_set_id.into_sql::<sql_types::Uuid>(),
    config
    .vrf
    .clone()
    .into_sql::<sql_types::Nullable<sql_types::Text>>(),
    Utc::now().into_sql::<sql_types::Timestamptz>(),
    Utc::now().into_sql::<sql_types::Timestamptz>(),
    ))
    .filter(diesel::dsl::not(diesel::dsl::exists(
    matching_entry_subquery,
    )));
    diesel::insert_into(dsl::bgp_config)
    .values(new_entry_subquery)
    .into_columns((
    dsl::id,
    dsl::name,
    dsl::description,
    dsl::asn,
    dsl::bgp_announce_set_id,
    dsl::vrf,
    dsl::time_created,
    dsl::time_modified,
    ))
    .execute_async(&conn)
    .await?;
    dsl::bgp_config
    .filter(dsl::name.eq(Name::from(config.name().clone())))
    .filter(dsl::time_deleted.is_null())
    .select(BgpConfig::as_select())
    .limit(1)
    .first_async(&conn)
    .await
    })
    .await
    .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
    }

ends with

Which means any error from this call is going to result in a 500.

rcgoodfellow commented

In the parts of the datastore code that are returning errors.

This should probably return something 404-ish on error.

omicron/nexus/db-queries/src/db/datastore/bgp.rs

Lines 48 to 54 in 95f1842

announce_set_dsl::bgp_announce_set
.filter(bgp_announce_set::time_deleted.is_null())
.filter(bgp_announce_set::name.eq(name.to_string()))
.select(bgp_announce_set::id)
.limit(1)
.first_async::<Uuid>(&conn)
.await?

This should probably return something 409-ish

omicron/nexus/db-queries/src/db/datastore/bgp.rs

Lines 89 to 102 in 95f1842

diesel::insert_into(dsl::bgp_config)
.values(new_entry_subquery)
.into_columns((
dsl::id,
dsl::name,
dsl::description,
dsl::asn,
dsl::bgp_announce_set_id,
dsl::vrf,
dsl::time_created,
dsl::time_modified,
))
.execute_async(&conn)
.await?;

taspelund commented

@rcgoodfellow do you want me to take a swing at fixing this one?

rcgoodfellow commented

This is a part of #6245 that @internet-diglett is currently working on. But please feel free to coordinate with Levon to potentially pair up on this.