You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/04/03 00:11:19 UTC
[GitHub] [apisix-ingress-controller] gxthrj opened a new pull request #337: fix: ssl need to be update when secret has been changed
gxthrj opened a new pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337
Please answer these questions before submitting a pull request
- Why submit this pull request?
- [x] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- Related issues
#324
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#discussion_r610995607
##########
File path: test/e2e/ingress/secret.go
##########
@@ -89,61 +159,129 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
tls, err := s.ListApisixTls()
assert.Nil(ginkgo.GinkgoT(), err, "list tls error")
assert.Len(ginkgo.GinkgoT(), tls, 1, "tls number not expect")
+ assert.Equal(ginkgo.GinkgoT(), cert, tls[0].Cert, "tls cert not expect")
+ assert.Equal(ginkgo.GinkgoT(), key_compare, tls[0].Key, "tls key not expect")
+
+ // check DP
+ s.NewAPISIXHttpsClient(host).GET("/ip").WithHeader("Host", host).Expect().Status(http.StatusOK).Body().Raw()
+
+ dialer := &net.Dialer{
+ Timeout: 30 * time.Second,
+ KeepAlive: 30 * time.Second,
+ DualStack: true,
+ }
+
+ http.DefaultTransport.(*http.Transport).DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
Review comment:
I think `DialContext` can also be removed if we already have the correct `ServerName`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-io edited a comment on pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#issuecomment-817065282
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=h1) Report
> Merging [#337](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=desc) (16c3570) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/c6e71803cca5470679ac0433753b361753e38783?el=desc) (c6e7180) will **decrease** coverage by `0.50%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #337 +/- ##
==========================================
- Coverage 43.98% 43.47% -0.51%
==========================================
Files 40 39 -1
Lines 3456 3457 +1
==========================================
- Hits 1520 1503 -17
- Misses 1767 1783 +16
- Partials 169 171 +2
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/ingress/controller/secret.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2luZ3Jlc3MvY29udHJvbGxlci9zZWNyZXQuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/apisix/cluster.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9jbHVzdGVyLmdv) | `28.44% <0.00%> (-5.97%)` | :arrow_down: |
| [pkg/apisix/route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9yb3V0ZS5nbw==) | `44.84% <0.00%> (-1.82%)` | :arrow_down: |
| [test/e2e/e2e.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-dGVzdC9lMmUvZTJlLmdv) | | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=footer). Last update [c6e7180...16c3570](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] gxthrj commented on a change in pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#discussion_r606917416
##########
File path: test/e2e/ingress/secret.go
##########
@@ -144,6 +144,6 @@ RU+QPRECgYB6XW24EI5+w3STbpnc6VoTS+sy9I9abTJPYo9LpCJwfMYc9Tg9Cx2K
tlsUpdate, err := s.ListApisixTls()
assert.Nil(ginkgo.GinkgoT(), err, "list tlsUpdate error")
assert.Len(ginkgo.GinkgoT(), tlsUpdate, 1, "tls number not expect")
- assert.NotEqual(ginkgo.GinkgoT(), KeyInApisix, tlsUpdate[0].Key, "tls key not expect")
+ assert.Equal(ginkgo.GinkgoT(), certUpdate, tlsUpdate[0].Cert, "tls cert not expect")
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] gxthrj commented on a change in pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#discussion_r606917389
##########
File path: test/e2e/ingress/secret.go
##########
@@ -144,6 +144,6 @@ RU+QPRECgYB6XW24EI5+w3STbpnc6VoTS+sy9I9abTJPYo9LpCJwfMYc9Tg9Cx2K
tlsUpdate, err := s.ListApisixTls()
assert.Nil(ginkgo.GinkgoT(), err, "list tlsUpdate error")
assert.Len(ginkgo.GinkgoT(), tlsUpdate, 1, "tls number not expect")
- assert.NotEqual(ginkgo.GinkgoT(), KeyInApisix, tlsUpdate[0].Key, "tls key not expect")
+ assert.Equal(ginkgo.GinkgoT(), certUpdate, tlsUpdate[0].Cert, "tls cert not expect")
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] gxthrj commented on pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
gxthrj commented on pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#issuecomment-813361004
> @gxthrj Now we only verify that data are passed to the DP but the verifications of the use of these certificates are missing.
Yes, you are right, I will add it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] gxthrj commented on a change in pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#discussion_r610996410
##########
File path: test/e2e/ingress/secret.go
##########
@@ -89,61 +159,129 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
tls, err := s.ListApisixTls()
assert.Nil(ginkgo.GinkgoT(), err, "list tls error")
assert.Len(ginkgo.GinkgoT(), tls, 1, "tls number not expect")
+ assert.Equal(ginkgo.GinkgoT(), cert, tls[0].Cert, "tls cert not expect")
+ assert.Equal(ginkgo.GinkgoT(), key_compare, tls[0].Key, "tls key not expect")
+
+ // check DP
+ s.NewAPISIXHttpsClient(host).GET("/ip").WithHeader("Host", host).Expect().Status(http.StatusOK).Body().Raw()
+
+ dialer := &net.Dialer{
+ Timeout: 30 * time.Second,
+ KeepAlive: 30 * time.Second,
+ DualStack: true,
+ }
+
+ http.DefaultTransport.(*http.Transport).DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
Review comment:
Yes, dirty codes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-io commented on pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#issuecomment-817065282
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=h1) Report
> Merging [#337](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=desc) (7a49678) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/80062976ab9bed2c147f1ad7812a003af02e96ff?el=desc) (8006297) will **decrease** coverage by `0.29%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #337 +/- ##
==========================================
- Coverage 43.72% 43.43% -0.30%
==========================================
Files 40 39 -1
Lines 3428 3442 +14
==========================================
- Hits 1499 1495 -4
- Misses 1761 1777 +16
- Partials 168 170 +2
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/ingress/controller/secret.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2luZ3Jlc3MvY29udHJvbGxlci9zZWNyZXQuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/apisix/cluster.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9jbHVzdGVyLmdv) | `28.44% <0.00%> (-5.97%)` | :arrow_down: |
| [test/e2e/e2e.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-dGVzdC9lMmUvZTJlLmdv) | | |
| [pkg/apisix/resource.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9yZXNvdXJjZS5nbw==) | `78.43% <0.00%> (+0.21%)` | :arrow_up: |
| [pkg/apisix/route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9yb3V0ZS5nbw==) | `44.17% <0.00%> (+0.22%)` | :arrow_up: |
| [pkg/apisix/upstream.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC91cHN0cmVhbS5nbw==) | `48.04% <0.00%> (+1.80%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=footer). Last update [8006297...7a49678](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers merged pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
tokers merged pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#issuecomment-813226703
@gxthrj Now we only verify that data are passed to the DP but the verifications of the use of these certificates are missing.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#discussion_r606607639
##########
File path: test/e2e/ingress/secret.go
##########
@@ -144,6 +144,6 @@ RU+QPRECgYB6XW24EI5+w3STbpnc6VoTS+sy9I9abTJPYo9LpCJwfMYc9Tg9Cx2K
tlsUpdate, err := s.ListApisixTls()
assert.Nil(ginkgo.GinkgoT(), err, "list tlsUpdate error")
assert.Len(ginkgo.GinkgoT(), tlsUpdate, 1, "tls number not expect")
- assert.NotEqual(ginkgo.GinkgoT(), KeyInApisix, tlsUpdate[0].Key, "tls key not expect")
+ assert.Equal(ginkgo.GinkgoT(), certUpdate, tlsUpdate[0].Cert, "tls cert not expect")
Review comment:
Both the cert and key should be checked.
##########
File path: test/e2e/ingress/secret.go
##########
@@ -144,6 +144,6 @@ RU+QPRECgYB6XW24EI5+w3STbpnc6VoTS+sy9I9abTJPYo9LpCJwfMYc9Tg9Cx2K
tlsUpdate, err := s.ListApisixTls()
assert.Nil(ginkgo.GinkgoT(), err, "list tlsUpdate error")
assert.Len(ginkgo.GinkgoT(), tlsUpdate, 1, "tls number not expect")
- assert.NotEqual(ginkgo.GinkgoT(), KeyInApisix, tlsUpdate[0].Key, "tls key not expect")
+ assert.Equal(ginkgo.GinkgoT(), certUpdate, tlsUpdate[0].Cert, "tls cert not expect")
Review comment:
We may also add some test cases to cover the ApisixTls delete.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] gxthrj commented on a change in pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#discussion_r610993313
##########
File path: test/e2e/scaffold/scaffold.go
##########
@@ -175,6 +179,17 @@ func (s *Scaffold) NewAPISIXHttpsClient() *httpexpect.Expect {
// accept any certificate; for testing only!
InsecureSkipVerify: true,
},
+ DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
Review comment:
Great, use `ServerName` instead.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-io edited a comment on pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#issuecomment-817065282
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=h1) Report
> Merging [#337](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=desc) (d4ddc85) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/c6e71803cca5470679ac0433753b361753e38783?el=desc) (c6e7180) will **decrease** coverage by `0.50%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #337 +/- ##
==========================================
- Coverage 43.98% 43.47% -0.51%
==========================================
Files 40 39 -1
Lines 3456 3457 +1
==========================================
- Hits 1520 1503 -17
- Misses 1767 1783 +16
- Partials 169 171 +2
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/ingress/controller/secret.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2luZ3Jlc3MvY29udHJvbGxlci9zZWNyZXQuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/apisix/cluster.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9jbHVzdGVyLmdv) | `28.44% <0.00%> (-5.97%)` | :arrow_down: |
| [pkg/apisix/route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9yb3V0ZS5nbw==) | `44.84% <0.00%> (-1.82%)` | :arrow_down: |
| [test/e2e/e2e.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337/diff?src=pr&el=tree#diff-dGVzdC9lMmUvZTJlLmdv) | | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=footer). Last update [c6e7180...d4ddc85](https://codecov.io/gh/apache/apisix-ingress-controller/pull/337?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #337: fix: ssl need to be update when secret has been changed
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #337:
URL: https://github.com/apache/apisix-ingress-controller/pull/337#discussion_r610990417
##########
File path: test/e2e/scaffold/scaffold.go
##########
@@ -175,6 +179,17 @@ func (s *Scaffold) NewAPISIXHttpsClient() *httpexpect.Expect {
// accept any certificate; for testing only!
InsecureSkipVerify: true,
},
+ DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
Review comment:
Why not set the `ServerName` in `tls.Config`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org