Skip to content
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

Add OIDC Federation Settings #479

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

d34dh0r53
Copy link

This templates the OIDC federation settings needed to configure Keystone to perform federation authentication.

@openshift-ci openshift-ci bot requested review from lewisdenny and stuggi October 9, 2024 19:11
api/bases/keystone.openstack.org_keystoneapis.yaml Outdated Show resolved Hide resolved
api/v1beta1/keystoneapi_types.go Outdated Show resolved Hide resolved
Copy link
Contributor

@vakwetu vakwetu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

@vakwetu
Copy link
Contributor

vakwetu commented Nov 14, 2024

Maybe I missed it, but I didn't see any kuttl or functional tests . Just the removal of a bunch of empty lines in keystoneapi_controller_test.go. Is that coming in a separate PR?

@d34dh0r53
Copy link
Author

Yeah, KUTTL and functional testing is coming in a follow-on patch that I'm working on now.

@d34dh0r53 d34dh0r53 force-pushed the template_federation branch 2 times, most recently from 03276e7 to e39d541 Compare November 18, 2024 18:16
api/v1beta1/keystoneapi_types.go Outdated Show resolved Hide resolved
controllers/keystoneapi_controller.go Outdated Show resolved Hide resolved
@d34dh0r53
Copy link
Author

d34dh0r53 commented Dec 3, 2024

Functional tests are failing with

Will run 53 of 53 specs
Running in parallel across 7 processes
•••
�[1m�[38;5;9mGinkgo timed out waiting for all parallel procs to report back�[0m
�[38;5;243mTest suite:�[0m functional (./tests/functional)

This occurs if a parallel process exits before it reports its results to the
Ginkgo CLI.  The CLI will now print out all the stdout/stderr output it's
collected from the running processes.  However you may not see anything useful
in these logs because the individual test processes usually intercept output to
stdout/stderr in order to capture it in the spec reports.

You may want to try rerunning your test suite with
�[38;5;246m--output-interceptor-mode=none�[0m to see additional output here and debug your
suite.
  
�[1mOutput from proc 1:�[0m

�[1mOutput from proc 2:�[0m

�[1mOutput from proc 3:�[0m

�[1mOutput from proc 4:�[0m

�[1mOutput from proc 5:�[0m

�[1mOutput from proc 6:�[0m

�[1mOutput from proc 7:�[0m

** End **�[38;5;9m�[1mginkgo run�[0m �[38;5;9mfailed�[0m
  Failed to combine cover profiles
  open /go/src/github.com/openstack-k8s-operators/operator/tests/functional/cover.out.1: no such file or directory
make: *** [Makefile:129: test] Error 1
{"component":"entrypoint","error":"wrapped process failed: exit status 2","file":"sigs.k8s.io/prow/pkg/entrypoint/run.go:84","func":"sigs.k8s.io/prow/pkg/entrypoint.Options.internalRun","level":"error","msg":"Error executing test process","severity":"error","time":"2024-12-03T03:38:32Z"}
--- 
�[36mINFO�[0m[2024-12-03T03:38:32Z] Reporting job state 'failed' with reason 'executing_graph:step_failed:running_pod' 

I'm not sure how to debug this failure as I haven't even added the test yet. @stuggi any ideas?

@stuggi
Copy link
Contributor

stuggi commented Dec 4, 2024

/retest

@stuggi
Copy link
Contributor

stuggi commented Dec 4, 2024

@d34dh0r53 have you tried to run the functional tests local? good to start with runnign them with e.g. GINKGO_ARGS="--output-interceptor-mode=none" PROCS=1 make test .

@millevy
Copy link

millevy commented Dec 9, 2024

recheck

@d34dh0r53 d34dh0r53 force-pushed the template_federation branch from 2f868a8 to 4a64a83 Compare December 9, 2024 19:09
@d34dh0r53
Copy link
Author

recheck

@d34dh0r53
Copy link
Author

/retest

2 similar comments
@d34dh0r53
Copy link
Author

/retest

@d34dh0r53
Copy link
Author

/retest

Copy link
Contributor

openshift-ci bot commented Dec 17, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: d34dh0r53
Once this PR has been reviewed and has the lgtm label, please ask for approval from stuggi. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment on lines +18 to +24
[openid]
remote_id_attribute={{ .federationRemoteIDAttribute }}

[auth]
methods = password,token,oauth1,mapped,application_credential,openid
{{ end }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to add this using an api parameter? we do not want to add tht parameters via the service operators api. a user could use the native interface with customServiceConfig to customize it.

Comment on lines +219 to 279
type KeystoneFederationSpec struct {
// +kubebuilder:validation:Optional
// +kubebuilder:default="OIDC-"
// OIDCClaimPrefix
OIDCClaimPrefix string `json:"oidcClaimPrefix"`

// +kubebuilder:validation:Optional
// +kubebuilder:default="id_token"
// OIDCResponseType
OIDCResponseType string `json:"oidcResponseType"`

// +kubebuilder:validation:Optional
// +kubebuilder:default="openid email profile"
// OIDCScope
OIDCScope string `json:"oidcScope"`

// +kubebuilder:validation:Optional
// +kubebuilder:default=""
// OIDCProviderMetadataURL
OIDCProviderMetadataURL string `json:"oidcProviderMetadataURL"`

// +kubebuilder:validation:Optional
// +kubebuilder:default=""
// OIDCIntrospectionEndpoint
OIDCIntrospectionEndpoint string `json:"oidcIntrospectionEndpoint"`

// +kubebuilder:validation:Optional
// +kubebuilder:default=""
// OIDCClientID
OIDCClientID string `json:"oidcClientID"`

// +kubebuilder:validation:Optional
// +kubebuilder:default=";"
// OIDCClaimDelimiter
OIDCClaimDelimiter string `json:"oidcClaimDelimiter"`

// +kubebuilder:validation:Optional
// +kubebuilder:default="claims"
// OIDCPassUserInfoAs
OIDCPassUserInfoAs string `json:"oidcPassUserInfoAs"`

// +kubebuilder:validation:Optional
// +kubebuilder:default="both"
// OIDCPassClaimsAs
OIDCPassClaimsAs string `json:"oidcPassClaimsAs"`

// +kubebuilder:validation:Optional
// +kubebuilder:default="memcache"
// OIDCCacheType
OIDCCacheType string `json:"oidcCacheType"`

// +kubebuilder:validation:Optional
// +kubebuilder:default="HTTP_OIDC_ISS"
// RemoteIDAttribute
RemoteIDAttribute string `json:"remoteIDAttribute"`

// +kubebuilder:validation:Optional
// +kubebuilder:default=""
// KeystoneFederationIdentityProviderName
KeystoneFederationIdentityProviderName string `json:"keystoneFederationIdentityProviderName"`
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably should think of a generic config customizion interface for httpd, like we have with customServiceConfig to not require most of these parameters. but right now we do not have it.

@d34dh0r53 d34dh0r53 force-pushed the template_federation branch from ca6fbdc to 44bd8f7 Compare January 7, 2025 14:05
Copy link
Contributor

openshift-ci bot commented Jan 7, 2025

@d34dh0r53: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/functional 48d040f link true /test functional
ci/prow/keystone-operator-build-deploy-kuttl 48d040f link true /test keystone-operator-build-deploy-kuttl

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants