apisix-helm-chart icon indicating copy to clipboard operation
apisix-helm-chart copied to clipboard

A more flexible apisix.serviceNamespace for ingress chart

Open NMichas opened this issue 2 years ago • 9 comments

Hello,

Currently the values.yaml of apisix-ingress-controller includes config.apisix.serviceNamespace=ingress-apisix. Users can override this during installation by passing --set config.apisix.serviceNamespace.

Wouldn't it be a little more flexible having config.apisix.serviceNamespace by default using the currently selected kubectl/release namespace and only if it is explicitly set by the user then to point to a different namespace?

The above suggestion only requires two small, backwards-compatible changes in configmap.yaml and deployment.yaml files, for which I could send a PR if you want.

Somewhat relevant issues: #176 #206

NMichas avatar May 20 '22 10:05 NMichas

by default using the currently selected kubectl/release namespace

You mean the namespace that when users run the helm command?

tokers avatar May 23 '22 01:05 tokers

I think this is valuable so that users can install APISIX and APISIX Ingress in the same namepace without specifying additional parameters.

https://github.com/apache/apisix-helm-chart/pull/284 There is a modification, but only the Release namespace is used. It does not solve all scenarios https://github.com/apache/apisix-helm-chart/pull/284#issuecomment-1126648297

Adding compatible configuration is very good, welcome to submit PR

tao12345666333 avatar May 23 '22 02:05 tao12345666333

@tokers yes, indeed.

@tao12345666333 exactly, in my case, I'm creating a meta-chart for our stack which includes APISIX. I'd like my users to be able to do a Helm install of our stack without having to specify additional parameters for the third-party dependencies of our chart.

Here's my PR for review: https://github.com/apache/apisix-helm-chart/pull/292

NMichas avatar May 23 '22 07:05 NMichas

@tokers yes, indeed.

@tao12345666333 exactly, in my case, I'm creating a meta-chart for our stack which includes APISIX. I'd like my users to be able to do a Helm install of our stack without having to specify additional parameters for the third-party dependencies of our chart.

Here's my PR for review: #292

Thanks! After all CI passed, we will review :)

BTW @NMichas, except for GitHub, we also have the #apisix Slack channel with 600+ members under the Apache Software Foundation's workspace. Welcome to join in via https://apisix.apache.org/docs/general/join/ Most of Apache APISIX users and maintainers are here.

juzhiyuan avatar May 23 '22 08:05 juzhiyuan

Thanks, I've already joined you on Slack :)

NMichas avatar May 23 '22 08:05 NMichas

@tokers I tested a bit more and realised serviceName exhibits a similar behaviour. I'm now testing an updated PR taking into account both serviceName and serviceNamespace. I shall be able to submit it in the next few hours, so you can discard my previous PR for the time being.

NMichas avatar May 23 '22 12:05 NMichas

So, PR is now updated:

  • Ingress Ingress chart can now be installed in the same namespace as the admin service without having to pass --set parameters to Helm. In addition, ingress' URL of the admin service can be created automatically if missing, prefixed with the release name. From values.yaml:

    apisix:
        # The name of the APISIX admin service. Leave it empty to use <Release name>-apisix-admin.
        serviceName: ""
        # The namespace of APISIX admin service. Leave it empty to use the release namespace.
        serviceNamespace: ""
    
  • Dashboard The URL of etcd is now automatically prefixed with the release name, when left empty. This allows the dashboard to be installed into a custom namespace without having to pass --set parameters to Helm. From values.yaml:

    config:
      conf:
        ...
        etcd:
          # Supports defining multiple etcd host addresses for an etcd cluster.
          # Leave it undefined to use <Release name>-etcd:2379 as an endpoint.
          # endpoints:
          #   - apisix-etcd:2379
    

The above PR allows you to:

  • Install admin with dashboard and/or ingress in one go on the same namespace using any Helm release name (i.e. helm install myapp https://charts.apiseven.com --set dashboard.enabled=true --set ingress-controller.enabled=true should now work anywhere).
  • Create a meta-chart for your own project having all three APISIX charts as dependencies, without having to specify any additional configuration for the namespace or the Helm release name (that's my use case).
  • If you wish so, you can still manually specify apisix.serviceName, apisix.serviceNamespace, and config.conf.etcd.endpoints via --set; in that case the behaviour of this PR's charts is the same as before this PR (in fact, manual configuration is still necessary if you decide to install the three charts individually).

Please let me know if you see anything wrong and I'm happy to modify it.

NMichas avatar May 23 '22 16:05 NMichas

SGTM

tao12345666333 avatar May 23 '22 17:05 tao12345666333

So, PR is now updated:

  • Ingress Ingress chart can now be installed in the same namespace as the admin service without having to pass --set parameters to Helm. In addition, ingress' URL of the admin service can be created automatically if missing, prefixed with the release name. From values.yaml:
    apisix:
        # The name of the APISIX admin service. Leave it empty to use <Release name>-apisix-admin.
        serviceName: ""
        # The namespace of APISIX admin service. Leave it empty to use the release namespace.
        serviceNamespace: ""
    
  • Dashboard The URL of etcd is now automatically prefixed with the release name, when left empty. This allows the dashboard to be installed into a custom namespace without having to pass --set parameters to Helm. From values.yaml:
    config:
      conf:
        ...
        etcd:
          # Supports defining multiple etcd host addresses for an etcd cluster.
          # Leave it undefined to use <Release name>-etcd:2379 as an endpoint.
          # endpoints:
          #   - apisix-etcd:2379
    

The above PR allows you to:

  • Install admin with dashboard and/or ingress in one go on the same namespace using any Helm release name (i.e. helm install myapp https://charts.apiseven.com --set dashboard.enabled=true --set ingress-controller.enabled=true should now work anywhere).
  • Create a meta-chart for your own project having all three APISIX charts as dependencies, without having to specify any additional configuration for the namespace or the Helm release name (that's my use case).
  • If you wish so, you can still manually specify apisix.serviceName, apisix.serviceNamespace, and config.conf.etcd.endpoints via --set; in that case the behaviour of this PR's charts is the same as before this PR (in fact, manual configuration is still necessary if you decide to install the three charts individually).

Please let me know if you see anything wrong and I'm happy to modify it.

LGTM

tokers avatar May 24 '22 01:05 tokers