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
THREESCALE-10523 - oidc in OpenAPI CR ignores some attributes for Product CR #909
Conversation
97ecf2f
to
185e1ae
Compare
controllers/capabilities/proxy.go
Outdated
@@ -200,6 +200,10 @@ func (t *ProductThreescaleReconciler) syncProxyOIDC(params threescaleapi.Params, | |||
// If plain value is not nil - use plain value as precedence over secret | |||
issuerEndpoint := oidcSpec.IssuerEndpoint | |||
if issuerEndpoint == "" { | |||
if oidcSpec.IssuerEndpointRef == nil { | |||
// If missing both IssuerEndpoint and IssuerEndpointRef in OpenApi CR - Product will fail SyncProxy | |||
return fmt.Errorf("missing IssuerEndpoint definition in OIDC spec in openapi CR. Product OpenID Connect Issuer will not be set.") |
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.
Hey @valerymo - this should be a validationError IMO and the reconciler should not be re-triggered, instead, it should wait for user change to the CR and re-reconcile 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.
hey @MStokluska . This Task was based on 10518, where the fix was done. Now after #915 PR (task 10518) merge - issue resolved and proxy.go file disapered from the list of changes in this PR. Tasks 10523 and 10524 are dependent to 10518 and mostly resolved there (in 10518). The main goal of current task - make the process more clear for user (documentation) Thank you for comment
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.
@valerymo I see, thanks. But please update the error handling as part of this PR since it got pretty bad with the reconciliation, alternatively open another PR. We are about to release so we either need the error fixed or the previous PR reverted.
hey @valerymo in validation scenario no 3, when this error is encountered, Operator is spamming the error very heavily. Instead, IMO the error should be an "InvalidSpec" type error and should not reconcile again until the owner of the CR makes an appropriate change. |
185e1ae
to
73b805f
Compare
73b805f
to
8fb156e
Compare
r.EventRecorder().Eventf(openapiCR, corev1.EventTypeWarning, "OIDC authentication flows in CR will be ignored and Product OIDC authentication flows will be set to match oauth2 flows in OAS since the SecuritySchemes type in OAS is \"oauth2\" (for OIDC it should be \"openIdConnect\")", "%v", "Product OIDC authentication flows parameters will be set to match oauth2 flows as following (OIDC ~ OAuth2): StandardFlowEnabled ~ AuthorizationCode, ImplicitFlowEnabled ~ Implicit, DirectAccessGrantsEnabled ~ Password, ServiceAccountsEnabled ~ ClientCredentials") | ||
} | ||
} | ||
if openapiCR.Spec.OIDC != nil && |
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.
this is duplicated from line 403. Please remove
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.
done. Thank you
@@ -428,6 +428,17 @@ func (r *OpenAPIReconciler) validateOIDCSettingsInCR(openapiCR *capabilitiesv1be | |||
} | |||
} | |||
} | |||
// when OAS securitySchemes type is oauth2, and openapiCR spec is OIDC, then CR OIDC Authentication Flows parameters will be ignored, |
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.
line 423 has ref to openapiref which causes constant updates to status of the CR which in turn re-reconciles on invalid error. Please double check the refs in invalidErrors.
Thanks
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.
8fb156e
to
24c11fb
Compare
doc/openapi-user-guide.md
Outdated
@@ -151,7 +151,7 @@ spec: | |||
- Only for OIDC: | |||
|
|||
| **Field** | **Required** | **Description** | | |||
| --- | --- | --- | | |||
| --- | --- |-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| |
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.
| --- | --- |-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | |
| --- | --- |---| |
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.
Fixed. Thank you. I will check in future such things, seem IDE is doing this.
24c11fb
to
43d3bd0
Compare
/lgtm The e2e will fail most likely due to on-going issue with apisonator. We can wait for the fix which should be available "soon" or merge with failed e2e - which I'm not a fan of. I suggest to wait. |
As for missing forwarding of the spec.oidc.gateway responses and security, they will be resolved as part of new Jira. |
08f8928
to
0e27914
Compare
Code Climate has analyzed commit 0e27914 and detected 0 issues on this pull request. View more on Code Climate. |
THREESCALE-10523 - oidc in OpenAPI CR ignores some attributes for Product CR
WHAT
Jira: https://issues.redhat.com/browse/THREESCALE-10523
PR is based on #908 (jira https://issues.redhat.com/browse/THREESCALE-10518) and will be merged after it.
Validation - Preparation
Install RHSSO
Create project rhsso-test
In RH User SSO web console:
petstore
In RH User SSO web console:
Client ID
: 3scale-zyncClient Settings:
3scale-zync Client setting will be as in the table below
Service Account Roles
tab -> Client Rolesrealm-management
->manage-clients
Install 3scale
please place your wildcardDomain
This is the secret that contains URL for issuerEndpoint.
The secret is referenced in OpenApi CR - field issuerEndpointRef.
Openapi Secret
Apply Secret that contains OAS (swagger) - OpenApi spec:
oc apply -f openapi-secret-oauth2.yaml
NOTES openapi-secret-oauth2 Secret contains
securitySchemes
with all authentication flows defined; this should result in all products OIDC authentication flows being selected. OpenAPI CR OIDC Authentication flows (spec/oidc/authenticationFlow) will be ignored.OpenApi CR
Validation
Test1
OpenAPI CR spec: OIDC
OAS securitySchemes type: oauth2
openapi-secret-oauth2
Secret:Expected behavior:
Notes: As noted before -
openapi-secret-oauth2
Secret containssecuritySchemes
with all authentication flows defined.Expected result:
Test2 - Regression
These are files below.
New Product will be created
Swagger Petstore 2
, that has OIDC Authentication flows as defined in CR.OpenApi CR
Openapi Secret, contains OAS
You can compare the swagger below with oauth2, used for Test1. Small number of diffrences - name of the product, but the main thing is secutirySchema type is openIdConnect (it was oauth2 before). Missing flows definitions, it will be taken from CR.
Test3 - Regression
Check that OpenApi must have oidc spec if Oauth2 or OIDC defined in OAS.
It will be enough to test just oauth2.