Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change listeners configuration to list to allow for more listeners #3603

Conversation

scholzj
Copy link
Member

@scholzj scholzj commented Sep 4, 2020

Type of change

  • Refactoring

Description

This PR implements the Strimzi Proposal no 5 and brings the possibility to configure listeners as a list and allow to configure more listeners at the same time.

Compared to the proposal, the internal listener type is now internal since service seemed a bit too confusing (all listeners use some services). Since all listeners now use the same API objects, there is now a new class for validation to check all kinds of different parameters and ensure only the compatible fields are used. But this also allows to use a lot of new configuration combinations which were not possible in the past because they were not implemented. That includes for example advertised host and port configuration for all listeners etc. I also merged the override and configuration sections which were confusing and had a big overlap in the past API. Additionally, I added option to use fully qualified service names in internal listeners as requested by some users (has to be enabled - #2656).

There is also a convertor which converts the old API to the new API for backwards compatibility. There are also several methods which take care of using the right names for the old listeners to not change listener names in Kafka or service names in Kube since that would change all addresses etc.

PS: Sorry for the big PR ... I really was trying to find ways how to split it into smaller PRs. But I do not think it was really possible here.

Checklist

  • Write tests
  • Make sure all tests pass
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Update CHANGELOG.md

@scholzj scholzj added this to the 0.20.0 milestone Sep 4, 2020
@scholzj
Copy link
Member Author

scholzj commented Sep 4, 2020

/azp run acceptance

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

public void serialize(ArrayOrObjectKafkaListeners value, JsonGenerator generator, SerializerProvider provider) throws IOException {
if (value != null) {
if (value.listValue != null) {
generator.writeObject(value.listValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We write an object for the list value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is one of the chaotic things where Object here is Java Object and not YAML / JSON Object :-(.

@scholzj
Copy link
Member Author

scholzj commented Sep 4, 2020

/azp run acceptance

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@scholzj scholzj force-pushed the make-it-possible-to-alternate-multiple-types-in-api-schema branch from ffc8c7f to 02b4b0c Compare September 4, 2020 18:01
@scholzj
Copy link
Member Author

scholzj commented Sep 4, 2020

/azp run acceptance

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member Author

scholzj commented Sep 4, 2020

/azp run acceptance

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member Author

scholzj commented Sep 5, 2020

/azp run regression

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work I had a first pass.

@scholzj
Copy link
Member Author

scholzj commented Sep 5, 2020

/azp run acceptance

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member Author

scholzj commented Sep 5, 2020

/azp run regression

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member Author

scholzj commented Sep 6, 2020

@strimzi-ci run tests

@strimzi-ci
Copy link

❌ Test Summary ❌

TEST_PROFILE: acceptance
EXCLUDED_GROUPS: networkpolicies
TEST_CASE: *ST
TOTAL: 50
PASS: 49
FAIL: 1
SKIP: 0
BUILD_NUMBER: 1514
BUILD_ENV: oc cluster up

Re-run command:
@strimzi-ci run tests false profile=acceptance testcase=io.strimzi.systemtest.bridge.HttpBridgeKafkaExternalListenersST#testTlsAuthWithWeirdUsername

@scholzj
Copy link
Member Author

scholzj commented Sep 6, 2020

@strimzi-ci run tests false profile=acceptance testcase=io.strimzi.systemtest.bridge.HttpBridgeKafkaExternalListenersST#testTlsAuthWithWeirdUsername

@strimzi-ci
Copy link

✔️ Test Summary ✔️

TEST_PROFILE: acceptance
EXCLUDED_GROUPS: networkpolicies,flaky
TEST_CASE: io.strimzi.systemtest.bridge.HttpBridgeKafkaExternalListenersST#testTlsAuthWithWeirdUsername
TOTAL: 1
PASS: 1
FAIL: 0
SKIP: 0
BUILD_NUMBER: 1515
BUILD_ENV: oc cluster up

@scholzj
Copy link
Member Author

scholzj commented Sep 6, 2020

@strimzi-ci run tests false profile=acceptance testcase=io.strimzi.systemtest.kafka.BackwardsCompatibleListenersST

@strimzi strimzi deleted a comment from azure-pipelines bot Sep 6, 2020
@scholzj
Copy link
Member Author

scholzj commented Sep 6, 2020

@tombentley @samuel-hawker @ppatierno @Frawless I changed this from Draft to regular PR and I think this should now be ready for the final review. Thanks

Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
Signed-off-by: Jakub Scholz <www@scholzj.com>
@scholzj scholzj force-pushed the make-it-possible-to-alternate-multiple-types-in-api-schema branch from 7250109 to c606555 Compare September 7, 2020 17:11
@scholzj
Copy link
Member Author

scholzj commented Sep 7, 2020

/azp run acceptance

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member Author

scholzj commented Sep 7, 2020

@strimzi-ci run tests

Copy link
Member

@Frawless Frawless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

.azure/regression-pipeline.yaml Show resolved Hide resolved
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Jakub Scholz <www@scholzj.com>
@scholzj
Copy link
Member Author

scholzj commented Sep 7, 2020

/azp run regression

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member Author

scholzj commented Sep 7, 2020

/azp run acceptance

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strimzi-ci
Copy link

✔️ Test Summary ✔️

TEST_PROFILE: acceptance
EXCLUDED_GROUPS: networkpolicies
TEST_CASE: *ST
TOTAL: 47
PASS: 47
FAIL: 0
SKIP: 0
BUILD_NUMBER: 1524
BUILD_ENV: oc cluster up

❗ Test Failures ❗

  • io.strimzi.systemtest.security.oauth.OauthTlsST

@scholzj scholzj merged commit e786ce9 into strimzi:master Sep 8, 2020
@scholzj scholzj deleted the make-it-possible-to-alternate-multiple-types-in-api-schema branch September 8, 2020 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants