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

Allow pandaproxy and schemaregistry to connect to kafka api over tls #7820

Merged
merged 3 commits into from
Dec 21, 2022

Conversation

alenkacz
Copy link
Contributor

@alenkacz alenkacz commented Dec 16, 2022

When kafka api has tls enabled, we don't properly configure pandaproxy and schema registry clients to connect to kafka via these private listeners which meant that it did not work. This PR adds the missing parts of configmap and mounts the missing certificates to make it possible for pp/sr to connect.

For mTLS we need both node cert (if self signed - so that client can verify node cert) and also client certs mounted.

Example redpanda node boostrap config when internal kafka listener has enabled Require Client Auth option:

pandaproxy:
    pandaproxy_api:
        - address: 0.0.0.0
          port: 8082
          name: proxy
pandaproxy_client:
    brokers:
        - address: cluster-0.cluster.local
          port: 123
    broker_tls:
        key_file: /etc/tls/certs/ca/tls.key
        cert_file: /etc/tls/certs/ca/tls.crt
        truststore_file: /etc/tls/certs/ca.crt
        enabled: true
schema_registry:
    schema_registry_api:
        - address: 0.0.0.0
          port: 8081
          name: schema-registry
schema_registry_client:
    brokers:
        - address: cluster-0.cluster.local
          port: 123
    broker_tls:
        key_file: /etc/tls/certs/ca/tls.key
        cert_file: /etc/tls/certs/ca/tls.crt
        truststore_file: /etc/tls/certs/ca.crt
        enabled: true

Fixes #5994

Backports Required

  • none - not a bug fix
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v22.3.x
  • v22.2.x
  • v22.1.x

UX Changes

Release Notes

Bug Fixes

  • Fixes pandaproxy and schemaregistry integration with redpanda when TLS is enabled on internal listener

@alenkacz alenkacz force-pushed the av/schema-proxy-tls branch 6 times, most recently from 06d17f8 to c507712 Compare December 19, 2022 14:09
@alenkacz alenkacz marked this pull request as ready for review December 19, 2022 14:47
@alenkacz alenkacz requested a review from a team as a code owner December 19, 2022 14:47
@alenkacz alenkacz changed the title Av/schema proxy tls Allow pandaproxy and schemaregistry to connect to kafka over tls Dec 19, 2022
@alenkacz alenkacz changed the title Allow pandaproxy and schemaregistry to connect to kafka over tls Allow pandaproxy and schemaregistry to connect to kafka api over tls Dec 19, 2022
Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

I have few question around:

  • How we recognise kafka internal listener self signed certificates in various setup?
  • Are we limiting NodeSecretRef usage with Panda Proxy and Schema Registry?
  • Why private client keys are included in Redpanda volume mounts?

Comment on lines +22 to +24
curl -s -v -X POST "http://cluster-tls-0.cluster-tls.$POD_NAMESPACE.svc.cluster.local:8082/topics/test" \
-H "Content-Type: application/vnd.kafka.json.v2+json" \
-d '{"records":[{"value":"Vectorized"},{"value":"Pandaproxy"},{"value":"JSON Demo"}]}' \
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT:

  1. kafkaapi-client-auth is not what this test is doing. First it creates topic and then we are using panda proxy. Shouldn't this be something like pandaproxy-with-client-auth (or pandaproxy-with-mtls, but we should be consistent)?
  2. The commit message lack of additional context like why we are reusing existing test and renaming it to the name that breaks the convention. Shouldn't we create new test?
  3. We could extend the https://github.com/redpanda-data/redpanda/tree/dev/src/go/k8s/tests/e2e/pandaproxy-produce-consume-tls-client test to reconfigure redpanda to use mTLS. In other words extend already existing test that tests mTLS
  4. Unfortunately, as @nicolaferraro rightly pointed out (Enable per kafka listener sasl #6940 (comment)), curl does not fail if the status code is not 2xx, you may need to check it manually. I agree all tests should be updated, as per @nicolaferraro comment, so we might need to address it in separated PR. Still :) let's not add more problem, please 🙏 The good example can be found ff18ceb6 from Allow using public TLS certificates for Pandaproxy API #6637. The next good example
    command:
    - /bin/sh
    - -c
    - -ex
    args:
    - >
    url=http://decommissioning-0.decommissioning.$NAMESPACE.svc.cluster.local:9644/v1/brokers
    res=$(curl --silent -L $url | tr '{' '\n' | grep node_id | wc -l) &&
    echo $res > /dev/termination-log &&
    if [[ "$res" != "3" ]]; then
    exit 1;
    fi
    and
    apiVersion: v1
    kind: Pod
    metadata:
    labels:
    job-name: wait-for-3-brokers
    status:
    containerStatuses:
    - name: curl
    state:
    terminated:
    message: |
    3
    phase: Succeeded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reused an existing test, I just renamed it. I also found out it does not fail, I thought I adjusted it though but perhaps did not 🤔 I'll fix that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no I think my test works, I've added grep -v error_code which means that it fails if that string is added. I had to do it that way because the proxy returns HTTP 200 but an error code (at least in case of missing tls broker config)

src/go/k8s/tests/e2e/kafkaapi-client-auth/04-assert.yaml Outdated Show resolved Hide resolved
src/go/k8s/pkg/resources/configmap.go Show resolved Hide resolved
src/go/k8s/pkg/resources/configmap.go Show resolved Hide resolved
src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go Outdated Show resolved Hide resolved
src/go/k8s/pkg/resources/configmap.go Outdated Show resolved Hide resolved
@RafalKorepta
Copy link
Contributor

I saw that PR cover letter is not correctly formatted as per https://github.com/redpanda-data/redpanda/actions/runs/3738891086

Unclear
Empty release notes section in the PR body, unspecified sub-section, or other ambiguous content. Refer to [CONTRIBUTING.md](https://github.com/redpanda-data/redpanda/blob/dev/CONTRIBUTING.md#contents-of-release-notes-section) for expected content.

* PR https://github.com/redpanda-data/redpanda/pull/7820 Allow pandaproxy and schemaregistry to connect to kafka api over tls by @alenkacz

@alenkacz alenkacz force-pushed the av/schema-proxy-tls branch 4 times, most recently from 52d7a5c to 828ed66 Compare December 20, 2022 18:39
@alenkacz
Copy link
Contributor Author

The PR now properly checks both node issuer and node cert to see whether they are self signed

RafalKorepta
RafalKorepta previously approved these changes Dec 20, 2022
Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

LGTM

@RafalKorepta RafalKorepta merged commit a4f52df into redpanda-data:dev Dec 21, 2022
@alenkacz
Copy link
Contributor Author

/backport v22.3.x

@vbotbuildovich
Copy link
Collaborator

Failed to run cherry-pick command. I executed the below command:

git cherry-pick -x 4be5d03eb96c5fc22f9830c71cb8049de454b61d 57aa55db4524fbca538351d3b8c1c83ceae96bb1 3d3288aa7224df8297b5994281e5f129cc7fd5af

Workflow run logs.

@BenPope BenPope mentioned this pull request Jan 6, 2023
6 tasks
joejulian pushed a commit to joejulian/redpanda that referenced this pull request Mar 10, 2023
…-proxy-tls

Allow pandaproxy and schemaregistry to connect to kafka api over tls
joejulian pushed a commit to joejulian/redpanda that referenced this pull request Mar 24, 2023
…ma-proxy-tls

Allow pandaproxy and schemaregistry to connect to kafka api over tls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants