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

create audience unaware authenticator wrappers #69916

Merged
merged 3 commits into from Nov 1, 2018

Conversation

@mikedanese
Member

mikedanese commented Oct 17, 2018

Last commit.
For some of #69893
Depends on #69914 and #69582

NONE
@fedebongio

This comment has been minimized.

Contributor

fedebongio commented Oct 18, 2018

/cc @liggitt

@k8s-ci-robot k8s-ci-robot requested a review from liggitt Oct 18, 2018

@k8s-ci-robot k8s-ci-robot added size/L and removed size/XXL labels Oct 23, 2018

mikedanese added a commit to mikedanese/kubernetes that referenced this pull request Oct 26, 2018

support multiple clients in the oidc authenticator
But don't expose this via the commandline. This is to support @liggitt's
excellent suggestion here:

kubernetes#69916 (comment)
@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 26, 2018

/retest

}
if len(resp.Audiences) > 0 {
// maybe the authenticator was audience aware after all.
return nil, false, fmt.Errorf("audience unaware authenticator wrapped an authenaticator that returned an audience: %q", resp.Audiences)

This comment has been minimized.

@liggitt

liggitt Oct 30, 2018

Member

nit: typo authenaticator

@liggitt

This comment has been minimized.

Member

liggitt commented Oct 30, 2018

/approve
/hold

one nit, lgtm otherwise. wasn't sure whether you wanted this in ahead of the service account and oidc changes or not, unhold at will

@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 31, 2018

For staging/BUILD approval:
/assign @lavalamp

limitations under the License.
*/
package audunaware

This comment has been minimized.

@lavalamp

lavalamp Oct 31, 2018

Member

We've got to fix the package name, it's unpronouncable and there's no package godoc to help me figure out what it's supposed to be an abbreviation for.

Can you drop it inside an internal/ subdir? Then I don't care too much about the name.

I'll try to think of another one. If we can't, we should at least not use "aud" as an abbreviation for audience.

This comment has been minimized.

@mikedanese

mikedanese Oct 31, 2018

Member

Unfortunately, I think this package needs to be external. Authenticators across plugin/, k8s.io/apiserver/plugin, k8s.io/apiserver, k8s.io/apiserver/pkg/authentication depend on it.

"aud" as an abbreviation for audience is part of many authentication standards, e.g. https://tools.ietf.org/html/rfc7519#section-4.1.3 so the choice of abbreviation is not ad-hoc. I think "audunaware" and "audienceunaware" are both pretty awkward. I'd like to skip introducing a new package for this but hit an import cycle requiring this request.AudiencesFrom:

func WithAudiences(ctx context.Context, auds authenticator.Audiences) context.Context {

I think I'll just go ahead a break the import cycle and avoid the tricky problem of package naming altogether.

This comment has been minimized.

@lavalamp

lavalamp Oct 31, 2018

Member

"aud" as an abbreviation for audience is part of many authentication standards,

TIL

How about audagnostic, just agnostic, or audience/agnostic for the package? "unaware" is a super awkward word to use like this.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 31, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mikedanese

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

@mikedanese

This comment has been minimized.

Member

mikedanese commented Oct 31, 2018

@liggitt @lavalamp refactored to break import cycle and moved the wrappers into the authenticator package (as suggested in #69916 (comment)). LMK what you think.

@liggitt

This comment has been minimized.

Member

liggitt commented Nov 1, 2018

/lgtm

@mikedanese

This comment has been minimized.

Member

mikedanese commented Nov 1, 2018

There's no ordering dependency between this and oidc or service accounts.

/hold cancel

@lavalamp

This comment has been minimized.

Member

lavalamp commented Nov 1, 2018

thanks :)

@mikedanese

This comment has been minimized.

Member

mikedanese commented Nov 1, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit 1165d66 into kubernetes:master Nov 1, 2018

16 of 18 checks passed

pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation mikedanese 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-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-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
tide In merge pool.
Details

@mikedanese mikedanese deleted the mikedanese:trev9 branch Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment