Bug 1986810: trust the oauth-server when constructing a client to OpenShift #220
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
stlaz
force-pushed
the
oauth-server-trust
branch
2 times, most recently
from
15cef28
to
4d9f40e
Compare
stlaz
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
stlaz
changed the title
openshift-ci
bot
added
the
bugzilla/severity-high
|
@stlaz: This pull request references Bugzilla bug 1986810, 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. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (liyao@redhat.com), skipping review request. 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/test-infra repository. |
openshift-ci
bot
added
the
bugzilla/valid-bug
providers/openshift/provider.go
Outdated
| return err | ||
| } | ||
|
|
||
| kubeInformersMachineNS := informers.NewSharedInformerFactoryWithOptions( |
There was a problem hiding this comment.
should've probably been MachineConfig. It's the NS with the configs that are maintained by operators, hence the "machine"
| return nil, err | ||
| } | ||
|
|
||
| oauthServerCert, err := p.configMapLister.ConfigMaps("openshift-config-managed").Get("oauth-serving-cert") |
There was a problem hiding this comment.
probably obvious, but this assumes that the client newOpenShiftClient is never ever reused betwen invocations, correct?
There was a problem hiding this comment.
the newOpenShiftClient does in fact promote the reuse of the constructed client (which is cached in the httpClientCache)
There was a problem hiding this comment.
ah, thank you, I wasn't aware. How do we deal with changes/rotation on those certs then?
There was a problem hiding this comment.
I just noticed there was a bug that would cause failure of the reuse, that's now fixed (see cachedKey).
When the certs change, we cache a new client that's then used. Each of the calls to OpenShift usually starts by calling the newOpenshiftClient()
There was a problem hiding this comment.
Each of the calls to OpenShift usually starts by calling the newOpenshiftClient()
ah, combined with the cached http client that makes sense to me now 👍
stlaz
force-pushed
the
oauth-server-trust
branch
from
4d9f40e
to
cfb1c02
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: s-urbaniak, stlaz 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 |
|
@stlaz: All pull requests linked via external trackers have merged: Bugzilla bug 1986810 has been moved to the MODIFIED state. 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/test-infra repository. |
implements the oauth-proxy part of openshift/enhancements#797
WIP because: untested