apache / apisix-ingress-controller

APISIX Ingress Controller for Kubernetes
https://apisix.apache.org/
Apache License 2.0
995 stars 341 forks source link

feat: Plugin configuration at ApisixRoute level instead of repeated for each rule #1496

Open sbrouet opened 1 year ago

sbrouet commented 1 year ago

Hello Team,

ApisixRoute CRD allows defining plugins to use at rule level only, either with plugins or plugin_config_name element. Currently, when 1 ApisixRoute has multiple rules, the plugins configuration must be repeated to each rule. Even though we can define the plugin configuration in an ApisixPluginConfig, we still need to specify the plugin_config_name element for each rule.

Please feel free to comment and propose better approach. Let me know if any information is missing or I did not respect some rule when writing this issue, this is the first one I am opening on the project.

High level proposal

Sample ApisixRoute BEFORE change (current situation)

Using plugins element

The proxy-rewrite plugin configuration must be repeated for each rule

apiVersion: apisix.apache.org/v2
kind: ApisixRoute
metadata:
  name: route-1-httpbin
  namespace: httpbin
spec:
  http:
    - name: rule-1
      # Criteria for a request to match this rule
      match:
        paths:
          - /httpbin/*
        methods:
          - GET
      backends:
        # Name (metadata.name) of target ApisixUpstream object
        - serviceName: httpbin
          servicePort: 80
          resolveGranularity: service
      plugins:
        - name: proxy-rewrite
          enable: true
          config:
            regex_uri: [ "/httpbin/(.*)", "/$1" ]
    - name: rule-2
      # Criteria for a request to match this rule
      match:
        paths:
          - /httpbin/status/*
        methods:
          - POST
      backends:
        # Name (metadata.name) of target ApisixUpstream object
        - serviceName: httpbin
          servicePort: 80
          resolveGranularity: service
      plugins:
        - name: proxy-rewrite
          enable: true
          config:
            regex_uri: [ "/httpbin/(.*)", "/$1" ]

Using plugin_config_name element

The plugin_config_name element must be repeated for each rule

apiVersion: apisix.apache.org/v2
kind: ApisixRoute
metadata:
  name: route-1-httpbin
  namespace: httpbin
spec:
  http:
    - name: rule-1
      # Criteria for a request to match this rule
      match:
        paths:
          - /httpbin/*
        methods:
          - GET
      backends:
        # Name (metadata.name) of target ApisixUpstream object
        - serviceName: httpbin
          servicePort: 80
          resolveGranularity: service
      # Plugin configuration is the name of ApisixPluginConfig object
      plugin_config_name: httpbin-plugin-config
    - name: rule-2
      # Criteria for a request to match this rule
      match:
        paths:
          - /httpbin/status/*
        methods:
          - POST
      backends:
        # Name (metadata.name) of target ApisixUpstream object
        - serviceName: httpbin
          servicePort: 80
          resolveGranularity: service
      # Plugin configuration is the name of ApisixPluginConfig object
      plugin_config_name: httpbin-plugin-config

Sample ApisixRoute AFTER change (current situation)

Using plugins element

apiVersion: apisix.apache.org/v2
kind: ApisixRoute
metadata:
  name: route-1-httpbin
  namespace: httpbin
spec:
  # This plugin configuration should apply to all rules
  plugins:
    - name: proxy-rewrite
      enable: true
      config:
        regex_uri: [ "/httpbin/(.*)", "/$1" ]
  http:
    - name: rule-1
      # Criteria for a request to match this rule
      match:
        paths:
          - /httpbin/*
        methods:
          - GET
      backends:
        # Name (metadata.name) of target ApisixUpstream object
        - serviceName: httpbin
          servicePort: 80
          resolveGranularity: service
      # proxy-rewrite does not need anymore to be defined here, but can be overridden if required
      # Other plugins configurations specific to this rule could be added
    - name: rule-2
      # Criteria for a request to match this rule
      match:
        paths:
          - /httpbin/status/*
        methods:
          - POST
      backends:
        # Name (metadata.name) of target ApisixUpstream object
        - serviceName: httpbin
          servicePort: 80
          resolveGranularity: service
      # proxy-rewrite does not need anymore to be defined here, but can be overridden if required
      # Sample additional plugin configuration which should apply to this rule
      # in addition to the globally defined proxy-rewrite
      plugins:
        - name: request-id
          enable: true

Using plugin_config_name element

apiVersion: apisix.apache.org/v2
kind: ApisixRoute
metadata:
  name: route-1-httpbin
  namespace: httpbin
spec:
  # This plugin configuration should apply to all rules
  plugin_config_name: httpbin-plugin-config
  http:
    - name: rule-1
      # Criteria for a request to match this rule
      match:
        paths:
          - /httpbin/*
        methods:
          - GET
      backends:
        # Name (metadata.name) of target ApisixUpstream object
        - serviceName: httpbin
          servicePort: 80
          resolveGranularity: service
      # proxy-rewrite does not need anymore to be defined here, but can be overridden if required
      # Other plugins configurations specific to this rule could be added
    - name: rule-2
      # Criteria for a request to match this rule
      match:
        paths:
          - /httpbin/status/*
        methods:
          - POST
      backends:
        # Name (metadata.name) of target ApisixUpstream object
        - serviceName: httpbin
          servicePort: 80
          resolveGranularity: service
      # proxy-rewrite does not need anymore to be defined here, but can be overridden if required
      # Sample additional plugin configuration which should apply to this rule
      # in addition to the globally defined proxy-rewrite
      plugin_config_name: request-id-config

Thank you Team for your good work ! Sébastien

tao12345666333 commented 1 year ago

Since we will differentiate layer4 and layer7 by spec.http and spec.stream, and they are very different. I think this field can be changed to spec.http.plugins and spec.http.plugin_config_name, what do you think?

tao12345666333 commented 1 year ago

In addition, I think we also need to describe its priority and what the final result will be when these fields are configured at the same time.

Normally, configurations with a smaller scope should have higher priority.

AlinsRan commented 1 year ago

Since we will differentiate layer4 and layer7 by spec.http and spec.stream, and they are very different. I think this field can be changed to spec.http.plugins and spec.http.plugin_config_name, what do you think?

Our current field is spce.http[].plugins and spec.http[].plugin_config_name.

Our spec.http is an array, rule level.

If this feature is supported, http should be an object, which would make an incompatible change, which would require a V3 API version.

sbrouet commented 1 year ago

Since we will differentiate layer4 and layer7 by spec.http and spec.stream, and they are very different. I think this field can be changed to spec.http.plugins and spec.http.plugin_config_name, what do you think?

Hello, Indeed I see currently plugins can be configured only on http routes (according to ApisixRoute/v2 Reference: https://apisix.apache.org/zh/docs/ingress-controller/references/apisix_route_v2/) So seems reasonnable

sbrouet commented 1 year ago

In addition, I think we also need to describe its priority and what the final result will be when these fields are configured at the same time.

Normally, configurations with a smaller scope should have higher priority.

Hello, Yes I agree. Do you want me to edit proposal ? Do you have an example (from other bug or documentation) I could use to have clear documentation ?

sbrouet commented 1 year ago

If this feature is supported, http should be an object, which would make an incompatible change, which would require a V3 API version.

Hello, That is a good remark. spec.http[].plugin_config_name indeed is a single value I don't know what is best practice in such cases. Versioning (v3) probably is cleaner.

Another - hacky - option would be keeping current spec.http[].plugin_config_name field and adding an optional array named spec.http[].plugin_config_names (with plural). In that case old configurations would be compatible. Priority would need to be defined when both plugin_config_name and plugin_config_names are present.

Thanks !