search

apache / apisix-helm-chart

Apache APISIX Helm Chart
https://apisix.apache.org/
Apache License 2.0
218 stars 208 forks source link

apisix-ingress-controller Helm Chart features & fixes #704

Open mstefany opened 5 months ago

mstefany commented 5 months ago

We are currently working on implementation of apisix into our EKS cluster. And it seems that Helm Chart is missing some of the features or has some bugs. Instead of opening many issues for you to resolve, I have tried to submit PRs with fixes/improvements.

We are currently installing apisix-ingress-controller Helm Chart using Flux CD, so many things can be workaround using kustomize postRenderer, but having these directly in Helm Chart would make things easier, see:

---
apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  name: ingress-apisix
  namespace: ingress-apisix
spec:
  chart:
    spec:
      chart: [REDACTED]/apisix/apisix-ingress-controller
      reconcileStrategy: ChartVersion
      sourceRef:
        kind: HelmRepository
        name: [REDACTED]
        namespace: flux-system
      version: 0.13.0
  interval: 1h
  postRenderers:
  - kustomize:
      patches:
      - patch: |
          - op: add
            path: /metadata/annotations/reloader.stakater.com~1auto
            value: "true"
          - op: add
            path: /spec/template/spec/containers/1/resources
            value: {"limits": {"cpu": "1", "memory": "1Gi"}, "requests": {"cpu": "100m", "memory": "128Mi"}}
          - op: add
            path: /spec/template/spec/containers/1/securityContext
            value: {"capabilities": {"add": ["NET_BIND_SERVICE"]}}
        target:
          kind: Deployment
          name: apisix-ingress-controller
          version: v1
  values:
  [...]

For this matter, I am submitting a overview of PRs for you to review and potentially merge (I don't mind further improving PRs if needed and if review is provided):

There is one more thing missing - support for enabling/disabling apisix plugins in the configMap (e.g. zipkin is enabled by default, but opentelemetry is not, and we would like to have it vice-versa).

EDIT: Additional PRs:

Revolyssup commented 5 months ago

Thanks for the PRs @mstefany . I have run the CI and will review your PRs

Revolyssup commented 5 months ago

@mstefany Can you also update the Readme in your PRs to fix lint tests? like these ones https://github.com/apache/apisix-helm-chart/actions/runs/7490527809/job/20534429629?pr=697#step:4:54

mstefany commented 5 months ago

@Revolyssup It's updated now, I somehow missed it on this PR.

Revolyssup commented 5 months ago

@mstefany Please fix this one as well https://github.com/apache/apisix-helm-chart/pull/694

mstefany commented 5 months ago

@Revolyssup Done.

mstefany commented 5 months ago

@Revolyssup One more PR to fix Prometheus metrics collection, see: https://github.com/apache/apisix-helm-chart/pull/706

mstefany commented 5 months ago

@Revolyssup One more PR, but this will probably require broader discussion, see: https://github.com/apache/apisix-helm-chart/pull/712

mstefany commented 4 months ago

@Revolyssup Additional PR: https://github.com/apache/apisix-helm-chart/pull/724, seems that loki-logger is not enabled by default while other logging plugins are (and we need loki). This is also related to my previous post/PR so that certain plugins are configurable (observability / logging / ...) since typically only certain are usually needed (opentelemetry & loki in our case, not default zipkin & whatever logging...)

mstefany commented 4 months ago

Additional PR for review related to Server Tokens: https://github.com/apache/apisix-helm-chart/pull/727