You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "FrankYang0529 (via GitHub)" <gi...@apache.org> on 2024/05/06 07:28:24 UTC
[PR] MINOR: enable test for ensureInternalEndpointIsSecured [kafka]
FrankYang0529 opened a new pull request, #15868:
URL: https://github.com/apache/kafka/pull/15868
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: enable test for ensureInternalEndpointIsSecured [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15868:
URL: https://github.com/apache/kafka/pull/15868#discussion_r1592649000
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/SessionedProtocolIntegrationTest.java:
##########
@@ -115,11 +112,11 @@ public void ensureInternalEndpointIsSecured() throws Throwable {
// Try again, but with an invalid signature
log.info(
"Making a POST request to the {} endpoint with no connector started and an invalid signature header; "
- + "expecting 403 error response",
+ + "expecting 503 error response",
connectorTasksEndpoint
);
assertEquals(
- FORBIDDEN.getStatusCode(),
+ SERVICE_UNAVAILABLE.getStatusCode(),
Review Comment:
Could you share the details with me?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16671: enable test for ensureInternalEndpointIsSecured [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #15868:
URL: https://github.com/apache/kafka/pull/15868#discussion_r1597538208
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/SessionedProtocolIntegrationTest.java:
##########
@@ -115,11 +112,11 @@ public void ensureInternalEndpointIsSecured() throws Throwable {
// Try again, but with an invalid signature
log.info(
"Making a POST request to the {} endpoint with no connector started and an invalid signature header; "
- + "expecting 403 error response",
+ + "expecting 503 error response",
connectorTasksEndpoint
);
assertEquals(
- FORBIDDEN.getStatusCode(),
+ SERVICE_UNAVAILABLE.getStatusCode(),
Review Comment:
Added it and `ensureInternalEndpointIsSecured` can pass in CI environment. Thanks.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: enable test for ensureInternalEndpointIsSecured [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #15868:
URL: https://github.com/apache/kafka/pull/15868#discussion_r1592700292
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/SessionedProtocolIntegrationTest.java:
##########
@@ -115,11 +112,11 @@ public void ensureInternalEndpointIsSecured() throws Throwable {
// Try again, but with an invalid signature
log.info(
"Making a POST request to the {} endpoint with no connector started and an invalid signature header; "
- + "expecting 403 error response",
+ + "expecting 503 error response",
connectorTasksEndpoint
);
assertEquals(
- FORBIDDEN.getStatusCode(),
+ SERVICE_UNAVAILABLE.getStatusCode(),
Review Comment:
This was added in https://github.com/apache/kafka/pull/11783.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16671: enable test for ensureInternalEndpointIsSecured [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #15868:
URL: https://github.com/apache/kafka/pull/15868#discussion_r1600286651
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/SessionedProtocolIntegrationTest.java:
##########
@@ -108,8 +110,8 @@ public void ensureInternalEndpointIsSecured() throws Throwable {
connectorTasksEndpoint
);
assertEquals(
- BAD_REQUEST.getStatusCode(),
- connect.requestPost(connectorTasksEndpoint, "[]", emptyHeaders).getStatus()
+ BAD_REQUEST.getStatusCode(),
Review Comment:
Revert it. Thanks.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: enable test for ensureInternalEndpointIsSecured [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #15868:
URL: https://github.com/apache/kafka/pull/15868#discussion_r1593273181
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/SessionedProtocolIntegrationTest.java:
##########
@@ -115,11 +112,11 @@ public void ensureInternalEndpointIsSecured() throws Throwable {
// Try again, but with an invalid signature
log.info(
"Making a POST request to the {} endpoint with no connector started and an invalid signature header; "
- + "expecting 403 error response",
+ + "expecting 503 error response",
connectorTasksEndpoint
);
assertEquals(
- FORBIDDEN.getStatusCode(),
+ SERVICE_UNAVAILABLE.getStatusCode(),
Review Comment:
CI can't pass. Not sure whether there is some connector forgot to close. The error is:
```
java.lang.AssertionError: expected:<503> but was:<403>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:647)
at org.junit.Assert.assertEquals(Assert.java:633)
at org.apache.kafka.connect.integration.SessionedProtocolIntegrationTest.ensureInternalEndpointIsSecured(SessionedProtocolIntegrationTest.java:118)
```
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: enable test for ensureInternalEndpointIsSecured [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #15868:
URL: https://github.com/apache/kafka/pull/15868#issuecomment-2095618948
@FrankYang0529 please fix the build error
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: enable test for ensureInternalEndpointIsSecured [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15868:
URL: https://github.com/apache/kafka/pull/15868#discussion_r1593737692
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/SessionedProtocolIntegrationTest.java:
##########
@@ -115,11 +112,11 @@ public void ensureInternalEndpointIsSecured() throws Throwable {
// Try again, but with an invalid signature
log.info(
"Making a POST request to the {} endpoint with no connector started and an invalid signature header; "
- + "expecting 403 error response",
+ + "expecting 503 error response",
connectorTasksEndpoint
);
assertEquals(
- FORBIDDEN.getStatusCode(),
+ SERVICE_UNAVAILABLE.getStatusCode(),
Review Comment:
`503` is a temporary error, because the worker is still in starting. Could you please add a wait to check the 503 first ?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16671: enable test for ensureInternalEndpointIsSecured [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #15868:
URL: https://github.com/apache/kafka/pull/15868
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: enable test for ensureInternalEndpointIsSecured [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #15868:
URL: https://github.com/apache/kafka/pull/15868#discussion_r1592697934
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/SessionedProtocolIntegrationTest.java:
##########
@@ -115,11 +112,11 @@ public void ensureInternalEndpointIsSecured() throws Throwable {
// Try again, but with an invalid signature
log.info(
"Making a POST request to the {} endpoint with no connector started and an invalid signature header; "
- + "expecting 403 error response",
+ + "expecting 503 error response",
connectorTasksEndpoint
);
assertEquals(
- FORBIDDEN.getStatusCode(),
+ SERVICE_UNAVAILABLE.getStatusCode(),
Review Comment:
Here, if the connector is not started, there is no `sessionKey`.
https://github.com/apache/kafka/blob/05df10449eb9c95fe6d6055b302c84686be8058d/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java#L2748-L2752
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16671: enable test for ensureInternalEndpointIsSecured [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15868:
URL: https://github.com/apache/kafka/pull/15868#discussion_r1600091237
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/SessionedProtocolIntegrationTest.java:
##########
@@ -108,8 +110,8 @@ public void ensureInternalEndpointIsSecured() throws Throwable {
connectorTasksEndpoint
);
assertEquals(
- BAD_REQUEST.getStatusCode(),
- connect.requestPost(connectorTasksEndpoint, "[]", emptyHeaders).getStatus()
+ BAD_REQUEST.getStatusCode(),
Review Comment:
please revert those unrelated changes
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org