-
Couldn't load subscription status.
- Fork 151
Add support for mTLS to GitHub App transport #1160
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
Conversation
6699497 to
545f697
Compare
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.
First pass 👌
41a8320 to
cb7dd16
Compare
|
This is looking good, thanks! Please add docs and amend the commit message to match the PR title 🙏 |
cb7dd16 to
6ed456c
Compare
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.
In the docs we must also mention that if a client cert is present, it will be used (just like we mentioned in the GitRepo docs)
This commit ensures that if GitHub app secret data contains ca.crt then a TLS config with user provided custom ca is used in the underlying HTTP transports. The ca.crt in GitHub App secretRef is ignored if certSecretRef is also provided. Signed-off-by: abhijith-darshan <[email protected]> (chore): keep Makefile in sync with other controllers Signed-off-by: abhijith-darshan <[email protected]> (chore): use proper func naming format Signed-off-by: abhijith-darshan <[email protected]> (chore): revert Makefile changes Signed-off-by: abhijith-darshan <[email protected]> (chore): add get secret helper This commit creates a getSecret helper func which can be used to resolve secret. createNotifier re-uses this helper func to extract and pass secrets down to other methods Signed-off-by: abhijith-darshan <[email protected]> (chore): adds tls test cases Signed-off-by: abhijith-darshan <[email protected]> (chore): remove debug logs Signed-off-by: abhijith-darshan <[email protected]> (chore): adds documentation Signed-off-by: abhijith-darshan <[email protected]> (chore): update docs with mTLS info Signed-off-by: abhijith-darshan <[email protected]>
6ed456c to
4eae0d3
Compare
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! 🚀
notification-controllercurrently generates a TLS config for the underlyingHTTPtransport ifProviderspec contains acertSecretRef.Related to the following issues, notification-controller will also use the TLS config for mTLS GitHub App scenarios as well -
git/github: Add support for mTLS to GitHub App transport pkg#999
Add support for mTLS to GitHub App transport source-controller#1860
Add support for mTLS to GitHub App transport image-automation-controller#947
.spec.certSecretRefis the default choice for extracting theca.crt.If the
ca.crtis present in both.spec.certSecretRefand.spec.secretRefthen.spec.certSecretReftakes the higher precedence.The
ca.crtin.spec.secretRefis only consider if the provider is of one of thegittypes andcertSecretRefis not specified in the spec.example:
TODOS:
ca.crtfor TLS config should be generated from.spec.certSecretRefand if not specified then it should attempt to generate TLS config from.spec.secretRef(only if the provider is one of thegittypes)ca.crtpreference