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

gRPC Connectors API #3245

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Conversation

twoojoo
Copy link

@twoojoo twoojoo commented Dec 27, 2023

Overview

I added gRPC API support for connectors (CRUD operations)

What this PR does / why we need it

I think it could be useful to have the possibility of handling connectors dynamically without the need to restart Dex.

Special notes for your reviewer

  • added a Config.Parse() function to prevent AdditionalFeatures field from being nil (default = empty slice)
  • added a list of ValidAdditionalFeaturs to prevent invalid features from being provided and making detecting typos easier
  • addtionalFeatures slice is passed down to the API struct, so new features that affect apis can be easily added

Does this PR introduce a user-facing change?

It adds connectors functions and types to the gRPC client, but it shouldn't break anything.

- Implemented additionalFeatures config entry (list)
- Implemented ConnectorsCRUD as additional feature
- Updated examples/config.dev.yaml file 
- Added gRPC API support for connectors CRUD operations
- Missing ConnectorsCRUD in additionalFeatures in config file will cause connectors gRPC functions to return an error
- Connectors configs are of the bytes type in .proto file and are checked for JSON validity when inserting and updating a connector
- All fields (id, name, type, config) are mandatory when inserting a new connector

twoojoo and others added 10 commits December 27, 2023 09:29
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
@twoojoo
Copy link
Author

twoojoo commented Dec 27, 2023

hi @nabokihms, this time it should pass. I tried running that workflow in a separate branch.

Copy link
Member

@nabokihms nabokihms left a comment

Choose a reason for hiding this comment

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

My only concern regarding this PR is that we extend capabilities for existing API servers. So users who can control oauth2 clients can now control connectors also.

@twoojoo
Copy link
Author

twoojoo commented Dec 28, 2023

I think that it won't affect a standard usage of Dex (at least from what I know about it), but I totally understand your concern. Wouldn't increasing the minor version be sufficient as a way to alert developers? After all, it's a backward-compatible change, although it depends on what is meant by "backward compatibility."

@twoojoo
Copy link
Author

twoojoo commented Dec 28, 2023

Could adding an 'allowEditConnectors' (false if missing) in the grpc section of the config file be a solution? Connectors API functions would still be there, but would return an error when called when that config entry is false.

@nabokihms
Copy link
Member

Yeah, something like that will work. I assume it is better to make it the list of features instead of a boolean flag, e.g., additionalFeatures: ["ConnectorsCRUD"].

In the future, we will be able to extend the API server with more features.

Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
Signed-off-by: Giovanni Campeol <giovanni.campeol.95@gmail.com>
@twoojoo
Copy link
Author

twoojoo commented Jan 7, 2024

hi @nabokihms , is there any way I could help move the PR forward? I am available to correct any inaccuracies or to add any missing elements

@nabokihms nabokihms self-requested a review March 12, 2024 09:49
Signed-off-by: Maksim Nabokikh <maksim.nabokikh@flant.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants