[GitHub] [apisix-ingress-controller] lingsamuel commented on a diff in pull request #1486: feat: support secret plugin config

[GitBox <gi...@apache.org>]

lingsamuel commented on code in PR #1486:
URL: https://github.com/apache/apisix-ingress-controller/pull/1486#discussion_r1039088293


##########
test/e2e/suite-plugins/suite-plugins-general/echo.go:
##########
@@ -57,6 +57,62 @@ spec:
          X-Foo: v1
          X-Foo2: v2
        
+`, backendSvc, backendPorts[0])
+
+			assert.Nil(ginkgo.GinkgoT(), s.CreateVersionedApisixResource(ar))
+
+			err := s.EnsureNumApisixUpstreamsCreated(1)
+			assert.Nil(ginkgo.GinkgoT(), err, "Checking number of upstreams")
+			err = s.EnsureNumApisixRoutesCreated(1)
+			assert.Nil(ginkgo.GinkgoT(), err, "Checking number of routes")
+
+			resp := s.NewAPISIXClient().GET("/ip").WithHeader("Host", "httpbin.org").Expect()
+			resp.Status(http.StatusOK)
+			resp.Header("X-Foo").Equal("v1")
+			resp.Header("X-Foo2").Equal("v2")
+			resp.Body().Contains("This is the preface")

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



##########
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.

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



##########
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"`

Review Comment:
   Are we still adding features for v2beta3? @tao12345666333 



##########
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 {

Review Comment:
   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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org