CNTRLPLANE-367: Update Authentication API GoDoc for OIDC-related fields #2314

everettraven · 2025-05-07T19:32:46Z

I had noticed that the GoDoc for the OIDC-related fields was a bit sparse and needed to be brought up to date with the OpenShift API conventions.

This PR attempts to improve the GoDoc for only the OIDC-related fields as part of the GA criteria for the BYO OIDC feature.

openshift-ci · 2025-05-07T19:32:56Z

Hello @everettraven! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

openshift-ci-robot · 2025-05-07T19:37:50Z

@everettraven: This pull request references CNTRLPLANE-367 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set.

In response to this:

I had noticed that the GoDoc for the OIDC-related fields was a bit sparse and needed to be brought up to date with the OpenShift API conventions.

This PR attempts to improve the GoDoc for only the OIDC-related fields as part of the GA criteria for the BYO OIDC feature.

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 openshift-eng/jira-lifecycle-plugin repository.

config/v1/types_authentication.go

liouk · 2025-05-08T14:39:05Z

config/v1/types_authentication.go

+	// name configures the unique human-readable identifier
+	// associated with the identity provider.
+	// name is used to distinguish between multiple identity providers.
+	// name has no impact on token validation or authentication mechanics.
+	// name is required.
+	// name must not be an empty string ("").


Instead of repeating name ... in the beginning of every sentence, would it make sense to make it a bit more natural, such as:

Suggested change

// name configures the unique human-readable identifier

// associated with the identity provider.

// name is used to distinguish between multiple identity providers.

// name has no impact on token validation or authentication mechanics.

// name is required.

// name must not be an empty string ("").

// Name configures the unique human-readable identifier

// associated with the identity provider.

// Used to distinguish between multiple identity providers and

// has no impact on token validation or authentication mechanics.

// It is required and must not be an empty string ("").

Also, we should probably match case in the doc comments.

This also seems to agree with our API conventions. This applies to various places in this diff.

Yeah, looking back at this I went a little overboard with the repetition there 😅 - updating.

Also, we should probably match case in the doc comments.

It's API convention to match the serialized name, not the field name in the Go type

For additional context on the serialized name matching, see: #2314 (comment)

liouk · 2025-05-08T14:41:51Z

config/v1/types_authentication.go

+	// issuer configures how the platform interacts
+	// with the identity provider and how tokens issued
+	// from the identity provider are evaluated by the
+	// Kubernetes API Server.


This looks like a more suitable comment for the type TokenIssuer instead of the field.

Comments on top level types do not get included in the generated OpenAPI spec field documentation that is shown to end users for fields of that type. This comment is here so that it appears for the end-user and explains what this field is used for if they run something like oc explain ...

liouk · 2025-05-08T14:43:12Z

config/v1/types_authentication.go

 	ClaimValidationRules []TokenClaimValidationRule `json:"claimValidationRules,omitempty"`
 }

 // +kubebuilder:validation:MinLength=1
 type TokenAudience string

 type TokenIssuer struct {
-	// URL is the serving URL of the token issuer.
-	// Must use the https:// scheme.
+	// issuerURL configures the URL that is used to issue tokens


Wrong field name.

issuerURL is the serialized name which we prefer to use for GoDoc on the OpenShift APIs because it is what end-users will be familiar with. The serialized field name is what the documentation, generated by the GoDoc, will be attached to when users do something like oc explain ... on the field

liouk · 2025-05-08T14:44:00Z

config/v1/types_authentication.go

-	// configuration namespace. The .data of the configMap must contain
-	// the "ca-bundle.crt" key.
-	// If unset, system trust is used instead.
+	// issuerCertificateAuthority configures the certificate authority,


Wrong field name here as well.

See explanation here: #2314 (comment)

liouk · 2025-05-08T14:46:15Z

config/v1/types_authentication.go

 	//
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=63
 	// +required
 	ComponentNamespace string `json:"componentNamespace"`

-	// clientID is the identifier of the OIDC client from the OIDC provider
+	// clientID configures the identifier, from the identity provider, that the platform


Suggested change

// clientID configures the identifier, from the identity provider, that the platform

// clientID configures the client identifier, from the identity provider, that the platform

bergerhoffer

Just a bunch of small nits, nice job!

bergerhoffer · 2025-05-08T19:28:42Z

config/v1/types_authentication.go

@@ -195,74 +195,109 @@ const (
 )

 type OIDCProvider struct {
-	// name of the OIDC provider
+	// name is a required field that configures the unique human-readable identifier


Should all of these references to the field name be capitalized throughout? Per the guidelines you linked to in slack, it seems like the examples there are capitalizing.

I poked around the API docs now and it is a little mixed in that some do lowercase, but quite a lot of them capitalized

Suggested change

// name is a required field that configures the unique human-readable identifier

// Name is a required field that configures the unique human-readable identifier

I can see why an argument could be made for keeping them lowercase, I think it'd be better to be consistent (especially if that's how the guidelines shows them). And for what it's worth, it looks better when looking at the actual docs if they're capitalized (like here), as opposed to lowercase (like here). And I'm sure it looks better when getting the details via the CLI too.

I think the convention used to be capitalizing the field names because that is what is in the Kubernetes API Conventions upstream, but we have an explicit deviation from that here: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#use-json-field-names-in-godoc

I think some of the examples in the conventions doc could use some updating - I can take an action item to do that to reduce confusion.

I'm happy to have a more nuanced discussion on whether or not that is the right approach, but I'd prefer if we moved that discussion to a different avenue than this PR - mostly because a discussion like that is beyond the scope of this PR

bergerhoffer · 2025-05-08T19:32:13Z

config/v1/types_authentication.go

+	// When not specified, the system trust is used.
+	//
+	// When specified, it must reference a ConfigMap in the openshift-config
+	// namespace containing the PEM encoded CA certificates under the 'ca-bundle.crt'


Suggested change

// namespace containing the PEM encoded CA certificates under the 'ca-bundle.crt'

// namespace containing the PEM-encoded CA certificates under the 'ca-bundle.crt'

bergerhoffer · 2025-05-08T19:35:13Z

config/v1/types_authentication.go

+	// by the identity provider.
+	// When referencing a claim, if the claim is present in the JWT
+	// token, its value must be list of groups separated by a comma (',').
+	// For example - '"foo"' and '"foo", "bar", "baz"' are valid claim values.


Any chance we could update these to use different values than 'foo' and 'bar'? Per the style guidelines we follow, we try to avoid having them in the docs, and if there are other example values that could get the same thing across, that would be better.

We usually go with things like "example1", "example2", and so on.

TIL - Will make an update

bergerhoffer · 2025-05-08T19:36:11Z

config/v1/types_authentication.go

 	//
-	// By default, claims other than `email` will be prefixed with the issuer URL to
-	// prevent naming clashes with other plugins.
+	// When set to Prefix, the value specifed in the prefix field will be


Single or double quotes around the literal values. Not sure if one is preferred over the other

Suggested change

// When set to Prefix, the value specifed in the prefix field will be

// When set to 'Prefix', the value specified in the prefix field will be

bergerhoffer · 2025-05-08T19:40:33Z

config/v1/types_authentication.go

 	//
-	// By default, no prefixing occurs.
+	// When omitted, no prefix is applied to the cluster identity attribute.
 	//
 	// Example: if `prefix` is set to "myoidc:"" and the `claim` in JWT contains


I know this was already here, but it looks like there is an extra quote here. Should it be this?

Suggested change

// Example: if `prefix` is set to "myoidc:"" and the `claim` in JWT contains

// Example: if `prefix` is set to "myoidc:" and the `claim` in JWT contains

Ah yeah, I missed that - will update. Thanks!

bergerhoffer · 2025-05-08T21:55:53Z

config/v1/types_authentication.go

+	// prefixPolicy is an optional field that configures how a prefix should be
+	// applied to the value of the JWT claim specified in the 'claim' field.
+	//
+	// Allowed values are Prefix, NoPrefix, and omitted.


Is "omitted" a literal value, or is that supposed to mean that it can be empty? I'd clarify - maybe by putting quotes around the literal allowed values

bergerhoffer · 2025-05-08T21:56:43Z

config/v1/types_authentication.go

-	// prevent naming clashes with other plugins.
+	// When set to Prefix, the value specifed in the prefix field will be
+	// prepended to the value of the JWT claim.
+	// The prefix field must be set when prefixPolicy is Prefix.


Suggested change

// The prefix field must be set when prefixPolicy is Prefix.

// The prefix field must be set when prefixPolicy is 'Prefix'.

bergerhoffer · 2025-05-08T21:56:52Z

config/v1/types_authentication.go

 	//
-	// Set to "NoPrefix" to disable prefixing.
+	// When set to NoPrefix, no prefix will be prepended to the value


Suggested change

// When set to NoPrefix, no prefix will be prepended to the value

// When set to 'NoPrefix', no prefix will be prepended to the value

bergerhoffer · 2025-05-08T21:57:08Z

config/v1/types_authentication.go

-	//         (a) "username": the mapped value will be "https://myoidc.tld#userA"
-	//         (b) "email": the mapped value will be "[email protected]"
+	// When omitted, this means no opinion and the platform is left to choose
+	// any prefixes that are applied - which may be subject to change over time.


Suggested change

// any prefixes that are applied - which may be subject to change over time.

// any prefixes that are applied - which might be subject to change over time.

bergerhoffer · 2025-05-08T21:59:02Z

config/v1/types_authentication.go

+	//
+	// Allowed values are RequiredClaim and omitted.
+	//
+	// When set to RequiredClaim, the Kubernetes API Server


I didn't point out every instance, but I'd do a quick check that literal values have some quotes

Suggested change

// When set to RequiredClaim, the Kubernetes API Server

// When set to 'RequiredClaim', the Kubernetes API Server

JoelSpeed · 2025-05-13T09:00:30Z

config/v1/types_authentication.go

 	//
 	// +kubebuilder:validation:Enum={"", "NoPrefix", "Prefix"}
+	// +optional
+	// +unionDiscriminator


Add +union to the struct

Should be done in the latest push

everettraven · 2025-05-13T15:32:25Z

Botched the rebase 🤦

/hold

Signed-off-by: Bryce Palmer <[email protected]>

everettraven · 2025-05-13T15:35:33Z

Rebase issues fixed

/hold cancel

JoelSpeed · 2025-05-13T16:16:33Z

/lgtm

openshift-ci · 2025-05-13T16:18:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, JoelSpeed

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:

~~OWNERS~~ [JoelSpeed]

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

openshift-ci-robot · 2025-05-13T21:45:16Z

/retest-required

Remaining retests: 0 against base HEAD 9052dea and 2 for PR HEAD fc7af95 in total

openshift-ci-robot · 2025-05-14T08:45:13Z

/retest-required

Remaining retests: 0 against base HEAD 0c841e7 and 1 for PR HEAD fc7af95 in total

openshift-ci · 2025-05-14T14:03:26Z

@everettraven: The following test 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/okd-scos-e2e-aws-ovn	`fc7af95`	link	false	`/test okd-scos-e2e-aws-ovn`

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.

openshift-ci-robot · 2025-05-14T14:22:07Z

/retest-required

Remaining retests: 0 against base HEAD 0c841e7 and 2 for PR HEAD fc7af95 in total

openshift-bot · 2025-05-15T11:24:43Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-config-api
This PR has been included in build ose-cluster-config-api-container-v4.20.0-202505151012.p0.g6a97aca.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 7, 2025

openshift-ci bot requested review from deads2k and JoelSpeed May 7, 2025 19:33

everettraven changed the title ~~Update Authentication API GoDoc for OIDC-related fields~~ CNTRLPLANE-367: Update Authentication API GoDoc for OIDC-related fields May 7, 2025

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 7, 2025

jacobsee reviewed May 7, 2025

View reviewed changes

config/v1/types_authentication.go Outdated Show resolved Hide resolved

config/v1/types_authentication.go Outdated Show resolved Hide resolved

liouk suggested changes May 8, 2025

View reviewed changes

bergerhoffer reviewed May 8, 2025

View reviewed changes

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 9, 2025

JoelSpeed reviewed May 13, 2025

View reviewed changes

everettraven force-pushed the chore/update-auth-oidc-field-godoc branch from 13aa77c to bb2a662 Compare May 13, 2025 15:14

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2025

config/v1: update oidc-related field godoc

fc7af95

Signed-off-by: Bryce Palmer <[email protected]>

everettraven force-pushed the chore/update-auth-oidc-field-godoc branch from bb2a662 to fc7af95 Compare May 13, 2025 15:34

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2025

openshift-ci bot assigned JoelSpeed May 13, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2025

openshift-merge-bot bot merged commit 6a97aca into openshift:master May 15, 2025
23 of 24 checks passed

bergerhoffer mentioned this pull request May 29, 2025

OSDOCS#12643: Docs for BYO OIDC auth provider openshift/openshift-docs#93779

Merged

1 task

	// clientID configures the identifier, from the identity provider, that the platform
	// clientID configures the client identifier, from the identity provider, that the platform

	// name is a required field that configures the unique human-readable identifier
	// Name is a required field that configures the unique human-readable identifier

	// namespace containing the PEM encoded CA certificates under the 'ca-bundle.crt'
	// namespace containing the PEM-encoded CA certificates under the 'ca-bundle.crt'

	// When set to Prefix, the value specifed in the prefix field will be
	// When set to 'Prefix', the value specified in the prefix field will be

	// Example: if `prefix` is set to "myoidc:"" and the `claim` in JWT contains
	// Example: if `prefix` is set to "myoidc:" and the `claim` in JWT contains

	// The prefix field must be set when prefixPolicy is Prefix.
	// The prefix field must be set when prefixPolicy is 'Prefix'.

	// When set to NoPrefix, no prefix will be prepended to the value
	// When set to 'NoPrefix', no prefix will be prepended to the value

	// any prefixes that are applied - which may be subject to change over time.
	// any prefixes that are applied - which might be subject to change over time.

	// When set to RequiredClaim, the Kubernetes API Server
	// When set to 'RequiredClaim', the Kubernetes API Server

CNTRLPLANE-367: Update Authentication API GoDoc for OIDC-related fields #2314

CNTRLPLANE-367: Update Authentication API GoDoc for OIDC-related fields #2314

Uh oh!

Conversation

everettraven commented May 7, 2025

Uh oh!

openshift-ci bot commented May 7, 2025

Uh oh!

openshift-ci-robot commented May 7, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergerhoffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everettraven commented May 13, 2025

Uh oh!

everettraven commented May 13, 2025

Uh oh!

JoelSpeed commented May 13, 2025

Uh oh!

openshift-ci bot commented May 13, 2025

Uh oh!

openshift-ci-robot commented May 13, 2025

Uh oh!

openshift-ci-robot commented May 14, 2025

Uh oh!

openshift-ci-robot commented May 7, 2025 •

edited by openshift-ci bot

Loading

openshift-ci bot commented May 14, 2025 •

edited

Loading