fix: ssl use cache #203
fix: ssl use cache #203
Conversation
|
@gxthrj The unit test cases failed. |
pkg/apisix/cluster.go
Outdated
| @@ -190,6 +190,7 @@ func (c *cluster) syncCacheOnce() (bool, error) { | |||
| } | |||
| } | |||
| for _, s := range ssl { | |||
| s.FullName = s.ID | |||
There was a problem hiding this comment.
Why not assigning FullName outside?
There was a problem hiding this comment.
Such as when you convert the v1.SSL.
pkg/apisix/ssl.go
Outdated
| @@ -168,6 +169,8 @@ func (s *sslClient) Create(ctx context.Context, obj *v1.Ssl) (*v1.Ssl, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| ssl.ID = obj.ID | |||
There was a problem hiding this comment.
Why should we re-assign here? APISIX doesn't change id.
pkg/apisix/ssl.go
Outdated
| @@ -188,6 +191,7 @@ func (s *sslClient) Delete(ctx context.Context, obj *v1.Ssl) error { | |||
| if err := s.cluster.deleteResource(ctx, url); err != nil { | |||
| return err | |||
| } | |||
| obj.FullName = obj.ID | |||
There was a problem hiding this comment.
I think we don't need to assign FullName here, since the obj already has a correct FullName when it was fetched from cache or APISIX.
pkg/apisix/ssl.go
Outdated
| @@ -228,10 +232,11 @@ func (s *sslClient) Update(ctx context.Context, obj *v1.Ssl) (*v1.Ssl, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| ssl.ID = obj.ID | |||
test/e2e/ingress/ssl.go
Outdated
| var _ = ginkgo.Describe("SSL Testing", func() { | ||
| s := scaffold.NewDefaultScaffold() | ||
| ginkgo.It("create a SSL from ApisixTls ", func() { | ||
| secretName := "test-atls" |
There was a problem hiding this comment.
change to apisix-tls
| } | ||
| err = cli.AddCluster(&apisix.ClusterOptions{ | ||
| BaseURL: u.String(), | ||
| }) |
There was a problem hiding this comment.
It seems we can prepare an APISIX client in the beforeEach function?
test/e2e/ingress/ssl.go
Outdated
| host := "api6.com" | ||
| err = s.NewApisixTls(tlsName, host, secretName) | ||
| assert.Nil(ginkgo.GinkgoT(), err, "create tls error") | ||
| // update ApisixTls resource |
There was a problem hiding this comment.
I think the update SSL logic can be deleted, since it was check in the last case.
Codecov Report
@@ Coverage Diff @@
## master #203 +/- ##
==========================================
+ Coverage 44.48% 44.61% +0.12%
==========================================
Files 34 34
Lines 2050 2053 +3
==========================================
+ Hits 912 916 +4
+ Misses 995 994 -1
Partials 143 143
Continue to review full report at Codecov.
|
|
@gxthrj Conflict again. |
test/e2e/ingress/resourcepushing.go
Outdated
| err = s.EnsureNumApisixUpstreamsCreated(1) | ||
| assert.Nil(ginkgo.GinkgoT(), err, "Checking number of upstreams") | ||
| assert.Nil(ginkgo.GinkgoT(), s.ScaleHTTPBIN(2), "scaling number of httpbin instancess") | ||
| assert.Nil(ginkgo.GinkgoT(), s.WaitAllHTTPBINPoddsAvailable(), "waiting for all httpbin pods ready") |
There was a problem hiding this comment.
I think it doesn't need to scale the pods here?
test/e2e/ingress/resourcepushing.go
Outdated
| tls, err := s.ListApisixTls() | ||
| assert.Nil(ginkgo.GinkgoT(), err, "list tls error") | ||
| assert.Len(ginkgo.GinkgoT(), tls, 1, "tls number not expect") | ||
| // FIXME |
There was a problem hiding this comment.
What's wrong should be fixed in the FIXME.
There was a problem hiding this comment.
This is a DP test error，will be added in other PR
test/e2e/ingress/resourcepushing.go
Outdated
| `, backendSvc, backendSvcPort[0]) | ||
| fmt.Println(apisixRoute) |
There was a problem hiding this comment.
Should remove this debugging code.
test/e2e/scaffold/apisix.go
Outdated
| @@ -174,6 +200,8 @@ func (s *Scaffold) newAPISIX() (*corev1.Service, error) { | |||
| return nil, err | |||
| } | |||
| if err := k8s.KubectlApplyFromStringE(s.t, s.kubectlOptions, _apisixDeployment); err != nil { | |||
| fmt.Println(_apisixDeployment) | |||
test/e2e/scaffold/apisix.go
Outdated
| return net.JoinHostPort(addr, strconv.Itoa(int(port.NodePort))), nil | ||
| } | ||
| } | ||
| return "", errors.New("no http port in apisix service") |
There was a problem hiding this comment.
typo: should be "no https port defined in apisix service".
pkg/apisix/cluster.go
Outdated
| @@ -310,7 +310,11 @@ func (s *cluster) listResource(ctx context.Context, url string) (*listResponse, | |||
| } | |||
|
|
|||
| func (s *cluster) createResource(ctx context.Context, url string, body io.Reader) (*createResponse, error) { | |||
| req, err := http.NewRequestWithContext(ctx, http.MethodPut, url, body) | |||
| return s.createResourceWithMethod(ctx, url, body, http.MethodPut) | |||
There was a problem hiding this comment.
Why encapsulating it further by s.createResourceWithMethod, I didn't find any other places to call the s.createResourceWithMethod.
There was a problem hiding this comment.
createResourceWithMethod removed
test/e2e/scaffold/apisix.go
Outdated
| return net.JoinHostPort(addr, strconv.Itoa(int(port.NodePort))), nil | ||
| } | ||
| } | ||
| return "", errors.New("no http port defined in apisix service") |
Please answer these questions before submitting a pull request
Why submit this pull request?
Bugfix
New feature provided
Improve performance
Backport patches
Related issues
Bugfix
Fix: bug: ssl synchronization is pathlogical #179