-
Notifications
You must be signed in to change notification settings - Fork 565
Handle deployment url for ingress, gateway etc.. #4234
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
base: develop
Are you sure you want to change the base?
Conversation
|
@claude full-review |
This comment was marked as outdated.
This comment was marked as outdated.
|
@claude full-review |
This comment was marked as outdated.
This comment was marked as outdated.
|
Classification template updates in |
|
@claude full-review |
This comment was marked as resolved.
This comment was marked as resolved.
Documentation Link Check Results❌ Absolute links check failed |
stefannica
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this idea of collecting all possible URLs and leaving it to the user to decide which one to use.
I like it so much in fact, that I think we should formalize it and allow ALL deployments to have one or more URLs in the main DeploymentResponse model, one default and some alternative ones.
| class KubernetesUrlPreference(StrEnum): | ||
| """URL preference for Kubernetes deployer when multiple URL types are available.""" | ||
|
|
||
| GATEWAY_API = "gateway_api" | ||
| INGRESS = "ingress" | ||
| LOAD_BALANCER = "load_balancer" | ||
| NODE_PORT = "node_port" | ||
| CLUSTER_IP = "cluster_ip" | ||
| AUTO = "auto" | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this belongs in a file somewhere under the kubernetes integration, not here.
|
|
||
| Tip: When using Ingress or Gateway API, combine `service_type="ClusterIP"` with the matching `url_preference` so the returned URL matches the routing layer you manage (and you avoid paying for an extra LoadBalancer). | ||
|
|
||
| - Strict preference behavior: If you set `url_preference` to a specific type and that URL can't be discovered (for example, LoadBalancer is still pending), the deployer raises an error instead of returning another URL type. This helps avoid accidental exposure paths. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this neat little insight!
| gateway: Union[Dict[str, Any], Any], | ||
| httproute: Union[Dict[str, Any], Any], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know there are no typed objects in the kubernetes client library for things like gateway and httproute, but using dictionaries is really error prone and difficult to read/understand. I suggest you define some simple pydantic objects that map to the attributes that you're using from these objects instead of using these unpredictable .get calls. Generally speaking, when it feels like you're using a lot of .get, hasattr and getattr calls, this is a sign that you're not using Python typing features correctly.
| name=httproute_item.name, | ||
| namespace=httproute_namespace, | ||
| kind="HTTPRoute", | ||
| api_version="gateway.networking.k8s.io/v1beta1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how likely is it that these versions will change or have changed ? shouldn't you try to cover all the relevant versions, past and future ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great insight, this is probably changing sometime soon once this get's more stable, hmmm but what would be the right way to handle this? let user pass the api version?
| name=gateway_item.name, | ||
| namespace=gateway_item_namespace, | ||
| kind="Gateway", | ||
| api_version="gateway.networking.k8s.io/v1beta1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: how likely is it that these versions will change or have changed ? shouldn't you try to cover all the relevant versions, past and future ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
| inventory, "Gateway", "gateway.networking.k8s.io/v1beta1" | ||
| ) | ||
| httproute_items = _filter_inventory( | ||
| inventory, "HTTPRoute", "gateway.networking.k8s.io/v1beta1" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about previous/future versions of these resources? can you check and make sure that this code is also valid on other kubernetes versions, past and (preferably) future?
| httproute_spec = httproute_dict.get("spec", {}) | ||
| httproute_rules = httproute_spec.get("rules", []) | ||
|
|
||
| routes_to_service = False | ||
| for rule in httproute_rules: | ||
| backend_refs = rule.get("backendRefs", []) | ||
| for backend_ref in backend_refs: | ||
| backend_service_name = backend_ref.get("name") | ||
| backend_namespace = ( | ||
| backend_ref.get("namespace") or httproute_namespace | ||
| ) | ||
|
|
||
| if ( | ||
| backend_service_name == service_item.name | ||
| and backend_namespace == service_namespace | ||
| ): | ||
| routes_to_service = True | ||
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here: prefer typed objects to these interminable and opaque .get calls.
|
|
Describe changes
I implemented improved URL discovery and preference handling for Kubernetes deployments to achieve better control over which URL is returned when multiple endpoint types are available (Gateway API, Ingress, LoadBalancer, NodePort, ClusterIP).
Key changes:
url_preferencesetting to explicitly select URL typeThis allows users to explicitly request specific URL types (e.g.,
url_preference="ingress") and raises clear errors if the requested type is unavailable, preventing accidental fallback to unexpected endpoints.Pre-requisites
developand the open PR is targetingdevelop.Types of changes