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

OIDC AuthenticationClass ADR #460

Merged
merged 33 commits into from
Dec 4, 2023
Merged

OIDC AuthenticationClass ADR #460

merged 33 commits into from
Dec 4, 2023

Conversation

fhennig
Copy link
Member

@fhennig fhennig commented Sep 26, 2023

related:

@netlify
Copy link

netlify bot commented Sep 26, 2023

Deploy Preview for stackable-docs ready!

Name Link
🔨 Latest commit 8638844
🔍 Latest deploy log https://app.netlify.com/sites/stackable-docs/deploys/6564be54708cf50008a2a48c
😎 Deploy Preview https://deploy-preview-460--stackable-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sbernauer

This comment was marked as outdated.

@sbernauer

This comment was marked as outdated.

@sbernauer

This comment was marked as outdated.

@maltesander
Copy link
Member

I tried to implement this on trino but could not get kube-rs to generate the correct CRD when flattening an optional enum (its always mandatory). Dont mind the names, just an example.

#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct TrinoAuthenticationClassRef {
    pub authentication_class: String,
    #[serde(flatten)]
    pub secret: Option<TrinoAuthenticationSecret>,
}

#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)]
#[serde(rename_all = "camelCase")]
pub enum TrinoAuthenticationSecret {
    Oidc {
        #[serde(rename = "clientCredentialsSecret")]
        client_credentials_secret: String,
    },
}

generates

                clusterConfig:
                  description: Trino cluster configuration options.
                  properties:
                    authentication:
                      default: []
                      description: Authentication options for Trino.
                      items:
                        oneOf:
                          - required:
                              - oidc
                        properties:
                          authenticationClass:
                            type: string
                          oidc:
                            properties:
                              clientCredentialsSecret:
                                type: string
                            required:
                              - clientCredentialsSecret
                            type: object
                        required:
                          - authenticationClass
                        type: object
                      type: array

Any ideas?

Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Thanks for the ADR updates @Techassi.
I have noted some things down that came to mind.

Copy link
Member

@adwk67 adwk67 left a comment

Choose a reason for hiding this comment

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

Just some minor typos.

modules/contributor/pages/adr/ADR032-oidc-support.adoc Outdated Show resolved Hide resolved
modules/contributor/pages/adr/ADR032-oidc-support.adoc Outdated Show resolved Hide resolved
modules/contributor/pages/adr/ADR032-oidc-support.adoc Outdated Show resolved Hide resolved
modules/contributor/pages/adr/ADR032-oidc-support.adoc Outdated Show resolved Hide resolved
fhennig and others added 4 commits November 21, 2023 09:12
Co-authored-by: Andrew Kenworthy <andrew.kenworthy@stackable.de>
Co-authored-by: Andrew Kenworthy <andrew.kenworthy@stackable.de>
Co-authored-by: Andrew Kenworthy <andrew.kenworthy@stackable.de>
Co-authored-by: Andrew Kenworthy <andrew.kenworthy@stackable.de>
@fhennig
Copy link
Member Author

fhennig commented Nov 21, 2023

Thanks @adwk67 !

razvan
razvan previously approved these changes Nov 22, 2023
Copy link
Member

@razvan razvan left a comment

Choose a reason for hiding this comment

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

lgtm

NickLarsenNZ
NickLarsenNZ previously approved these changes Nov 22, 2023
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Just spotted two things, otherwise ok by me

modules/contributor/pages/adr/ADR032-oidc-support.adoc Outdated Show resolved Hide resolved
modules/contributor/pages/adr/ADR032-oidc-support.adoc Outdated Show resolved Hide resolved
@NickLarsenNZ NickLarsenNZ dismissed stale reviews from razvan and themself via d3138d3 November 22, 2023 14:01
@NickLarsenNZ NickLarsenNZ self-requested a review December 4, 2023 08:07
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Good to go

@Techassi Techassi added this pull request to the merge queue Dec 4, 2023
Merged via the queue into main with commit a69628a Dec 4, 2023
6 checks passed
@Techassi Techassi deleted the adr/oidc branch December 4, 2023 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants