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

Support for OIDC 'username-prefix' and 'groups-prefix' flags #4085

Merged
merged 1 commit into from
Dec 18, 2017
Merged

Support for OIDC 'username-prefix' and 'groups-prefix' flags #4085

merged 1 commit into from
Dec 18, 2017

Conversation

xoen
Copy link
Contributor

@xoen xoen commented Dec 15, 2017 •
edited
Loading

What

Added support for --oidc-username-prefix and --oidc-groups-prefix.
By passing these it's possible to override the default prefixes used to map the OIDC user with the username in kubernetes.

See

See: https://kubernetes.io/docs/admin/authentication/#configuring-the-api-server

IMPORTANT

I'm far from a kubernetes/KOPS, this is not tested so someone needs to have a look and see if something is missing or if this can cause troubles! (don't want to accidentally cause the destruction of the universe 💥 )

It's basically the same done in this other PR: https://github.com/kubernetes/kops/pull/1438/files

I did not change the zz_generated.conversion.go files as according to comment at the top of them they're autogenerated:

// This file was autogenerated by conversion-gen. Do not edit it manually!

(I wonder if they should be .gitignored)

Fixes

This should fix #4007: field oidcUsernamePrefix is not recognized in cluster configuration file

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 15, 2017
@xoen
Copy link
Contributor Author

xoen commented Dec 15, 2017

FYI: CLA signed.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 15, 2017
@chrislovecnm
Copy link
Contributor

@xoen yes they are autogenerated, we need them generated.make apimachinery is your friend.
/ok-to-test

will review when I have a couple of minutes.

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 15, 2017
chrislovecnm
chrislovecnm suggested changes Dec 16, 2017
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Update the docs and run the make command and we can get this in. Thanks for the PR!!

pkg/apis/kops/componentconfig.go Outdated
@@ -228,9 +228,15 @@ type KubeAPIServerConfig struct {
// The OpenID claim to use as the user name.
// Note that claims other than the default ('sub') is not guaranteed to be unique and immutable.
OIDCUsernameClaim *string `json:"oidcUsernameClaim,omitempty" flag:"oidc-username-claim"`
// Prefix prepended to username claims to prevent clashes with existing
// names (such as 'system:' users).
OIDCUsernamePrefix *string `json:"oidcUsernamePrefix,omitempty" flag:"oidc-username-prefix"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Great docs. You mind updating them to follow the go style guide? https://blog.golang.org/godoc-documenting-go-code

Sentence starts with the struct member name.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are trying to clean these up as you can see many are not correct.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 18, 2017
@xoen
Copy link
Contributor Author

xoen commented Dec 18, 2017 •
edited
Loading

Hi @chrislovecnm - I ran make apimachinery as suggested and updated all the documentation for the OIDC-related fields (as I was in there).

Let me know if this is good.

@chrislovecnm
Copy link
Contributor

Looks awesome, if you can squash you git comitts into one commit, we can merge.

Thanks again!

@xoen xoen force-pushed the ag-oidc-prefixes-config branch from 781c4bf to 9e30999 Compare December 18, 2017 15:37
@xoen
Copy link
Contributor Author

xoen commented Dec 18, 2017

@chrislovecnm my pleasure, it's just a tiny one :) - I squashed the changes in one single commit now (sorry, wasn't sure about that).

@xoen
Copy link
Contributor Author

xoen commented Dec 18, 2017

@chrislovecnm I added these two fields by copying the other PR but this is not tested.
As far as I can tell these are optional and this should be retro-compatible but...do you reckon will break anything?

@justinsb
Copy link
Member

/lgtm

Thanks @xoen :-)

And yes, this is correct: these fields won't be set by default, so can't cause any damage unless someone actively sets the fields. And the mapping to flags looks correct. It is possible to do a test but frankly it's probably overkill vs just manually verifying that the flag values are the right values :-)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2017
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit 2eae62d into kubernetes:master Dec 18, 2017
@xoen xoen mentioned this pull request Jan 19, 2018
Closed
@sh31 sh31 mentioned this pull request Apr 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrislovecnm chrislovecnm chrislovecnm requested changes

Assignees

@justinsb justinsb

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

field oidcUsernamePrefix is not recognized in cluster configuration file
5 participants
@xoen @k8s-ci-robot @chrislovecnm @justinsb @k8s-github-robot