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

[Doc] Bridge integration doc #1665

Merged
merged 19 commits into from
Jun 11, 2019
Merged

[Doc] Bridge integration doc #1665

merged 19 commits into from
Jun 11, 2019

Conversation

sknot-rh
Copy link
Member

Type of change

  • Documentation

Description

Bridge integration doc. Realted to #1641

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Update/write design documentation in ./design
  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md

@sknot-rh sknot-rh changed the title Bridge integration doc [Doc] Bridge integration doc May 30, 2019
@d-laing
Copy link
Member

d-laing commented May 30, 2019

I produced a local build using AsciiDoctor but cannot see the Kafka Bridge content. It looks like you need to add the assembly-deployment-configuration-kafka-bridge.adoc file to the assembly-deployment-configuration assembly .

Are there any further parent assemblies that need updating?

@sknot-rh
Copy link
Member Author

That is because this PR is dependent on #1641
There are generated some resources from the CRD.
This PR is just docs and is not working without PR mentioned above.

@d-laing
Copy link
Member

d-laing commented May 30, 2019

I understand about the auto-generated content. However, I think you do need to add assembly-deployment-configuration-kafka-bridge.adoc to the assembly-deployment-configuration assembly.

Copy link
Contributor

@PaulRMellor PaulRMellor left a comment

Choose a reason for hiding this comment

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

Looks good. I've left a few suggestions. I have a couple of questions on the Healthchecks and Scheduling sections. The sections may be fine in terms of content required, but I've flagged for checking just in case.

sknot-rh and others added 3 commits May 31, 2019 13:24
Co-Authored-By: PaulRMellor <47596553+PaulRMellor@users.noreply.github.com>
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

I left some comments. I think this is missing two major parts:

  • Exposing the Bridge ... this should suggest to the users to create their own routes, node-ports or load-balancers if needed (and warn them about security consequences of that).
  • Securing the bridge ... could for example suggest to write your own network policies to manage the access to the bridge.

I think these have to be added to this PR. (but the code needs to be finished first so that we can do RC1)

documentation/book/assembly-kafka-bridge-replicas.adoc Outdated Show resolved Hide resolved
documentation/book/assembly-kafka-bridge-replicas.adoc Outdated Show resolved Hide resolved
documentation/book/ref-kafka-bridge-tls.adoc Outdated Show resolved Hide resolved
@PaulRMellor PaulRMellor closed this Jun 3, 2019
@PaulRMellor PaulRMellor reopened this Jun 3, 2019
@PaulRMellor
Copy link
Contributor

I left some comments. I think this is missing two major parts:

  • Exposing the Bridge ... this should suggest to the users to create their own routes, node-ports or load-balancers if needed (and warn them about security consequences of that).
  • Securing the bridge ... could for example suggest to write your own network policies to manage the access to the bridge.

I think these have to be added to this PR. (but the code needs to be finished first so that we can do RC1)

@laidan6000 @stanlyDoge - this new content will be picked up in another PR.

Copy link
Member

@d-laing d-laing left a comment

Choose a reason for hiding this comment

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

Looks good from a docs POV.

@PaulRMellor
Copy link
Contributor

@scholzj @ppatierno - ready to merge?

Kafka Bridge can run multiple nodes.
The number of nodes is defined in the `KafkaBridge` resource.
Running a Kafka Bridge with multiple nodes can provide better availability and scalability.
However, when running Kafka Bridge on {ProductPlatformName} it is not absolutely necessary to run multiple nodes of Kafka Bridge for high availability.
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about this section. Due to our problem with consumers which have to connect to the same bridge instance where the consumer were created, as we mentioned in the past we cannot run the bridge with multiple replicas but having just one and for HA maybe more bridge deployment with just one replica. @scholzj wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Well, having multiple bridge instances with a single replica will not be HA. Because either you have one service handling them all in case you have the same issues with case you have the same issues as with single instance in multiple replicas. Or you use different services in which case you can hardly call it HA because clients will need to reconfigure to fail over.

That said, maybe this section of the docs should do more about describing the problems with multiple replicas than explain around HA.

documentation/book/con-kafka-bridge-authentication.adoc Outdated Show resolved Hide resolved
documentation/book/con-kafka-bridge-authentication.adoc Outdated Show resolved Hide resolved
documentation/book/con-kafka-bridge-authentication.adoc Outdated Show resolved Hide resolved
documentation/book/ref-kafka-bridge-tls.adoc Outdated Show resolved Hide resolved
documentation/book/ref-kafka-bridge-tls.adoc Outdated Show resolved Hide resolved
certificate: ca.crt
- secretName: my-other-secret
certificate: certificate.crt
- secretName: my-secret
Copy link
Member Author

@sknot-rh sknot-rh Jun 7, 2019

Choose a reason for hiding this comment

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

From examples/kafka-connect/kafka-connect.yaml

  tls:
    trustedCertificates:
      - secretName: my-cluster-cluster-ca-cert
        certificate: ca.crt

Are we sure about the indentation?

Copy link
Member

Choose a reason for hiding this comment

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

Both are valid and working. This:

  tls:
    trustedCertificates:
      - secretName: my-cluster-cluster-ca-cert
        certificate: ca.crt

has the same meaning as this:

  tls:
    trustedCertificates:
    - secretName: my-cluster-cluster-ca-cert
      certificate: ca.crt

I usually prefer the first one as I find it more readable. But I think that most editors will format into the second. Either way in both case the secret reference will be in the hierarchy under the trustedCertificates.

@@ -0,0 +1,25 @@
// Module included in the following assemblies:
//
// assembly-kafka-bridge.adoc
Copy link
Member

Choose a reason for hiding this comment

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

@stanlyDoge - Please can you combine the OCP and Kubernetes deployment procedures into a single procedure and apply IF statements. Please ask if you're unsure.

@scholzj scholzj merged commit e00c2eb into master Jun 11, 2019
@scholzj scholzj deleted the bridge-integration-doc branch June 11, 2019 12:24
@scholzj scholzj added this to the 0.12.0 milestone Jun 11, 2019
scholzj pushed a commit that referenced this pull request Jun 11, 2019
* Bridge integration doc

* comments1

* comments

* Apply suggestions from Paul's code review

Co-Authored-By: PaulRMellor <47596553+PaulRMellor@users.noreply.github.com>

* deploying

* typos+forbidden

* review edits js

* Bridge integration doc

* comments1

* comments

* Apply suggestions from Paul's code review

Co-Authored-By: PaulRMellor <47596553+PaulRMellor@users.noreply.github.com>

* deploying

* typos+forbidden

* review edits js

* comments

* kafkabridge ref

* another comments

* review edits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants