Bug 1797107: config/v1/types_proxy: Clarify trustedCA semantics #582
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci-robot
added
the
size/S
wking
force-pushed
the
proxy-trust-details
branch
from
331b7ac to
0bbb031
Compare
Contributor
|
/lgtm |
openshift-ci-robot
added
lgtm
wking
force-pushed
the
proxy-trust-details
branch
from
0bbb031 to
830aa0e
Compare
Contributor
|
/lgtm |
wking
force-pushed
the
proxy-trust-details
branch
from
830aa0e to
1161fae
Compare
openshift-ci-robot
added
size/M
wking
changed the title
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@wking: This pull request references Bugzilla bug 1797107, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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/test-infra repository. |
Logic for merging with the "system" trust store is [1]. We might want to optionally relax that in the future [2], but it's not clear to me what the API change associated with that relaxation would look like. To motivate the "may" for the non-proxy case: * Here's the registry calling for the merged CA bundle to get injected [3] and dropping it into its system store [4]. That means the merged CA bundle will be used for all connections from that registry container, regardless of whether they are intended for the proxy or not (e.g. via a NO_PROXY environment variable [5,6]). * The cluster-version operator loads the proxy transport directly from the Proxy object [7,8]. This is probably a CVO bug, because as the docs I'm touching here say, reading the Proxy's trustedCA directly (like [9]) is frowned on. But regardless, It then appends its own system cert pool locally [10], and uses the resulting transport when retrieving image signatures [11,12] and Cincinnati graphs [13]. It does not use the additional proxy CAs for other HTTPS, e.g. talking to the Kubernetes API. [1]: https://github.com/openshift/cluster-network-operator/blob/4175354bbe5d12273d4f877f64478127e9d2777f/pkg/controller/proxyconfig/validation.go#L95-L109 [2]: https://github.com/openshift/enhancements/blame/14b2d1a262dc8520603eaec1910aabf5f2d0115f/enhancements/proxy/global-cluster-egress-proxy.md#L208-L211 [3]: https://github.com/openshift/cluster-image-registry-operator/blob/75e8e851700add9c847190fb228d2e702b2af2e8/manifests/04-ca-trusted.yaml#L5-L8 [4]: https://github.com/openshift/cluster-image-registry-operator/blob/75e8e851700add9c847190fb228d2e702b2af2e8/manifests/07-operator.yaml#L87-L97 [5]: https://github.com/openshift/cluster-image-registry-operator/blob/75e8e851700add9c847190fb228d2e702b2af2e8/manifests/07-operator.yaml#L8 [6]: https://github.com/openshift/enhancements/blame/14b2d1a262dc8520603eaec1910aabf5f2d0115f/enhancements/proxy/global-cluster-egress-proxy.md#L155-L156 [7]: https://github.com/openshift/cluster-version-operator/blob/27c4671aa0cbfbbf20324cf06145b44ec2bca45e/pkg/cvo/cvo.go#L749-L765 [8]: https://github.com/openshift/cluster-version-operator/blob/27c4671aa0cbfbbf20324cf06145b44ec2bca45e/pkg/cvo/availableupdates.go#L201-L223 [9]: https://github.com/openshift/cluster-version-operator/blob/27c4671aa0cbfbbf20324cf06145b44ec2bca45e/pkg/cvo/availableupdates.go#L219 [10]: https://github.com/openshift/cluster-version-operator/blob/27c4671aa0cbfbbf20324cf06145b44ec2bca45e/pkg/cvo/availableupdates.go#L238 [11]: https://github.com/openshift/cluster-version-operator/blob/27c4671aa0cbfbbf20324cf06145b44ec2bca45e/pkg/cvo/cvo.go#L728-L738 [12]: https://github.com/openshift/cluster-version-operator/blob/27c4671aa0cbfbbf20324cf06145b44ec2bca45e/pkg/verify/verify.go#L277 [13]: https://github.com/openshift/cluster-version-operator/blob/27c4671aa0cbfbbf20324cf06145b44ec2bca45e/pkg/cvo/availableupdates.go#L43-L48
wking
force-pushed
the
proxy-trust-details
branch
from
1161fae to
60d997e
Compare
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@wking: All pull requests linked via external trackers have merged. Bugzilla bug 1797107 has been moved to the MODIFIED state. DetailsIn 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/test-infra repository. |
danwinship
added a commit
to danwinship/cluster-kube-controller-manager-operator
that referenced
this pull request
Mar 2, 2020
In particular, to get openshift/api#585: config: disable IPv6DualStack feature flag so we don't launch kube-controller-manager with that (still-broken) feature Also includes: openshift/api#557: create the IBMCLoudPlatform type for the ingress operator try2 openshift/api#570: Clarify image config doc openshift/api#569: Enable overriding service account issuer for bound tokens openshift/api#527: Add kubebuilder annotations to the network types openshift/api#574: add deprecaction notice for build pipeline strategy openshift/api#582: config/v1/types_proxy: Clarify trustedCA semantics openshift/api#583: Clarify FROM behavior in builds openshift/api#573: Add CRD generator documentation to Readme openshift/api#576: Remove Description from CLI output to improve its display openshift/api#589: Add missing enum validations openshift/api#583: operator/ingress: add dnsrecord type
soltysh
added a commit
to soltysh/cluster-kube-controller-manager-operator
that referenced
this pull request
Mar 4, 2020
In particular, to get openshift/api#585: config: disable IPv6DualStack feature flag so we don't launch kube-controller-manager with that (still-broken) feature Also includes: openshift/api#557: create the IBMCLoudPlatform type for the ingress operator try2 openshift/api#570: Clarify image config doc openshift/api#569: Enable overriding service account issuer for bound tokens openshift/api#527: Add kubebuilder annotations to the network types openshift/api#574: add deprecaction notice for build pipeline strategy openshift/api#582: config/v1/types_proxy: Clarify trustedCA semantics openshift/api#583: Clarify FROM behavior in builds openshift/api#573: Add CRD generator documentation to Readme openshift/api#576: Remove Description from CLI output to improve its display openshift/api#589: Add missing enum validations openshift/api#583: operator/ingress: add dnsrecord type
soltysh
added a commit
to soltysh/cluster-kube-controller-manager-operator
that referenced
this pull request
Mar 6, 2020
In particular, to get openshift/api#585: config: disable IPv6DualStack feature flag so we don't launch kube-controller-manager with that (still-broken) feature Also includes: openshift/api#557: create the IBMCLoudPlatform type for the ingress operator try2 openshift/api#570: Clarify image config doc openshift/api#569: Enable overriding service account issuer for bound tokens openshift/api#527: Add kubebuilder annotations to the network types openshift/api#574: add deprecaction notice for build pipeline strategy openshift/api#582: config/v1/types_proxy: Clarify trustedCA semantics openshift/api#583: Clarify FROM behavior in builds openshift/api#573: Add CRD generator documentation to Readme openshift/api#576: Remove Description from CLI output to improve its display openshift/api#589: Add missing enum validations openshift/api#583: operator/ingress: add dnsrecord type
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Logic for merging with the "system" trust store is here. We might want to optionally relax that in the future, but it's not clear to me what the API change associated with that relaxation would look like.
To motivate the "may" for the non-proxy case:
Here's the registry calling for the merged CA bundle to get injected and dropping it into its system store. That means the merged CA bundle will be used for all connections from that registry container, regardless of whether they are intended for the proxy or not (e.g. via a
NO_PROXYenvironment variable).The cluster-version operator loads the proxy transport directly from the Proxy object. This is probably a CVO bug, because, as the docs I'm touching here say, reading the Proxy's
trustedCAdirectly (like this) is frowned on. But regardless, It then appends its own system cert pool locally, and uses the resulting transport when retrieving image signatures and Cincinnati graphs. It does not use the additional proxy CAs for other HTTPS, e.g. talking to the Kubernetes API.