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

✨ Scaffold auth proxy #513

Merged
merged 3 commits into from
Dec 11, 2018
Merged

✨ Scaffold auth proxy #513

merged 3 commits into from
Dec 11, 2018

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Dec 6, 2018 •
edited
Loading

The 1st commit update CR and CT to pickup latest changes that is necessary for this PR.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 6, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mengqiy

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2018
droot
droot reviewed Dec 6, 2018
View reviewed changes
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Thanks for splitting the changes in separate commits. That was helpful. Change looks good. I have suggestions about using "auth_proxy" everywhere.

pkg/scaffold/manager/cmd.go Outdated
@@ -57,6 +57,8 @@ import (
)

func main() {
var metricsBindAddr string
flag.StringVar(&metricsBindAddr, "metrics-bind-addr", ":8080", "The address the metric endpoint binds to.")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we call the flag metrics-addr ? (internally we can use metricsBindAddr)

pkg/scaffold/project/kustomize_proxy_patch.go Outdated

// KustomizeProxyPatch scaffolds the patch file for enabling
// prometheus metrics for manager Pod.
type KustomizeProxyPatch struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call this AuthProxy instead of Proxy ?

pkg/scaffold/project/kustomize.go Outdated
# Comment the following 3 lines if you want to disable
# kube-rbac-proxy (https://github.com/brancz/kube-rbac-proxy)
# which protects your /metrics endpoint.
- ../rbac/kube-rbac-proxy-service.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name this file auth_proxy_service.yaml to keep it consistent with the roles file (also note the underscore ?

pkg/scaffold/project/kustomize.go Outdated
# Protect the /metrics endpoint by putting it behind auth.
# Only one of manager_kube_rbac_proxy_patch.yaml and
# manager_prometheus_metrics_patch.yaml should be enabled.
- manager_kube_rbac_proxy_patch.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this file to use manager_auth_proxy_patch.yaml ?

pkg/scaffold/project/kustomize.go Outdated
# manager_prometheus_metrics_patch.yaml should be enabled.
- manager_kube_rbac_proxy_patch.yaml
# If you want your controller-manager to expose the /metrics
# endpoint w/o any authorization, uncomment the following line and
Copy link
Contributor

Choose a reason for hiding this comment

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

w/o any authn/authz ?

@mengqiy
Copy link
Member Author

mengqiy commented Dec 7, 2018

PTAL

@mengqiy mengqiy changed the title [WIP] ✨ Scaffold auth ✨ Scaffold auth Dec 7, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2018
@mengqiy
Copy link
Member Author

mengqiy commented Dec 7, 2018 •
edited
Loading

@droot The last 2 commits are incremental changes since your last review.
I will squash the fixup commits after get LGTM.

@mengqiy mengqiy force-pushed the scaffold_auth branch 2 times, most recently from 4af3bbf to 9818fef Compare December 7, 2018 21:39
mengqiy
mengqiy commented Dec 7, 2018
View reviewed changes
Gopkg.toml
@@ -5,6 +5,7 @@ required = [
"github.com/go-openapi/spec",
"github.com/onsi/ginkgo", # for integration testing
"github.com/spf13/pflag",
"github.com/pkg/errors",
Copy link
Member Author

Choose a reason for hiding this comment

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

dep will drop it w/o this line even it's a dependency of sigs.k8s.io/controller-tools

@droot
Copy link
Contributor

droot commented Dec 10, 2018

Looks good to me. Pl. squash the commits and then we are good to do.

@mengqiy
Copy link
Member Author

mengqiy commented Dec 10, 2018

Looks good to me. Pl. squash the commits and then we are good to do.

Done

@droot droot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2018
@k8s-ci-robot k8s-ci-robot merged commit fd32f8b into kubernetes-sigs:master Dec 11, 2018
@mengqiy mengqiy deleted the scaffold_auth branch December 11, 2018 19:27
@mengqiy mengqiy mentioned this pull request Dec 17, 2018
Closed
@mengqiy mengqiy changed the title ✨ Scaffold auth ✨ Scaffold auth proxy Feb 28, 2019
@micahhausler micahhausler mentioned this pull request May 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droot droot droot left review comments

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

Assignees
No one assigned
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mengqiy @k8s-ci-robot @droot