[EV-5401] Add oidc certificate and configs for queryserver #3599
Conversation
| @@ -382,6 +383,43 @@ func (r *ReconcileAPIServer) Reconcile(ctx context.Context, request reconcile.Re | |||
| // Render the desired objects from the CRD and create or update them. | |||
| reqLogger.V(3).Info("rendering components") | |||
|
|
|||
| var authenticationCR *operatorv1.Authentication | |||
| // Fetch the Authentication spec. If present, we use to configure user authentication. | |||
There was a problem hiding this comment.
| // Fetch the Authentication spec. If present, we use to configure user authentication. | |
| // Fetch the Authentication spec. If present, we use it to configure user authentication. |
| nil, reqLogger) | ||
| return reconcile.Result{}, nil | ||
| } | ||
| trustedBundle.AddCertificates(certificate) |
There was a problem hiding this comment.
Theoretically speaking, trustedBundle can be nil here. I'd suggest creating the bundle after this line:
if installationSpec.Variant == operatorv1.TigeraSecureEnterprise {
+ trustedBundle = certificateManager.CreateTrustedBundle()
Then change the following line:
- trustedBundle = certificateManager.CreateTrustedBundle(prometheusCertificate)
+ trustedBundle.AddCertificates(prometheusCertificate)
There was a problem hiding this comment.
thanks for catching this 👍
ti-afra
force-pushed
the
qs-env
branch
2 times, most recently
from
217e5ca
to
d12fa3b
Compare
|
|
||
| hostnameParts := strings.Split(hostname, ".") | ||
|
|
||
| if len(hostnameParts) == 2 { |
There was a problem hiding this comment.
This will turn an external host "idp.org" into a service of ns=idp and name=org. Is that really desirable?
There was a problem hiding this comment.
it will, but there is not rule saying that your namespace cannot be called "org" 😁
I thought this is not a bad compromise unless we want to query the cluster and check if namespace org already exists or not.
There was a problem hiding this comment.
Given the time sensitivity of the issue I propose we assume that the idp is external and create egress according to that assumption. As a follow-up we can explore adding the right egress rule in the case that it isn't. OIDC type tigera is something that we don't expect anyone uses in the wild, and in the case of cloud, they won't be needing this egress rule as the queryserver won't get any traffic from idp issued bearer tokens.
| // the result will include an egress rules with the urlString passed in: | ||
| // 1. egress rule: egress rule assuming the oidc is external to the cluster | ||
| func GetOIDCEgressRule(urlString string) v3.Rule { | ||
| parsedURL, err := url.Parse(urlString) |
There was a problem hiding this comment.
Do we check already somewhere that this can be parsed? We don't want this panic to ever happen.
There was a problem hiding this comment.
I think this has to be check when initializing the authentication cr values. Let me check it.
ti-afra
force-pushed
the
qs-env
branch
2 times, most recently
from
f1e67d1
to
e0c593b
Compare
| } | ||
|
|
||
| if keyValidatorConfig != nil { | ||
| if _, err := url.Parse(keyValidatorConfig.Issuer()); err != nil { |
There was a problem hiding this comment.
I see now that this check is not necessary.
The constructor for KeyValidatorConfig will create a well known config, which is obtained through making a request to the URL.
This means that we can remove this check again.
| @@ -442,6 +443,13 @@ func (r *ReconcileCompliance) Reconcile(ctx context.Context, request reconcile.R | |||
| return reconcile.Result{}, err | |||
| } | |||
|
|
|||
| if keyValidatorConfig != nil { | |||
| if _, err := url.Parse(keyValidatorConfig.Issuer()); err != nil { | |||
There was a problem hiding this comment.
I see now that this check is not necessary.
The constructor for KeyValidatorConfig will create a well known config, which is obtained through making a request to the URL.
This means that we can remove this check again.
ti-afra
changed the title
Description
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.