-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix(#27): Enable SSL transport options on sources #29
fix(#27): Enable SSL transport options on sources #29
Conversation
@christophd: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
13f6e68
to
f9d1b86
Compare
The integration source needs to enable the SSL client via environment variables and set the path to the injected CA certs and PEM files: | ||
|
||
* CAMEL_KNATIVE_CLIENT_SSL_ENABLED=true | ||
* CAMEL_KNATIVE_CLIENT_SSL_KEY_PATH=/knative-custom-certs/knative-eventing-bundle.pem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/knative-custom-certs/...
can potentially have multiple files https://knative.dev/docs/eventing/features/transport-encryption/#configure-custom-event-sources-to-trust-the-eventing-ca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, this requires more sophisticated configuration then. Let's open a follow-up issue for supporting multiple files, ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
options.setKeyCertOptions(new PemKeyCertOptions().setKeyPath(keyPath).setCertPath(certPath)); | ||
} | ||
|
||
options.setTrustOptions(TrustAllOptions.INSTANCE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this config trusting any CA without using /knative-custom-certs/...
config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far I have only seen keystores being injected into /knative-custom-certs/
. should knative-eventing-bundle.jks
also be used as a truststore then?
I will add a configuration option for truststore path and password and just use TrustAll option as a default when nothing is set, WDYT?
aws-s3-source/src/main/java/dev/knative/eventing/KnativeClientOptions.java
Outdated
Show resolved
Hide resolved
aws-s3-source/src/main/java/dev/knative/eventing/KnativeClientOptions.java
Outdated
Show resolved
Hide resolved
f9d1b86
to
7eb696d
Compare
Ok, I have partially tested this. I;ve copied the but still, I seem to get the following in my reconciler-tests:
|
7eb696d
to
04b2382
Compare
16f1e56
to
9aa1709
Compare
- Adds Http client SSL transport options - Uses options as a Camel bean and configures via environment variables when SSL is enabled - Enables SSL support on all sources
9aa1709
to
302c991
Compare
@christophd thx for update. back to work on Thursday - will take a look then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
tested on branch w/ TimerSource rekt-tests
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christophd, matzew The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes
/kind enhancement
Fixes #27
Release Note
Docs