Skip to content
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

Add support for using gateway.spec.addresses as service external ips #1322

Merged
merged 13 commits into from
Apr 27, 2023

Conversation

shawnh2
Copy link
Contributor

@shawnh2 shawnh2 commented Apr 17, 2023

since service.externalIP only support IP addresses, in this implementation, only the gateway.spec.addresses with type IPAddress has been used.

refer to #360

@shawnh2 shawnh2 requested a review from a team as a code owner April 17, 2023 15:11
@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #1322 (c51aca2) into main (12aa0ad) will decrease coverage by 0.02%.
The diff coverage is 78.26%.

@@            Coverage Diff             @@
##             main    #1322      +/-   ##
==========================================
- Coverage   62.31%   62.30%   -0.02%     
==========================================
  Files          78       79       +1     
  Lines       10895    10918      +23     
==========================================
+ Hits         6789     6802      +13     
- Misses       3656     3664       +8     
- Partials      450      452       +2     
Impacted Files Coverage Δ
internal/ir/infra.go 64.36% <ø> (ø)
internal/ir/zz_generated.deepcopy.go 12.89% <0.00%> (-0.11%) ⬇️
internal/gatewayapi/address.go 100.00% <100.00%> (ø)
internal/gatewayapi/translator.go 97.22% <100.00%> (+0.12%) ⬆️
...frastructure/kubernetes/proxy/resource_provider.go 88.80% <100.00%> (+0.04%) ⬆️
internal/status/gateway.go 93.47% <100.00%> (+0.29%) ⬆️

... and 2 files with indirect coverage changes

tommie and others added 5 commits April 17, 2023 23:21
Copies the addresses to the generated Service.ExternalIPs.

Signed-off-by: Shawnh2 <[email protected]>
Signed-off-by: Shawnh2 <[email protected]>
Signed-off-by: Shawnh2 <[email protected]>
@shawnh2 shawnh2 force-pushed the udef-gw-addr/external-ips branch from 450c485 to 651b5fa Compare April 17, 2023 15:22
@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 17, 2023

I've run some simple test in local.

apply config altered from quickstart.yaml
apiVersion: gateway.networking.k8s.io/v1beta1
kind: GatewayClass
metadata:
name: eg
spec:
controllerName: gateway.envoyproxy.io/gatewayclass-controller
---
apiVersion: gateway.networking.k8s.io/v1beta1
kind: Gateway
metadata:
name: eg
spec:
gatewayClassName: eg
listeners:
  - name: http
    protocol: HTTP
    port: 80
addresses:
  - type: IPAddress
     value: 1.2.3.4
  - type: IPAddress
    value: 1.2.3.4   # repeat is okay, will only store once
  - type: IPAddress
    value: 5.6.7.8
  - type: Hostname   # do not support, so will skip
     value: foo.bar
---
apiVersion: v1
kind: ServiceAccount
metadata:
name: backend
---
apiVersion: v1
kind: Service
metadata:
name: backend
labels:
  app: backend
  service: backend
spec:
ports:
  - name: http
    port: 3000
    targetPort: 3000
selector:
  app: backend
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: backend
spec:
replicas: 1
selector:
  matchLabels:
    app: backend
    version: v1
template:
  metadata:
    labels:
      app: backend
      version: v1
  spec:
    serviceAccountName: backend
    containers:
      - image: gcr.io/k8s-staging-ingressconformance/echoserver:v20221109-7ee2f3e
        imagePullPolicy: IfNotPresent
        name: backend
        ports:
          - containerPort: 3000
        env:
          - name: POD_NAME
            valueFrom:
              fieldRef:
                fieldPath: metadata.name
          - name: NAMESPACE
            valueFrom:
              fieldRef:
                fieldPath: metadata.namespace
---
apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
name: backend
spec:
parentRefs:
  - name: eg
hostnames:
  - "www.example.com"
rules:
  - backendRefs:
      - group: ""
        kind: Service
        name: backend
        port: 3000
        weight: 1
    matches:
      - path:
          type: PathPrefix
          value: /
checkout backend service
NAME                           READY   STATUS    RESTARTS   AGE
pod/backend-7f6b74f5b7-78xp4   1/1     Running   0          113m

NAME                 TYPE        CLUSTER-IP    EXTERNAL-IP   PORT(S)    AGE
service/backend      ClusterIP   10.96.95.68   <none>        3000/TCP   113m
service/kubernetes   ClusterIP   10.96.0.1     <none>        443/TCP    162m

NAME                      READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/backend   1/1     1            1           113m

NAME                                 DESIRED   CURRENT   READY   AGE
replicaset.apps/backend-7f6b74f5b7   1         1         1       113m
checkout gateway-class and gateway
NAME   CONTROLLER                                      ACCEPTED   AGE
eg     gateway.envoyproxy.io/gatewayclass-controller   True       114m


NAME   CLASS   ADDRESS   PROGRAMMED   AGE
eg     eg      1.2.3.4   True         115m     # it has two addresses, but only one is showing


$kubectl describe gtw eg
Name:         eg
Namespace:    default
Labels:       <none>
Annotations:  <none>
API Version:  gateway.networking.k8s.io/v1beta1
Kind:         Gateway
Metadata:
  Creation Timestamp:  2023-04-17T13:41:50Z
  Generation:          2
  Managed Fields:
    API Version:  gateway.networking.k8s.io/v1beta1
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        f:addresses:
        f:conditions:
          k:{"type":"Accepted"}:
            f:lastTransitionTime:
            f:message:
            f:observedGeneration:
            f:reason:
            f:status:
          k:{"type":"Programmed"}:
            .:
            f:lastTransitionTime:
            f:message:
            f:observedGeneration:
            f:reason:
            f:status:
            f:type:
        f:listeners:
          .:
          k:{"name":"http"}:
            .:
            f:attachedRoutes:
            f:conditions:
              .:
              k:{"type":"Accepted"}:
                .:
                f:lastTransitionTime:
                f:message:
                f:observedGeneration:
                f:reason:
                f:status:
                f:type:
              k:{"type":"Programmed"}:
                .:
                f:lastTransitionTime:
                f:message:
                f:observedGeneration:
                f:reason:
                f:status:
                f:type:
            f:name:
            f:supportedKinds:
    Manager:      envoy-gateway
    Operation:    Update
    Subresource:  status
    Time:         2023-04-17T13:49:42Z
    API Version:  gateway.networking.k8s.io/v1beta1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
      f:spec:
        .:
        f:addresses:
        f:gatewayClassName:
        f:listeners:
          .:
          k:{"name":"http"}:
            .:
            f:allowedRoutes:
              .:
              f:namespaces:
                .:
                f:from:
            f:name:
            f:port:
            f:protocol:
    Manager:         kubectl-client-side-apply
    Operation:       Update
    Time:            2023-04-17T13:49:42Z
  Resource Version:  4881
  UID:               c119f71e-b445-4e2c-8a27-cdce7e969b8e
Spec:
  Addresses:
    Type:              IPAddress
    Value:             1.2.3.4
    Type:              IPAddress
    Value:             1.2.3.4
    Type:              IPAddress
    Value:             5.6.7.8
    Type:              Hostname
    Value:             foo.bar
  Gateway Class Name:  eg
  Listeners:
    Allowed Routes:
      Namespaces:
        From:  Same
    Name:      http
    Port:      80
    Protocol:  HTTP
Status:
  Addresses:
    Type:   IPAddress
    Value:  1.2.3.4
    Type:   IPAddress
    Value:  5.6.7.8
  Conditions:
    Last Transition Time:  2023-04-17T13:49:42Z
    Message:               The Gateway has been scheduled by Envoy Gateway
    Observed Generation:   2
    Reason:                Accepted
    Status:                True
    Type:                  Accepted
    Last Transition Time:  2023-04-17T13:49:42Z
    Message:               Address assigned to the Gateway, 1/1 envoy Deployment replicas available
    Observed Generation:   2
    Reason:                Programmed
    Status:                True
    Type:                  Programmed
  Listeners:
    Attached Routes:  1
    Conditions:
      Last Transition Time:  2023-04-17T13:49:42Z
      Message:               Sending translated listener configuration to the data plane
      Observed Generation:   2
      Reason:                Programmed
      Status:                True
      Type:                  Programmed
      Last Transition Time:  2023-04-17T13:49:42Z
      Message:               Listener has been successfully translated
      Observed Generation:   2
      Reason:                Accepted
      Status:                True
      Type:                  Accepted
    Name:                    http
    Supported Kinds:
      Group:  gateway.networking.k8s.io
      Kind:   HTTPRoute
      Group:  gateway.networking.k8s.io
      Kind:   GRPCRoute
Events:       <none>
checkout service in namespace: envoy-gateway-system
kubectl get svc -n envoy-gateway-system
NAME                            TYPE           CLUSTER-IP      EXTERNAL-IP       PORT(S)        AGE
envoy-default-eg-64656661       LoadBalancer   10.96.42.180    1.2.3.4,5.6.7.8   80:32146/TCP   118m
envoy-gateway                   ClusterIP      10.96.135.124   <none>            18000/TCP      122m
envoy-gateway-metrics-service   ClusterIP      10.96.121.189   <none>            8443/TCP       122m


kubectl describe -n envoy-gateway-system svc envoy-default-eg-64656661
Name:                     envoy-default-eg-64656661
Namespace:                envoy-gateway-system
Labels:                   app.gateway.envoyproxy.io/name=envoy
                          gateway.envoyproxy.io/owning-gateway-name=eg
                          gateway.envoyproxy.io/owning-gateway-namespace=default
Annotations:              <none>
Selector:                 app.gateway.envoyproxy.io/name=envoy,gateway.envoyproxy.io/owning-gateway-name=eg,gateway.envoyproxy.io/owning-gateway-namespace=default
Type:                     LoadBalancer
IP Family Policy:         SingleStack
IP Families:              IPv4
IP:                       10.96.42.180
IPs:                      10.96.42.180
External IPs:             1.2.3.4,5.6.7.8
Port:                     http  80/TCP
TargetPort:               10080/TCP
NodePort:                 http  32146/TCP
Endpoints:                10.244.0.11:10080
Session Affinity:         None
External Traffic Policy:  Local
HealthCheck NodePort:     30880
Events:                   <none>

switch *addr.Type {
case v1beta1.IPAddressType:
if _, err := netip.ParseAddr(addr.Value); err == nil {
ipAddr.Insert(addr.Value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we expose the error in the status condition ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, we cannot expose this error in the status condition.

becasue when we apply an invalid IP address (like a.b.c.d) to Gateway, it will fail to create a Gateway Service,

func (i *Infra) createOrUpdateService(ctx context.Context, svc *corev1.Service) error {
current := &corev1.Service{}
key := types.NamespacedName{
Namespace: svc.Namespace,
Name: svc.Name,
}
if err := i.Client.Get(ctx, key, current); err != nil {
// Create if not found.
if kerrors.IsNotFound(err) {
if err := i.Client.Create(ctx, svc); err != nil {
return fmt.Errorf("failed to create service %s/%s: %w",
svc.Namespace, svc.Name, err)

thus we cannot get this Service when setting the status condition:

func UpdateGatewayStatusProgrammedCondition(gw *gwapiv1b1.Gateway, svc *corev1.Service, deployment *appsv1.Deployment) {
var addresses, hostnames []string
// Update the status addresses field.
if svc != nil {

IMO, we can solve this by validating the Gateway.Spec.Addresses before creating the Service, since the GW API does not provide any validation webhook on this field, should we validate it ourselves or raise an issue in upstream?

cc @arkodg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for raising an issue upstream !

@arkodg
Copy link
Contributor

arkodg commented Apr 17, 2023

thanks for this PR @shawnh2 , the code looks good, added some minor comments
also hoping this PR lands after #1259 which is making changes in a similar area

arkodg
arkodg previously approved these changes Apr 24, 2023
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks

@Xunzhuo
Copy link
Member

Xunzhuo commented Apr 25, 2023

Can you resolve failures on CI ? @shawnh2

@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 25, 2023

The CI has been fixed, @arkodg @Xunzhuo PTAL

Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a doc for this ? @shawnh2

And also need to resolve some conflicts, thanks.

Others LGTM

@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 25, 2023

Can you add a doc for this ? @shawnh2

And also need to resolve some conflicts, thanks.

Others LGTM

Done

arkodg
arkodg previously approved these changes Apr 25, 2023
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks !

@arkodg arkodg requested a review from Xunzhuo April 26, 2023 21:14
Xunzhuo
Xunzhuo previously approved these changes Apr 27, 2023
Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks ! @shawnh2

@Xunzhuo
Copy link
Member

Xunzhuo commented Apr 27, 2023

Ops ... Conflicts again : (

Mind resolve it one more time ? Then I can merge it : ) @shawnh2

@shawnh2 shawnh2 dismissed stale reviews from Xunzhuo and arkodg via c51aca2 April 27, 2023 02:39
@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 27, 2023

Ops ... Conflicts again : (

Mind resolve it one more time ? Then I can merge it : ) @shawnh2

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants