Enhancement to optionally fetch the OIDCAuthIntrospectionURL #571

abellotti · 2020-07-02T17:31:11Z

Enhancement to optionally fetch the OIDCAuthIntrospectionURL from the Provider Metadata URL if not provided to us.

Solves: #512

carbonin

High level review to start. I figure it's easier to get this into the right place in the source, then we can tackle the implementation.

carbonin · 2020-07-02T17:42:43Z

manageiq-operator/pkg/apis/manageiq/v1alpha1/manageiq_types.go

+			if spec.OIDCProviderURL == "" || spec.OIDCClientSecret == "" {
+				errs = append(errs, "HttpdAuthConfig or both OIDCProviderURL and OIDCClientSecret must be provided for openid-connect authentication")
+			} else {
+				// If the OAuth Introspection URL was not specified, let's try to fetch it from the Provider URL


I don't really think this belongs in Validate.

My intention here was to make this act kind of like an AR validation where we wouldn't change the object we're validating and wouldn't take any further action.

I think it would be better to make the introspection URL optional and fetch it if it was not provided. Then if we fail to fetch it at that point we can error out of reconcile.

carbonin · 2020-07-02T17:43:43Z

manageiq-operator/pkg/apis/manageiq/v1alpha1/manageiq_types.go

@@ -283,6 +286,28 @@ func init() {
 	SchemeBuilder.Register(&ManageIQ{}, &ManageIQList{})
 }

+func fetchIntrospectionUrl(providerUrl string) (introspectionUrl string, errMsg string) {


This doesn't belong in the v1alpha1 package. I think this all should go at least into the controller, possibly into the httpd component.

chessbyte · 2020-07-08T20:25:50Z

@abellotti @carbonin let's get this one over the finish line, so we can call ManageIQ/manageiq#19867 complete for Jansa (I removed the lower priority nice-to-haves from the list on that issue).

carbonin · 2020-07-08T20:42:50Z

Alberto and I chatted about the direction this should go last week. That new direction would have conflicted with #572, but since that was merged today we should be able to make more progress here.

Fryguy · 2020-07-08T20:43:59Z

I'm assuming that #572 is jansa/no, though, right? So will it still have issues on backport?

carbonin · 2020-07-08T20:49:05Z

Ah, yeah. Will probably need a separate jansa-only PR as I did with #579 and #580

the Provider Metadata URL if not provided to us.

…on code and into the config map generation code.

abellotti · 2020-07-10T18:22:50Z

/cc @carbonin latest changes on master, I will create a separate PR for Jansa. Thanks for all your help!!

abellotti · 2020-07-10T19:05:15Z

The Jansa PR: #582

carbonin

Can you add a bit to the comment in the ManageIQSpec struct about how we will fetch the introspection URL if it isn't provided?

Few minor comments. Otherwise looks good

carbonin · 2020-07-10T18:59:30Z

manageiq-operator/pkg/helpers/miq-components/httpd.go

@@ -81,7 +84,16 @@ func Ingress(cr *miqv1alpha1.ManageIQ, scheme *runtime.Scheme) (*extensionsv1bet
 	return ingress, f
 }

-func HttpdConfigMap(cr *miqv1alpha1.ManageIQ, scheme *runtime.Scheme) (*corev1.ConfigMap, controllerutil.MutateFn) {
+func HttpdConfigMap(cr *miqv1alpha1.ManageIQ, scheme *runtime.Scheme) (*corev1.ConfigMap, controllerutil.MutateFn, error) {
+


I think we can lose this leading whitespace.

carbonin · 2020-07-10T19:13:44Z

manageiq-operator/pkg/helpers/miq-components/httpd.go

+		if err != nil {
+			return nil, nil, err
+		}
+		(&cr.Spec).OIDCOAuthIntrospectionURL = introspectionURL


You don't need the & and parens here. cr.Spec.OIDCOAuthIntrospectionURL = introspectionURL will do what you want as cr is already a pointer.

I assume you lifted this from Initialize?

If that's the case that's different because we're using an intermediate local to store the spec so we don't need m.Spec everywhere.

carbonin · 2020-07-10T19:15:41Z

manageiq-operator/pkg/helpers/miq-components/httpd.go

+	}
+
+	if result["token_introspection_endpoint"] == nil {
+		return "", fmt.Errorf("%s - token_introspection_endpoint is missing", errMsg)


Maybe "token_introspection_endpoint is missing from response"?
I don't think I would understand what this was saying as is.

- Added comments on the fetching of the introspection URL if not specified. - Removed unnecessary white spaces - Defining introspection URL via cr.Spec... = instead of (&cr.Spec) - Clarified error message for missing token_introspection_endpoint

miq-bot · 2020-07-10T20:30:58Z

Checked commits abellotti/manageiq-pods@f94430f~...aec43e3 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 1 offense detected

**

💣 💥 🔥 🚒 - Linter/Yaml - missing config files

abellotti · 2020-07-10T20:53:28Z

ok @carbonin I think I got 'em.

simaishi · 2020-07-13T20:19:15Z

Backported to jansa via #582

abellotti added enhancement jansa/yes? labels Jul 2, 2020

abellotti requested review from bdunne and carbonin as code owners July 2, 2020 17:31

abellotti assigned carbonin Jul 2, 2020

abellotti force-pushed the auto_fetch_oidc_introspection_endpoint branch from 20eb63e to 5f777be Compare July 2, 2020 17:39

carbonin suggested changes Jul 2, 2020

View reviewed changes

abellotti removed the jansa/yes? label Jul 2, 2020

abellotti added 2 commits July 9, 2020 17:09

Enhancement to optionally fetch the OIDCAuthIntrospectionURL from

f94430f

the Provider Metadata URL if not provided to us.

Moving the Fetching of the OIDC Introspection URL out of the validati…

f5d658d

…on code and into the config map generation code.

abellotti force-pushed the auto_fetch_oidc_introspection_endpoint branch from 5f777be to f5d658d Compare July 10, 2020 18:18

carbonin reviewed Jul 10, 2020

View reviewed changes

abellotti force-pushed the auto_fetch_oidc_introspection_endpoint branch from 78e720a to cbc1236 Compare July 10, 2020 20:15

PR Review updates

aec43e3

- Added comments on the fetching of the introspection URL if not specified. - Removed unnecessary white spaces - Defining introspection URL via cr.Spec... = instead of (&cr.Spec) - Clarified error message for missing token_introspection_endpoint

abellotti force-pushed the auto_fetch_oidc_introspection_endpoint branch from cbc1236 to aec43e3 Compare July 10, 2020 20:29

carbonin approved these changes Jul 13, 2020

View reviewed changes

carbonin merged commit 64a2c27 into ManageIQ:master Jul 13, 2020

gtanzillo added the jansa/conflict label Jul 13, 2020

abellotti mentioned this pull request Jul 13, 2020

[JANSA] Enhancement to optionally fetch the OIDCAuthIntrospectionURL from the Provider #582

Merged

simaishi added jansa/backported and removed jansa/conflict labels Jul 13, 2020

simaishi mentioned this pull request Aug 6, 2020

[JANSA] Fix oidcAuthIntrospectionURL/oidcClientSecret description #609

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement to optionally fetch the OIDCAuthIntrospectionURL #571

Enhancement to optionally fetch the OIDCAuthIntrospectionURL #571

abellotti commented Jul 2, 2020

carbonin left a comment

carbonin Jul 2, 2020

carbonin Jul 2, 2020

chessbyte commented Jul 8, 2020

carbonin commented Jul 8, 2020

Fryguy commented Jul 8, 2020

carbonin commented Jul 8, 2020

abellotti commented Jul 10, 2020

abellotti commented Jul 10, 2020

carbonin left a comment

carbonin Jul 10, 2020

carbonin Jul 10, 2020

carbonin Jul 10, 2020

miq-bot commented Jul 10, 2020

abellotti commented Jul 10, 2020

simaishi commented Jul 13, 2020

Enhancement to optionally fetch the OIDCAuthIntrospectionURL #571

Enhancement to optionally fetch the OIDCAuthIntrospectionURL #571

Conversation

abellotti commented Jul 2, 2020

carbonin left a comment

Choose a reason for hiding this comment

carbonin Jul 2, 2020

Choose a reason for hiding this comment

carbonin Jul 2, 2020

Choose a reason for hiding this comment

chessbyte commented Jul 8, 2020

carbonin commented Jul 8, 2020

Fryguy commented Jul 8, 2020

carbonin commented Jul 8, 2020

abellotti commented Jul 10, 2020

abellotti commented Jul 10, 2020

carbonin left a comment

Choose a reason for hiding this comment

carbonin Jul 10, 2020

Choose a reason for hiding this comment

carbonin Jul 10, 2020

Choose a reason for hiding this comment

carbonin Jul 10, 2020

Choose a reason for hiding this comment

miq-bot commented Jul 10, 2020

abellotti commented Jul 10, 2020

simaishi commented Jul 13, 2020