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.

Sign up for GitHub

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

Jump to bottom

Update secret-controller to create OAuth client #177

Merged
merged 1 commit into from
Oct 18, 2022
Merged

Update secret-controller to create OAuth client #177

merged 1 commit into from
Oct 18, 2022

Conversation

VaishnaviHire
Copy link
Member

@VaishnaviHire VaishnaviHire commented Oct 5, 2022 •
edited
Loading

Fixes #175

Description

How Has This Been Tested?

Operator Image: quay.io/vhire/opendatahub-operator:testoauth

  1. Create an example secret with required annotations and plain text
apiVersion: v1
kind: Secret
metadata:
  name: example
  annotations:
    secret-generator.opendatahub.io/name: "password"
    secret-generator.opendatahub.io/type: "random"
    secret-generator.opendatahub.io/complexity: "16"
    secret-generator.opendatahub.io/oauth-client-route: "example-route"
type: Opaque
  1. Create a Route for an example service to be accessed by Oauth client
kind: Route
apiVersion: route.openshift.io/v1
metadata:
  name: example-route
  namespace: test
spec:
  host: >-
    example-route-test.apps.ci-ln-2gzkqh2-76ef8.origin-ci-int-aws.dev.rhcloud.com
  to:
    kind: Service
    name: example
    weight: 100
  port:
    targetPort: 9376

The seceret Controller will generate a secret and an Oauth client

apiVersion: v1
kind: Secret
metadata:
  name: example-generated
data:
  password: jgKGv6grDaLEMo6r
type: Opaque

apiVersion: oauth.openshift.io/v1
grantMethod: auto
kind: OAuthClient
metadata:
  name: example-route
redirectURIs:
- example-route-test.apps.ci-ln-2gzkqh2-76ef8.origin-ci-int-aws.dev.rhcloud.com
secret: <secret-value>

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci openshift-ci bot requested review from anishasthana and LaVLaS October 5, 2022 16:28
@VaishnaviHire VaishnaviHire changed the title [WIP] Update secret-controller to create OAuth client Update secret-controller to create OAuth client Oct 6, 2022
@VaishnaviHire
Copy link
Member Author

Review request - @samuelvl

samuelvl
samuelvl suggested changes Oct 6, 2022
View reviewed changes
pkg/controller/secretgenerator/secret.go Outdated Show resolved Hide resolved
pkg/controller/secretgenerator/secret.go Outdated Show resolved Hide resolved
pkg/controller/secretgenerator/secret.go Outdated Show resolved Hide resolved
pkg/controller/secretgenerator/secret.go Outdated
// Create OauthClient resource
oauthClient := &ocv1.OAuthClient{
TypeMeta: metav1.TypeMeta{},
ObjectMeta: metav1.ObjectMeta{
Copy link
Contributor

Choose a reason for hiding this comment

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

OAuhClient is missing ownerreferences for garbage collection, as done in:

opendatahub-operator/pkg/controller/secretgenerator/secretgenerator_controller.go

Lines 107 to 109 in 10e7bba

OwnerReferences: []metav1.OwnerReference{
*metav1.NewControllerRef(foundSecret, foundSecret.GroupVersionKind()),
},

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated code to delete OauthClient using delete events for secret. As discussed offline, adding OwnerReferences doesn't work because OauthClient is a ClusterScoped resource

pkg/controller/secretgenerator/secret.go Outdated Show resolved Hide resolved
@samuelvl
Copy link
Contributor

samuelvl commented Oct 6, 2022 •
edited
Loading

I am using this kfdef to test the PR with the quay.io/vhire/opendatahub-operator:testoauth image and the OAuthClient is not created:

$ oc get secret oauth-token-generated                                        
NAME                    TYPE     DATA   AGE
oauth-token-generated   Opaque   1      23m

$ oc get route secret-generator        
NAME               HOST/PORT                                                             PATH   SERVICES           PORT   TERMINATION   WILDCARD
secret-generator   secret-generator-opendatahub.apps.mydomain.foo

$ oc get oauthclient secret-generator                    
Error from server (NotFound): oauthclients.oauth.openshift.io "secret-generator" not found

I dont't see any error in the logs.

@VaishnaviHire
Copy link
Member Author

quay.io/vhire/opendatahub-operator:testoauth

It should be created in subsequent reconcile, since the route is created at the same time. Used the provided Kfdef -

$ oc get secret oauth-token-generated -n test
NAME                    TYPE     DATA   AGE
oauth-token-generated   Opaque   1      4m33s

$ oc get route secret-generator -n test
NAME               HOST/PORT                                                                          PATH   SERVICES           PORT   TERMINATION   WILDCARD
secret-generator   secret-generator-test.apps.ci-ln-2gzkqh2-76ef8.origin-ci-int-aws.dev.rhcloud.com          secret-generator   9376                 None

$ oc get oauthclient secret-generator -n test
NAME               SECRET                  WWW-CHALLENGE   TOKEN-MAX-AGE   REDIRECT URIS
secret-generator   oauth-token-generated   false           default         secret-generator-test.apps.ci-ln-2gzkqh2-76ef8.origin-ci-int-aws.dev.rhcloud.com

@samuelvl
Copy link
Contributor

samuelvl commented Oct 6, 2022

@VaishnaviHire I think my problem is I'm using the odh-operator overlay and the operator pod is not running correctly. What are the deployment instructions?

@VaishnaviHire
Copy link
Member Author

@VaishnaviHire I think my problem is I'm using the odh-operator overlay and the operator pod is not running correctly. What are the deployment instructions?

Quickest would be to deploy the operator with Operator Hub and replace the value in CSV
spec.image with quay.io/vhire/opendatahub-operator:testoauth

@VaishnaviHire VaishnaviHire force-pushed the add_oauthclient branch 5 times, most recently from 6824ddf to 863eb5d Compare October 10, 2022 13:15
samuelvl
samuelvl suggested changes Oct 11, 2022
View reviewed changes
pkg/controller/secretgenerator/secretgenerator_controller.go Outdated
Comment on lines 233 to 234
listopts := []client.ListOption{
client.MatchingLabels{secretLabel: secretName}}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can change the createOAuthClient function to avoid using a LIST operation here. Name the OAuthClient object with the original secret name, instead of using the route name:

err = r.createOAuthClient(foundSecret.Name, secret.Value, oauthClientRoute.Spec.Host)

func (r *ReconcileSecretGenerator) createOAuthClient(name string, secret string, uri string) error {
...
}

This way, in the deleteOAuthClient method you can just retrieve the OAuthClient by doing a GET operation (much better performance).

pkg/controller/secretgenerator/secretgenerator_controller.go Outdated

// getRoute returns an OpenShift route object. It waits until the .spec.host value exists to avoid possible race conditions, fails otherwise.
func (r *ReconcileSecretGenerator) getRoute(name string, namespace string) (*routev1.Route, error) {
oauthClientRoute := &routev1.Route{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace s/oauthClientRoute/route to make it more generic (it can return any route, not only the OAuth client route).

pkg/controller/secretgenerator/secretgenerator_controller.go Outdated
}

func (r *ReconcileSecretGenerator) createOAuthClient(secretName string, name string, secret string, uri string) error {
// Create OauthClient resource
Copy link
Contributor

Choose a reason for hiding this comment

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

There are multiple places with this typo, OauthClient should be OAuthClient.

pkg/controller/secretgenerator/secretgenerator_controller.go Outdated Show resolved Hide resolved
samuelvl
samuelvl approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@samuelvl samuelvl left a comment

Choose a reason for hiding this comment

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

Tested in opendatahub-io/odh-dashboard#658

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 11, 2022
@samuelvl
Copy link
Contributor

@LaVLaS Could you approve this PR? I have already reviewed it

@samuelvl samuelvl mentioned this pull request Oct 11, 2022
Merged
3 tasks
@samuelvl
Copy link
Contributor

/retest

1 similar comment
@samuelvl
Copy link
Contributor

/retest

@LaVLaS
Copy link
Contributor

LaVLaS commented Oct 13, 2022

/approve

@openshift-ci
Copy link

openshift-ci bot commented Oct 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LaVLaS, samuelvl

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@LaVLaS
Copy link
Contributor

LaVLaS commented Oct 18, 2022

/retest

@openshift-merge-robot openshift-merge-robot merged commit a523454 into opendatahub-io:master Oct 18, 2022
cfchase pushed a commit to red-hat-data-services/opendatahub-operator that referenced this pull request Oct 25, 2022
@VaishnaviHire @cfchase
[UPSTREAM] Update secret-controller to create OAuth client (opendatah…
71581e1 
…ub-io#177)
VaishnaviHire added a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Feb 2, 2024
@VaishnaviHire
Merge pull request opendatahub-io#177 from VedantMahabaleshwarkar/kse…
d153054 
…rve-alerts

Redefine Kserve Controller Manager scrape config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelvl samuelvl samuelvl approved these changes

@anishasthana anishasthana Awaiting requested review from anishasthana

@LaVLaS LaVLaS Awaiting requested review from LaVLaS

Assignees

@samuelvl samuelvl

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement OAuthClient generation in the SecretGenerator controller
4 participants
@VaishnaviHire @samuelvl @LaVLaS @openshift-merge-robot