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 support for token authentication with network proxy #88419

Merged
merged 3 commits into from Mar 3, 2020
Merged

Add support for token authentication with network proxy #88419

merged 3 commits into from Mar 3, 2020

Conversation

Jefftree
Copy link
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add support for token based authentication for konnectivity server to authenticate konnectivity agent.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

KUBE_ENABLE_EGRESS_VIA_KONNECTIVITY_SERVICE=true ./cluster/kube-up.sh to set up

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/assign @caesarxuchao
/cc @cheftako
/cc @dberkov
/sig api-machinery

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Feb 21, 2020
@k8s-ci-robot
Copy link
Contributor

@Jefftree: GitHub didn't allow me to request PR reviews from the following users: dberkov.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add support for token based authentication for konnectivity server to authenticate konnectivity agent.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

KUBE_ENABLE_EGRESS_VIA_KONNECTIVITY_SERVICE=true ./cluster/kube-up.sh to set up

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/assign @caesarxuchao
/cc @cheftako
/cc @dberkov
/sig api-machinery

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 k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 21, 2020
@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Feb 21, 2020
@Jefftree Jefftree force-pushed the netproxy-udstoken branch 2 times, most recently from 42965ed to 0d0eb69 Compare February 27, 2020 19:03
@k8s-ci-robot k8s-ci-robot added area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Feb 27, 2020
@Jefftree
Copy link
Member Author

/retest

@caesarxuchao
Copy link
Member

The configuration changes are consistent with kubernetes-sigs/apiserver-network-proxy#51.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2020
@Jefftree
Copy link
Member Author

Jefftree commented Feb 27, 2020 •
edited

/assign @liggitt (for approval)

lavalamp
lavalamp reviewed Feb 29, 2020
View reviewed changes
cluster/gce/gci/configure-helper.sh
@@ -652,8 +652,13 @@ function create-master-auth {
append_or_replace_prefixed_line "${known_tokens_csv}" "${GCE_GLBC_TOKEN}," "system:controller:glbc,uid:system:controller:glbc"
fi
if [[ -n "${ADDON_MANAGER_TOKEN:-}" ]]; then
append_or_replace_prefixed_line "${known_tokens_csv}" "${ADDON_MANAGER_TOKEN}," "system:addon-manager,uid:system:addon-manager,system:masters"
append_or_replace_prefixed_line "${known_tokens_csv}" "${ADDON_MANAGER_TOKEN}," "system:addon-manager,uid:system:addon-manager,system:masters"
Copy link
Member

Choose a reason for hiding this comment

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

s/ / /

Copy link
Member

Choose a reason for hiding this comment

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

(there should be 1 space instead of a bunch of spaces and a tab there)

Copy link
Member

Choose a reason for hiding this comment

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

actually just revert this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes the alignment with the previous lines. Would you still like me to revert?

Copy link
Member

Choose a reason for hiding this comment

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

ack, sorry, the diff obscured this.

lavalamp
lavalamp reviewed Feb 29, 2020
View reviewed changes
cluster/gce/manifests/konnectivity-server.yaml Outdated
type: DirectoryOrCreate
---
Copy link
Member

Choose a reason for hiding this comment

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

it's weird to have a second blob in this .yaml, when you made a separate file above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Each cluster addon has its own directory but all the manifests share the same directory (at least in gce). Didn't want to pollute the manifests directory.

Copy link
Member

Choose a reason for hiding this comment

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

I see, ok.

@lavalamp
Copy link
Member

/approve

/hold

for nit fixes

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 29, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2020
@caesarxuchao
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2020
@caesarxuchao
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2020
@caesarxuchao
Copy link
Member

/retest

1 similar comment
@Jefftree
Copy link
Member Author

Jefftree commented Mar 2, 2020

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2020
@caesarxuchao
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@Jefftree
Copy link
Member Author

Jefftree commented Mar 2, 2020

/test pull-kubernetes-e2e-gce

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2020
@caesarxuchao
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2020
@Jefftree
Copy link
Member Author

Jefftree commented Mar 3, 2020 •
edited

/test pull-kubernetes-e2e-gce-100-performance

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 3, 2020 •
edited

@Jefftree: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-network-proxy 0cd61e94608e803717df8cfaba8236e141286a44 link /test pull-kubernetes-e2e-gce-network-proxy

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.

@Jefftree
Copy link
Member Author

Jefftree commented Mar 3, 2020

/test pull-kubernetes-e2e-gce-100-performance

@k8s-ci-robot k8s-ci-robot merged commit eaceb7b into kubernetes:master Mar 3, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Mar 3, 2020
@Jefftree Jefftree deleted the netproxy-udstoken branch December 7, 2021 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@caesarxuchao caesarxuchao caesarxuchao left review comments

@dberkov dberkov dberkov left review comments

@cheftako cheftako Awaiting requested review from cheftako

Assignees

@liggitt liggitt

@caesarxuchao caesarxuchao

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.18
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Jefftree @k8s-ci-robot @caesarxuchao @lavalamp @fejta-bot @dberkov @liggitt