move balancer creation out to a separate cr #34360
Conversation
doy-materialize
force-pushed
the
push-zokpppwmvvrp
branch
from
4dbe038 to
f35188b
Compare
doy-materialize
force-pushed
the
push-wtyxknkkzltv
branch
from
e81e9c1 to
6ecac28
Compare
jubrad
left a comment
jubrad
left a comment
There was a problem hiding this comment.
I think we should add an orchestratord test for this.
test/orchestratord/mzcompose.py
Perhaps not now, but at some point we'll also want to update our automatic doc generation to include balancerd and console
| #[serde(rename_all = "camelCase")] | ||
| pub struct StaticRoutingConfig { | ||
| pub environmentd_namespace: String, | ||
| pub environmentd_service_name: String, |
There was a problem hiding this comment.
Should we be able to determine this from the environment name.
There was a problem hiding this comment.
not in general - we have a specific naming convention in cloud, but it's not required for self-managed
| #[derive(Clone, Debug)] | ||
| pub enum Routing<'a> { | ||
| Static(&'a StaticRoutingConfig), | ||
| Frontegg(&'a FronteggRoutingConfig), | ||
| } |
There was a problem hiding this comment.
I think something like UpstreamTargetting might be more in line with the terminology used in proxies.
There was a problem hiding this comment.
should i also change the field names? pub static_upstream_targetting: Option<StaticUpstreamTargettingConfig> or something like that?
| #[derive(Clone, Debug, PartialEq, Deserialize, Serialize, JsonSchema)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct FronteggRoutingConfig { | ||
| // TODO | ||
| } |
There was a problem hiding this comment.
Is this needed before we merge, or just before we adopt the new model in cloud?
There was a problem hiding this comment.
i think we can wait to figure this out for a future pr
| // Configuration for statically routing traffic | ||
| pub static_routing: Option<StaticRoutingConfig>, | ||
| // Configuration for routing traffic via Frontegg | ||
| pub frontegg_routing: Option<FronteggRoutingConfig>, |
There was a problem hiding this comment.
Are these exclusive? If so should this be an enum?
There was a problem hiding this comment.
yeah - i was considering that but i'm not super sure how tagged union style enums work in the context of a crd (like how would it look in the resource yaml?), and we're also already deriving Default on the crd struct which would also be awkward (we could probably change that but not sure what else that would require). i imagine we're probably going to be iterating on all of these crds going forward, i'm not super worried about being able to write the migration logic or something like that if necessary in the future.
There was a problem hiding this comment.
Fair enough. Would it be worth clarifying that this resource is in development... clearly the resource is v1alpha1 but that might be a bit undercut by the materialize CRD also being v1alpha1.
| fn create_external_certificate_object(&self, balancer: &Balancer) -> Option<Certificate> { | ||
| create_certificate( | ||
| self.config | ||
| .default_certificate_specs | ||
| .balancerd_external | ||
| .clone(), | ||
| balancer, | ||
| balancer.spec.external_certificate_spec.clone(), | ||
| balancer.external_certificate_name(), | ||
| balancer.external_certificate_secret_name(), | ||
| None, | ||
| CertificatePrivateKeyAlgorithm::Rsa, | ||
| Some(4096), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Not for this PR but I would love it if this was configurable so we could use ed25519 here, I think it would drastically reduce CPU usage on balancers.. I think this PR will make making that configurable way easier.
There was a problem hiding this comment.
yeah, that would definitely be nice!
|
we already have tests that the balancerd pods are created correctly - is there something specific you think we should test about the crd? |
I was thinking a test that explicitly created the balancerd CR, but perhaps something that does some validation of the CR that gets created, or ensure delete logic cleans up assoicated CRs and sub resources. |
doy-materialize
force-pushed
the
push-zokpppwmvvrp
branch
from
f35188b to
6091b33
Compare
doy-materialize
force-pushed
the
push-zokpppwmvvrp
branch
2 times, most recently
from
35c28b5 to
6d2d9bb
Compare
doy-materialize
force-pushed
the
push-zokpppwmvvrp
branch
from
6d2d9bb to
ee60b22
Compare
Motivation
simplify our resource management (this should help ensure balancer stuff is decoupled from the environmentd rollout code)
Tips for reviewer
sorry about the size - most of this is just moving code around but it wasn't really easy to make a single pr smaller than this
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.