feat: add ingress score #32

taha · 2025-05-07T01:18:54Z

taha commented

2025-05-07 01:18:54 +00:00

No description provided.

wjro approved these changes 2025-05-12 19:51:55 +00:00

johnride requested changes 2025-05-14 15:33:38 +00:00

johnride left a comment

Only thing that must be improved at the moment is to use correct type safe types in the Score definition.

harmony/src/modules/k8s/ingress.rs Outdated

						
				@ -0,0 +11,4 @@

				use super::resource::{K8sResourceInterpret, K8sResourceScore};

				#[derive(Debug, Clone, Serialize)]

				pub struct K8sIngressScore {

johnride commented

2025-05-14 14:56:13 +00:00

I think it's now worth the time to use correct types for each k8s field types.

Name should not be a string as it doesn't support all utf-8 strings, host should probably be a URL, etc. Look at the k8s spec and implement the right types.

This will allow us to create macros eventually for a compile-safe DX/UX.

I think it's now worth the time to use correct types for each k8s field types. Name should not be a string as it doesn't support all utf-8 strings, host should probably be a URL, etc. Look at the k8s spec and implement the right types. This will allow us to create macros eventually for a compile-safe DX/UX.

harmony/src/modules/k8s/ingress.rs Outdated

						
				@ -0,0 +15,4 @@

				    pub name: String,

				    pub host: String,

				    pub backend_service: String,

				    pub port: String,

johnride commented

2025-05-14 14:54:19 +00:00

should be integer https://docs.redhat.com/en/documentation/openshift_container_platform/4.15/html/network_apis/ingress-networking-k8s-io-v1#spec-rules-http-paths-backend-service-port

harmony/src/modules/k8s/ingress.rs Outdated

						
				@ -0,0 +67,4 @@

				    }

				    fn name(&self) -> String {

				        "K8sIngressScore".to_string()

johnride commented

2025-05-14 14:57:45 +00:00

Not specific to this p-r but in general I think we should revisit the concept of Score names... this is not very useful in a list of multiple K8sIngressScores in the TUI for example.

harmony/src/modules/lamp.rs Outdated

						
				@ -133,3 +134,3 @@

				        info!("LAMP deployment_score {deployment_score:?}");

				        Ok(Outcome::success("Successfully deployed LAMP Stack!".to_string()))

				        let lamp_ingress = K8sIngressScore {

johnride commented

2025-05-14 15:02:03 +00:00

~~There should maybe be an option in LampConfig to enable/disable ingress creation~~

No, after writing this I think it's fine to impose an ingress creation here. Most use cases will want one. If/when we stumble on a legitimate use case to disable the ingress we will add the feature. But for now it breaks the idea of the LAMPScore which is to provide the simplest way to deploy a LAMP application anywhere.

~~There should maybe be an option in LampConfig to enable/disable ingress creation~~ No, after writing this I think it's fine to impose an ingress creation here. Most use cases will want one. If/when we stumble on a legitimate use case to disable the ingress we will add the feature. But for now it breaks the idea of the LAMPScore which is to provide the simplest way to deploy a LAMP application anywhere.

johnride commented

2025-05-14 15:20:36 +00:00

(out of scope for this PR but worth the read here in context)

Below comment related to this ADR : https://git.nationtech.io/NationTech/harmony/src/branch/master/adr/003-infrastructure-abstractions.md

Thinking about the idea of using a K8sIngressScore here :

A very important idea in Harmony is that we provide an opinionated infrastructure (generally K8s + Ceph + OPNSense) but the user knows nothing about it.

However, the next stage is, as we already did for all the firewall related services (LoadBalancer, Router, TftpServer, etc), is to create sensibles abstractions of the infrastructure components of an application.

That means that we should define a harmony "Ingress" that only has information required by harmony to deploy the concept of an ingress. Then Harmony will provide opinionated implementations of the Ingress object for the various supported Topologies.

For now, this covers only k8s stuff, but at some point (probably soon), it will make sense to provide other implementations.

Let's say for one of our current clients, we have to make a deployment on a windows server. Instead of using k3d, we could provide a more windows friendly implementation based on something else, which is not easily doable when we define a K8sIngressScore, but would be totally natural if it were an IngressScore that requires the Ingress capability in the associated Topology.

So the signature would become :

impl <T: Topology + Ingress> Score<T> for IngressScore { ... }

pub struct IngressConfig {
  pub port: integer,
  pub host: Hostname, // probably a harmony specific type too
  // Other fields that are required in the abstraction of an ingress, nothing implementation specific
}

pub trait Ingress {
  fn create(&self, config: IngressConfig) -> Result<...>
}

impl Ingress for K8sAnywhereTopology {
  fn create(&self, config: IngressConfig) -> Result<...> {
    K8sIngressScore::from(config); // just an idea here but it could make sense to have an easy way to generate a score that is specific to the current topology's technology from the more core Harmony abstract Scores? I feel like this would favor a cohesive, yet easy to use set of abstract/concrete scores.
  }
}

(out of scope for this PR but worth the read here in context) Below comment related to this ADR : https://git.nationtech.io/NationTech/harmony/src/branch/master/adr/003-infrastructure-abstractions.md Thinking about the idea of using a K8sIngressScore here : A very important idea in Harmony is that we provide an opinionated infrastructure (generally K8s + Ceph + OPNSense) but the user knows nothing about it. However, the next stage is, as we already did for all the firewall related services (LoadBalancer, Router, TftpServer, etc), is to create sensibles abstractions of the infrastructure components of an application. That means that we should define a harmony "Ingress" that only has information required by harmony to deploy the concept of an ingress. Then Harmony will provide opinionated implementations of the Ingress object for the various supported Topologies. For now, this covers only k8s stuff, but at some point (probably soon), it will make sense to provide other implementations. Let's say for one of our current clients, we have to make a deployment on a windows server. Instead of using k3d, we could provide a more windows friendly implementation based on something else, which is not easily doable when we define a K8sIngressScore, but would be totally natural if it were an IngressScore that requires the Ingress capability in the associated Topology. So the signature would become : ```rust impl <T: Topology + Ingress> Score<T> for IngressScore { ... } pub struct IngressConfig { pub port: integer, pub host: Hostname, // probably a harmony specific type too // Other fields that are required in the abstraction of an ingress, nothing implementation specific } pub trait Ingress { fn create(&self, config: IngressConfig) -> Result<...> } impl Ingress for K8sAnywhereTopology { fn create(&self, config: IngressConfig) -> Result<...> { K8sIngressScore::from(config); // just an idea here but it could make sense to have an easy way to generate a score that is specific to the current topology's technology from the more core Harmony abstract Scores? I feel like this would favor a cohesive, yet easy to use set of abstract/concrete scores. } } ```

taha force-pushed add-ingress-score from fff4ba0b21 to 966fd757bc

2025-05-15 03:37:21 +00:00

Compare

taha reviewed 2025-05-15 03:38:17 +00:00

harmony/Cargo.toml

						
				@ -39,3 +39,11 @@ lazy_static = "1.5.0"

				dockerfile_builder = "0.1.5"

				temp-file = "0.1.9"

				convert_case.workspace = true

				fqdn = { version = "0.4.6", features = [

taha commented

2025-05-15 03:38:17 +00:00

https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#rfc-1035-label-names

👍 1

johnride requested changes 2025-05-15 14:56:46 +00:00

harmony/src/modules/k8s/ingress.rs Outdated

						
				@ -0,0 +18,4 @@

				    pub port: u16,

				    pub path: Option<String>,

				    pub path_type: Option<String>,

				    pub namespace: Option<String>,

johnride commented

2025-05-15 14:29:55 +00:00

What about path, path_type and namespace? These certainly don't support any String?

taha commented

2025-05-15 15:31:20 +00:00

Seems like validating the path is not as straightforward as the rest, as it has different validations based on the PathType and other things: https://github.com/kubernetes/ingress-nginx/issues/11176

Fixed NS and PathType though

Seems like validating the `path` is not as straightforward as the rest, as it has different validations based on the PathType and other things: https://github.com/kubernetes/ingress-nginx/issues/11176 Fixed NS and PathType though

johnride commented

2025-05-15 15:36:39 +00:00

https://kubernetes.io/docs/reference/kubernetes-api/service-resources/ingress-v1/#IngressSpec