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

delegated authn/z: optionally opt-out of mandatory authn/authz kubeconfig #67545

Merged
merged 1 commit into from Aug 28, 2018

Conversation

@sttts
Contributor

sttts commented Aug 17, 2018

This adds RemoteKubeConfigFileOptional field to the delegated authn/z option structs. If set to true, the authn/z kubeconfig file flags are optional. If no kubeconfig is given, all token requests are considered to be anonymous and no client CA is looked up in the cluster.

Prerequisite for #64149 and #67069.

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 17, 2018

Contributor

@sttts: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

Contributor

k8s-ci-robot commented Aug 17, 2018

@sttts: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 17, 2018

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://git.k8s.io/community/CLA.md#the-contributor-license-agreement 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.


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.

Contributor

k8s-ci-robot commented Aug 17, 2018

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://git.k8s.io/community/CLA.md#the-contributor-license-agreement 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.


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.

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 22, 2018

Contributor

/retest

Contributor

sttts commented Aug 22, 2018

/retest

@awly

This comment has been minimized.

Show comment
Hide comment
@awly

awly Aug 22, 2018

Contributor

This seems to have a lot of overlap with #67543

Contributor

awly commented Aug 22, 2018

This seems to have a lot of overlap with #67543

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 23, 2018

Contributor

@awly no overlap. It is just based on the former.

Contributor

sttts commented Aug 23, 2018

@awly no overlap. It is just based on the former.

@nikhita

This comment has been minimized.

Show comment
Hide comment
@nikhita

nikhita Aug 23, 2018

Member

/remove-area custom-resources

Member

nikhita commented Aug 23, 2018

/remove-area custom-resources

@yue9944882

This comment has been minimized.

Show comment
Hide comment
@yue9944882

yue9944882 Aug 27, 2018

Contributor

gotcha. need to rebase onto #67768.

Contributor

yue9944882 commented Aug 27, 2018

gotcha. need to rebase onto #67768.

@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Aug 27, 2018

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 27, 2018

Contributor

@awly @yue9944882 after the prerequisite merged, this is pretty trivial now. ptal.

Contributor

sttts commented Aug 27, 2018

@awly @yue9944882 after the prerequisite merged, this is pretty trivial now. ptal.

@sttts sttts added this to the v1.12 milestone Aug 27, 2018

@@ -41,6 +42,9 @@ type DelegatingAuthorizationOptions struct {
// RemoteKubeConfigFile is the file to use to connect to a "normal" kube API server which hosts the
// SubjectAccessReview.authorization.k8s.io endpoint for checking tokens.
RemoteKubeConfigFile string
// RemoteKubeConfigFileOptional is specifying whether not specifying the kubeconfig or
// a missing in-cluster config will be fatal.
RemoteKubeConfigFileOptional bool

This comment has been minimized.

@yue9944882

yue9944882 Aug 28, 2018

Contributor

if we don’t add it to the flagset,where can we set its value?

@yue9944882

yue9944882 Aug 28, 2018

Contributor

if we don’t add it to the flagset,where can we set its value?

This comment has been minimized.

@sttts

sttts Aug 28, 2018

Contributor

the component using the option struct sets it.

@sttts

sttts Aug 28, 2018

Contributor

the component using the option struct sets it.

This comment has been minimized.

@yue9944882

yue9944882 Aug 28, 2018

Contributor

i thought that every field in option struct should be added to flagset

@yue9944882

yue9944882 Aug 28, 2018

Contributor

i thought that every field in option struct should be added to flagset

This comment has been minimized.

@sttts

sttts Aug 28, 2018

Contributor

No, why? We have many which are not.

@sttts

sttts Aug 28, 2018

Contributor

No, why? We have many which are not.

This comment has been minimized.

@sttts

sttts Aug 28, 2018

Contributor

look into SecureServing as an example.

@sttts

sttts Aug 28, 2018

Contributor

look into SecureServing as an example.

if err != nil {
return nil, err
if client == nil {
glog.Warningf("No authorization-kubeconfig provided, so SubjectAccessReview of authorization tokens won't work.")

This comment has been minimized.

@yue9944882

yue9944882 Aug 28, 2018

Contributor

nit: what about straightly return the union here?a long else block looks odd to me 👀

@yue9944882

yue9944882 Aug 28, 2018

Contributor

nit: what about straightly return the union here?a long else block looks odd to me 👀

This comment has been minimized.

@sttts

sttts Aug 28, 2018

Contributor

I prefer the if block. The authorizers slice is constructed one by one by appending new authorizers. I think it is easier to reason about the code the way it is. The pattern is clear when another authorizer is added with another if block. Then an early return wouldn't work anymore.

@sttts

sttts Aug 28, 2018

Contributor

I prefer the if block. The authorizers slice is constructed one by one by appending new authorizers. I think it is easier to reason about the code the way it is. The pattern is clear when another authorizer is added with another if block. Then an early return wouldn't work anymore.

@yue9944882

This comment has been minimized.

Show comment
Hide comment
@yue9944882

yue9944882 Aug 28, 2018

Contributor

/retest

Contributor

yue9944882 commented Aug 28, 2018

/retest

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 28, 2018

Contributor

@yue9944882 @awly addressed all comments. Anything left?

Contributor

sttts commented Aug 28, 2018

@yue9944882 @awly addressed all comments. Anything left?

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 28, 2018

Contributor

/retest

Contributor

sttts commented Aug 28, 2018

/retest

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 28, 2018

Contributor

@awly lgty?

Contributor

sttts commented Aug 28, 2018

@awly lgty?

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 28, 2018

Contributor

@sttts: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gke 74f594e link /test pull-kubernetes-e2e-gke
pull-kubernetes-cross 74f594e link /test pull-kubernetes-cross

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

Contributor

k8s-ci-robot commented Aug 28, 2018

@sttts: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gke 74f594e link /test pull-kubernetes-e2e-gke
pull-kubernetes-cross 74f594e link /test pull-kubernetes-cross

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 28, 2018

Contributor

/retest

Contributor

sttts commented Aug 28, 2018

/retest

@awly

Small nits, but LGTM

@@ -313,7 +318,7 @@ func DefaultKubernetesUserAgent() string {
func InClusterConfig() (*Config, error) {

This comment has been minimized.

@awly

awly Aug 28, 2018

Contributor

Update the above comment to mention ErrNotInCluster

@awly

awly Aug 28, 2018

Contributor

Update the above comment to mention ErrNotInCluster

This comment has been minimized.

@sttts

sttts Aug 28, 2018

Contributor

fixed

@sttts

sttts Aug 28, 2018

Contributor

fixed

Show outdated Hide outdated staging/src/k8s.io/client-go/rest/config.go

@sttts sttts added the lgtm label Aug 28, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 28, 2018

@awly

This comment has been minimized.

Show comment
Hide comment
@awly

awly Aug 28, 2018

Contributor

/lgtm

Contributor

awly commented Aug 28, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 28, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 28, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awly, sttts

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:

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

Contributor

k8s-ci-robot commented Aug 28, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awly, sttts

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:

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

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 28, 2018

Contributor

Automatic merge from submit-queue (batch tested with PRs 66960, 67545). If you want to cherry-pick this change to another branch, please follow the instructions here.

Contributor

k8s-merge-robot commented Aug 28, 2018

Automatic merge from submit-queue (batch tested with PRs 66960, 67545). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 1b3a2dd into kubernetes:master Aug 28, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation sttts authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment