search

l7mp / stunner-gateway-operator

STUNner Kubernetes Gateway Operator
Apache License 2.0
15 stars 6 forks source link

test: Hinting public address in gateway spec #29

Closed davidkornel closed 1 year ago

davidkornel commented 1 year ago

Add missing test about hinting the public address in the Gateway spec.

Setting the address should set the LoadBalancerIP in the rendered LB svc.

@rg0now we talked about this but not sure what we agreed on. I'm opening this PR to point out that we miss the tests for the feature specified in the title. Also, we don't have any documentation about it. I made some docs locally but as we were not sure if it is going to work anywhere because the field that we use to set the LoadBalancer's IP is deprecated and probably not enabled by 99% of the cloud providers.

The docs about the field:

    // Only applies to Service Type: LoadBalancer.
    // This feature depends on whether the underlying cloud-provider supports specifying
    // the loadBalancerIP when a load balancer is created.
    // This field will be ignored if the cloud-provider does not support the feature.
    // Deprecated: This field was under-specified and its meaning varies across implementations,
    // and it cannot support dual-stack.
    // As of Kubernetes v1.24, users are encouraged to use implementation-specific annotations when available.
    // This field may be removed in a future API version.
    // +optional
    LoadBalancerIP string `json:"loadBalancerIP,omitempty" protobuf:"bytes,8,opt,name=loadBalancerIP"`
rg0now commented 1 year ago

Thanks, this seems really useful.

One additional thing we should maybe document and test is the following behavior: if Gateway.Spec.Addresses is set then in the stunnerd-config the public IP address (PublicAddr) should be unconditionally set to that same address. If you add a test for this, that should go into internal/render/render_pipeline_test.go.

Wdyt?

davidkornel commented 1 year ago

Closing it in favor of #32 Better to have it on the same branch