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

Add authz flags #1357

Merged
merged 1 commit into from
Jan 21, 2017
Merged

Add authz flags #1357

merged 1 commit into from
Jan 21, 2017

Conversation

justinsb
Copy link
Member

@justinsb justinsb commented Jan 5, 2017 •
edited by k8s-reviewable
Loading

We aren't wiring them up now, but this unblocks people that want to have
a go.

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 5, 2017
@justinsb justinsb modified the milestone: 1.5.0 Jan 5, 2017
chrislovecnm
chrislovecnm reviewed Jan 5, 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.

Question

pkg/apis/kops/componentconfig.go
@@ -384,6 +384,9 @@ type KubeAPIServerConfig struct {
KubeletPreferredAddressTypes []string `json:"kubeletPreferredAddressTypes,omitempty" flag:"kubelet-preferred-address-types"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why we would add something to the API, and not wire it up. The API now has stuff in it that does not work. Kind of confusing for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 here - what gives?

Copy link
Contributor

Choose a reason for hiding this comment

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

@justinsb explained that we can have API values on components that don't have legacy yaml stuff, and it will add the flags automatically.

@krisnova
Copy link
Contributor

krisnova commented Jan 5, 2017

If this is unblocking a few devs can we maybe

  1. Have them fork off this branch to get your changes? That way we don't pollute 1.5 with breaking API directives
  2. At least wait to merge after 1.5.0 beta? For the same reason :)

@bickfordb
Copy link

It would also be great if you could add the various authentication flags? e.g. --oidc-$x

@justinsb
Copy link
Member Author

@kris-nova @chrislovecnm so you could specify these flags by putting this into your cluster spec:

spec:
  kubeAPIServer:
    authorizationMode: rbac
    authorizationRbacSuperUser: admin

@blakebarnett blakebarnett mentioned this pull request Jan 10, 2017
Merged
chrislovecnm
chrislovecnm reviewed Jan 12, 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.

Do we need any tests or just list it as alpha?

pkg/apis/kops/componentconfig.go
@@ -384,6 +384,9 @@ type KubeAPIServerConfig struct {
KubeletPreferredAddressTypes []string `json:"kubeletPreferredAddressTypes,omitempty" flag:"kubelet-preferred-address-types"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@justinsb explained that we can have API values on components that don't have legacy yaml stuff, and it will add the flags automatically.

@blakebarnett blakebarnett mentioned this pull request Jan 12, 2017
Closed
liggitt
liggitt reviewed Jan 15, 2017
View reviewed changes
pkg/apis/kops/componentconfig.go
@@ -384,6 +384,9 @@ type KubeAPIServerConfig struct {
KubeletPreferredAddressTypes []string `json:"kubeletPreferredAddressTypes,omitempty" flag:"kubelet-preferred-address-types"`

StorageBackend *string `json:"storageBackend,omitempty" flag:"storage-backend"`

AuthorizationMode *string `json:"authorizationMode,omitempty" flag:"authorization-mode"`
AuthorizationRBACSuperUser *string `json:"authorizationRbacSuperUser,omitempty" flag:"authorization-rbac-super-user"`
Copy link
Member

Choose a reason for hiding this comment

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

note that the superuser flag was removed post-1.5. not sure you want to add it just for the 1.5 release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! But I think people will want to set it in 1.5, so we have to map it.

@justinsb
Add authz flags
33884d2 
We aren't wiring them up now, but this unblocks people that want to have
a go.
This was referenced Jan 20, 2017
Closed
Merged
Closed
krisnova
krisnova approved these changes Jan 21, 2017
View reviewed changes
Copy link
Contributor

@krisnova krisnova left a comment

Choose a reason for hiding this comment

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

LGTM

@krisnova krisnova merged commit 97afdf9 into kubernetes:master Jan 21, 2017
@zmerlynn zmerlynn mentioned this pull request Jan 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrislovecnm chrislovecnm chrislovecnm left review comments

@liggitt liggitt liggitt left review comments

@krisnova krisnova krisnova approved these changes

Assignees
No one assigned
Labels
blocks-next cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Milestone
1.5.1
Development

Successfully merging this pull request may close these issues.
6 participants
@justinsb @krisnova @bickfordb @chrislovecnm @liggitt @k8s-ci-robot