feat: support secret plugin config #1486

An-DJ · 2022-12-04T07:45:09Z

Type of change:

Bugfix
New feature provided
Improve performance
Backport patches

What this PR does / why we need it:

Config in ApisixRoute and ApisixPluginConfig can be stored into and referred from the kubernetes secret.

A echo plugin demo is like below, the before_body and after_body can be stored in their corresponding secret.

apiVersion: v1
kind: Secret
metadata:
  name: echo
data:
  before_body: IlRoaXMgaXMgdGhlIHByZWZhY2Ui
  after_body: IlRoaXMgaXMgdGhlIGVwaWxvZ3VlIg==
---
apiVersion: apisix.apache.org/v2beta3
kind: ApisixRoute
metadata:
 name: httpbin-route
spec:
 http:
 - name: rule1
   match:
     hosts:
     - httpbin.org
     paths:
       - /ip
   backends:
   - serviceName: %s
     servicePort: %d
     weight: 10
   plugins:
   - name: echo
     enable: true
     config:
       headers:
         X-Foo: v1
         X-Foo2: v2
     secretRef: echo

Close #1408

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Config in ApisixRoute and ApisixPluginConfig can be stored into and referred from kubernetes secret Close apache#1408

An-DJ · 2022-12-04T07:47:49Z

@tao12345666333 PTAL

AlinsRan · 2022-12-05T01:46:54Z

pkg/providers/apisix/translation/apisix_pluginconfig.go

@@ -42,6 +42,11 @@ func (t *translator) TranslatePluginConfigV2beta3(config *configv2beta3.ApisixPl
 						zap.Any("new", plugin.Config),
 					)
 				}
+				if sec, err := t.SecretLister.Secrets(config.Namespace).Get(plugin.SecretConfig); err == nil {


I think there should be logs here.

Why are errors being ignored here? There is no way for the user to know why their configuration (although wrong) is being ignored. At least we should print a log or add an error message to the sync status field.

tao12345666333

thanks for your contribution.

There are some things that need to be modified:

SecretConfig semantics are less clear about naming
v2beta3 is deprecated, we don't need to modify it anymore.
The current code and test cases do not cover the case of duplicate configuration.
You didn't add documentation about it. References need to be updated at least https://github.com/apache/apisix-ingress-controller/tree/master/docs/en/latest/references

tao12345666333 · 2022-12-05T01:55:10Z

pkg/kube/apisix/apis/config/v2/types.go

@@ -170,6 +170,8 @@ type ApisixRoutePlugin struct {
 	Enable bool `json:"enable" yaml:"enable"`
 	// Plugin configuration.
 	Config ApisixRoutePluginConfig `json:"config" yaml:"config"`
+	// Plugin configuration secretRef.
+	SecretConfig string `json:"secretConfig" yaml:"secretConfig"`


I prefer it called SecretRef. The semantics are clearer, this is a reference to the secret

It can be left unset, right?

tao12345666333 · 2022-12-05T01:56:05Z

pkg/kube/apisix/apis/config/v2beta3/types.go

@@ -161,6 +161,8 @@ type ApisixRouteHTTPPlugin struct {
 	Enable bool `json:"enable" yaml:"enable"`
 	// Plugin configuration.
 	Config ApisixRouteHTTPPluginConfig `json:"config" yaml:"config"`
+	// Plugin configuration secretRef.
+	SecretConfig string `json:"secretConfig" yaml:"secretConfig"`


v2beta3 is deprecated, we don't need to modify it anymore.

lingsamuel

Also need to update the documentation. And please fix the CI errors.

pkg/kube/apisix/apis/config/v2beta3/types.go

lingsamuel · 2022-12-05T02:12:36Z

pkg/providers/apisix/translation/apisix_pluginconfig.go

@@ -42,6 +42,11 @@ func (t *translator) TranslatePluginConfigV2beta3(config *configv2beta3.ApisixPl
 						zap.Any("new", plugin.Config),
 					)
 				}
+				if sec, err := t.SecretLister.Secrets(config.Namespace).Get(plugin.SecretConfig); err == nil {


Why are errors being ignored here? There is no way for the user to know why their configuration (although wrong) is being ignored. At least we should print a log or add an error message to the sync status field.

lingsamuel · 2022-12-05T02:16:22Z

test/e2e/suite-plugins/suite-plugins-general/echo.go

+			resp.Status(http.StatusOK)
+			resp.Header("X-Foo").Equal("v1")
+			resp.Header("X-Foo2").Equal("v2")
+			resp.Body().Contains("This is the preface")


Any way to check if it actually appears before the later one?

lingsamuel · 2022-12-05T02:20:45Z

pkg/kube/apisix/apis/config/v2beta3/types.go

@@ -161,6 +161,8 @@ type ApisixRouteHTTPPlugin struct {
 	Enable bool `json:"enable" yaml:"enable"`
 	// Plugin configuration.
 	Config ApisixRouteHTTPPluginConfig `json:"config" yaml:"config"`
+	// Plugin configuration secretRef.


We should have a document/field comment that explains how the priority work if this conflicts with the Config fields.

An additional section Config with secretRef in apisix_route.md is added to explain that.

* remove secretRef of plugins.config in v2beta3 * add key logs output * add test case to validate the plugins config priority * add the corresponding doc section Close apache#1408

codecov-commenter · 2022-12-06T12:55:11Z

Codecov Report

Merging #1486 (dd9e470) into master (ed92690) will decrease coverage by 0.20%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1486      +/-   ##
==========================================
- Coverage   41.26%   41.06%   -0.21%     
==========================================
  Files          83       83              
  Lines        7365     7401      +36     
==========================================
  Hits         3039     3039              
- Misses       3972     4008      +36     
  Partials      354      354

Impacted Files	Coverage Δ
...roviders/apisix/translation/apisix_pluginconfig.go	`30.55% <0.00%> (-6.12%)`	⬇️
pkg/providers/apisix/translation/apisix_route.go	`30.48% <0.00%> (-1.03%)`	⬇️
pkg/config/config.go	`64.13% <0.00%> (ø)`
pkg/apisix/cluster.go	`36.12% <0.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Close apache#1408

An-DJ · 2022-12-07T14:06:15Z

Perhaps the unit-test-ci error was caused by network random delay. My local test has passed.

Could the test workflow be retriggered by hand?

tao12345666333 · 2022-12-08T10:19:34Z

OK

feat: support secret plugin config

4e4e700

Config in ApisixRoute and ApisixPluginConfig can be stored into and referred from kubernetes secret Close apache#1408

tao12345666333 added the area/controller label Dec 5, 2022

tao12345666333 requested review from tao12345666333, lingsamuel and AlinsRan December 5, 2022 01:34

AlinsRan reviewed Dec 5, 2022

View reviewed changes

tao12345666333 requested changes Dec 5, 2022

View reviewed changes

lingsamuel requested changes Dec 5, 2022

View reviewed changes

feat: remove secretRef of plugins.config in v2beta3

dd9e470

* remove secretRef of plugins.config in v2beta3 * add key logs output * add test case to validate the plugins config priority * add the corresponding doc section Close apache#1408

An-DJ added 2 commits December 6, 2022 05:58

docs: add a blank line

b223b51

Close apache#1408

docs: add a blank line before yaml

10d708d

Close apache#1408

lingsamuel approved these changes Dec 12, 2022

View reviewed changes

tao12345666333 approved these changes Dec 12, 2022

View reviewed changes

tao12345666333 merged commit 0009b5d into apache:master Dec 12, 2022

tao12345666333 added this to the v1.6.0 milestone Dec 12, 2022

tao12345666333 mentioned this pull request Dec 12, 2022

feat: Allow use of config maps and secrets in plugin configs #1408

Closed

An-DJ deleted the secret-plugin-config branch December 13, 2022 01:00

lpiob mentioned this pull request Mar 12, 2023

feat: Allow ApisixRoute's secretRef to provision nested configuration #1722

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support secret plugin config #1486

feat: support secret plugin config #1486

An-DJ commented Dec 4, 2022 •

edited

An-DJ commented Dec 4, 2022

AlinsRan Dec 5, 2022

tao12345666333 Dec 5, 2022

lingsamuel Dec 5, 2022

An-DJ Dec 6, 2022

tao12345666333 left a comment

tao12345666333 Dec 5, 2022

tao12345666333 Dec 5, 2022

AlinsRan Dec 5, 2022

tao12345666333 Dec 5, 2022

An-DJ Dec 6, 2022

lingsamuel left a comment

lingsamuel Dec 5, 2022

lingsamuel Dec 5, 2022

An-DJ Dec 6, 2022

lingsamuel Dec 5, 2022

An-DJ Dec 6, 2022

codecov-commenter commented Dec 6, 2022

An-DJ commented Dec 7, 2022

tao12345666333 commented Dec 8, 2022

feat: support secret plugin config #1486

feat: support secret plugin config #1486

Conversation

An-DJ commented Dec 4, 2022 • edited

Type of change:

What this PR does / why we need it:

Pre-submission checklist:

An-DJ commented Dec 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tao12345666333 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lingsamuel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 6, 2022

Codecov Report

An-DJ commented Dec 7, 2022

tao12345666333 commented Dec 8, 2022

An-DJ commented Dec 4, 2022 •

edited