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

Support Use of Annotations on Cilium Gateway API Load Balancer Services #25357

Closed
2 tasks done
cmcga1125 opened this issue May 10, 2023 · 27 comments
Closed
2 tasks done
Assignees
Labels
area/servicemesh GH issues or PRs regarding servicemesh kind/community-report This was reported by a user in the Cilium community, eg via Slack. kind/feature This introduces new functionality. needs/triage This issue requires triaging to establish severity and next steps. sig/agent Cilium agent related.

Comments

@cmcga1125
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

  1. using Linode's infrastrucuture
  2. create a Kubernetes cluster with cilium as CNI & Gateway Class
  3. Create HTTP Routes & add Cert Manager
  4. Traffic needed to validate the HTTP Cert Request fail / timeout
    Failed to connect to hostname.example.com port 80 after 4 ms: Couldn't connect to server
  5. this is happening because the load-balencer service needs the following annotation for linode LB / pod routing
    service.beta.kubernetes.io/linode-loadbalancer-hostname-only-ingress: "true"
  6. If I manually edit the cilium-gateway-service which generates the LB - the request will work for a moment and then fail as the gateway controller removes the annotations.

is it possible to have the controller to not remove the annotations

  • and/or -

the ability to supply the needed service annotations :)

Cilium Version

v1.13.0

Kernel Version

UBUNTU 22.04
Linux dev-master-0 5.15.0-60-generic #66-Ubuntu SMP Fri Jan 20 14:29:49 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Kubernetes Version

v1.26.3

Sysdump

No response

Relevant log output

No response

Anything else?

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@cmcga1125 cmcga1125 added kind/bug This is a bug in the Cilium logic. kind/community-report This was reported by a user in the Cilium community, eg via Slack. needs/triage This issue requires triaging to establish severity and next steps. labels May 10, 2023
@youngnick youngnick added sig/agent Cilium agent related. area/servicemesh GH issues or PRs regarding servicemesh labels May 11, 2023
@cmcga1125
Copy link
Author

For a bit more details, here's the slack conversation:

https://cilium.slack.com/archives/C53TG4J4R/p1683666665428989

Also, you mentioned upstream being aware, I'd there an issue or creator request I can follow?

@youngnick
Copy link
Contributor

We have an open issue on using paramsRef in GatewayClass, #21923. We absolutely could use that to do this sort of customisation of the generated Service object.

However, also relevant is kubernetes-sigs/gateway-api#1868, which is adding a new infrastructure stanza to Gateways which we can also support, and will allow us to do the same thing.

Practically, depending on the timelines for Gateway API releases and Cilium releases, we may shuffle which one comes first though.

@youngnick
Copy link
Contributor

We don't have an implementation issue yet for the infrastructure stanza, I'll link it in here once I have better info about timings and open one.

@sergeyshevch
Copy link
Contributor

We also met the same issue. We need to configure some annotations on the service for correct usage of AWS NLB that's controlled by the AWS ALB ingress controller. So all users on actual versions of EKS will met the same issue

@sergeyshevch
Copy link
Contributor

@cmcga1125 If you are looking for a workaround. We mitigated such an issue with MutationWebhook by Kyverno. You can use other policy engines as well

@github-actions
Copy link

This issue has been automatically marked as stale because it has not
had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jul 23, 2023
@wwentland
Copy link

Thanks! A MutationWebhook does indeed work well. It would still be nice for Cilium to support this directly though.

@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jul 24, 2023
@rauanmayemir
Copy link
Contributor

rauanmayemir commented Jul 31, 2023

@sergeyshevch Do you mind sharing an example of such policy? For kyverno in particular.

@youngnick
Copy link
Contributor

I should be clear that we are going to build support for this into Cilium, we just have to finish designing it in Gateway API first.

@sergeyshevch
Copy link
Contributor

@youngnick As I see cilium ingress controller currently supports such a thing for dedicated lb mode. I guess we can support this for gateway API same way

@sergeyshevch
Copy link
Contributor

@rauanmayemir Sure. There is our example

Details

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: mutate-rest-gateway-svc
  annotations:
    policies.kyverno.io/title: Mutate Rest Gateway Service
    policies.kyverno.io/severity: medium
    policies.kyverno.io/description: >-
      We need to mutate the rest gateway service for adjusting some annotations.
spec:
  rules:
    - name: mutate-svc-annotations
      match:
        any:
          - resources:
              kinds:
                - Service
              namespaces:
                - gateway-api
              name: cilium-gateway-rest-gateway
      mutate:
        patchStrategicMerge:
          metadata:
            annotations:
              service.beta.kubernetes.io/aws-load-balancer-scheme: internet-facing
              service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443"
              service.beta.kubernetes.io/aws-load-balancer-backend-protocol: tcp

@rauanmayemir
Copy link
Contributor

@sergeyshevch thanks a lot, this is great.

@Cajga
Copy link

Cajga commented Aug 3, 2023

@youngnick would the support of infrastructure in cilium also support NodePorts as gateway service?
Our use case in short: We are on Bare Metal (EKS Anywhere). We got an external and an internal HW load balancer cluster. We would like to have two gateways defined in Cilium (one internal and one external facing) using nodeports and forward the traffic from the LBs to these.

@youngnick
Copy link
Contributor

Ah, that's the use case covered in your CFP (#27273 )? Yes, infrastructure should help with that as well, although we don't have an upstream GEP to define that yet. We may look at setting that globally at a Cilium level instead for now, but I'll put something on #27273 when we have more.

@Cajga
Copy link

Cajga commented Aug 17, 2023

Thanks for your answer. Yes, #27273 is about this use case. Sorry for opening that CFP, as well as writing here but I was not sure it this covers our use case so, I thought to describe it with the Istio implementation in a separate CFP just to make sure it is captured.

@sergeyshevch
Copy link
Contributor

@rauanmayemir Please note that my workaround policy will also cause #27493 to appear in your cluster. I attached updated workaround mutation policy in #27493 issue

@zs-ko
Copy link

zs-ko commented Sep 29, 2023

@youngnick do cilium support GatewayInfrastructure in the Gateway API or is this something that is going to be added in the future?

@sergeyshevch
Copy link
Contributor

@zs-ko We discussed it with @youngnick in the Closed PR below. AFAIK it will be added in the future (After it will be included in the GW API release)

@snovak7
Copy link

snovak7 commented Oct 1, 2023

Well it was added 4days ago, now just to make it official in next iteration, I already see it present in website docs, but no implementation supports that.
https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/gateway_types.go#L188

@rauanmayemir
Copy link
Contributor

'Infrastructure' helps setting LB IP, but there is still no way to set LB service labels and annotations.

@youngnick
Copy link
Contributor

With the merge of kubernetes-sigs/gateway-api#2440, the infrastructure field in Gateway will grow the required fields in the 1.0 release, due out in the next month or so. Once that is out and we have upgraded Cilium to use that version, we'll be able to use those fields to do this.

I'm sorry this has taken a long time, but we are making progress! As to what version of Cilium this will be included in, I'm not sure - I don't think it likely we will get this in for 1.15.0, but depending on how big the rest of the committers think this feature is, I will try to make the argument that this should be backported to 1.15.x once we get it merged into main after the release is cut. But I can't make any promises there, sorry.

@cmcga1125
Copy link
Author

Thanks for the update!

@youngnick youngnick self-assigned this Oct 16, 2023
@youngnick youngnick added kind/bug This is a bug in the Cilium logic. kind/feature This introduces new functionality. and removed kind/bug This is a bug in the Cilium logic. labels Oct 24, 2023
@youngnick
Copy link
Contributor

youngnick commented Nov 28, 2023

Note that with the merging of #29122, support for the infrastructure stanza including label and annotation support has landed in main.

I should also note that these fields are still experimental in Gateway API, so it's not guaranteed that they'll stay the same.

This is a big requirement for a lot of implementations though, so I think big changes are unlikely.

In the event that did happen, we will provide an upgrade path for this config.

@youngnick
Copy link
Contributor

@cmcga1125, I think that should solve the initial issue as well?

@sergeyshevch
Copy link
Contributor

@youngnick Looks like it will cover our case with AWS NLB. Will this merge be included in the next 1.15 release?

@cmcga1125
Copy link
Author

thanks everyone! this will work for the original issues!

@youngnick
Copy link
Contributor

Feature freeze is this week, and it's merged already, so yes, it will be included in 1.15.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/servicemesh GH issues or PRs regarding servicemesh kind/community-report This was reported by a user in the Cilium community, eg via Slack. kind/feature This introduces new functionality. needs/triage This issue requires triaging to establish severity and next steps. sig/agent Cilium agent related.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants