Skip to content

cmd/helm-operator/main.go: add --metrics-addr flag #3440

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

Merged
joelanford merged 4 commits into from
Jul 16, 2020
Merged

cmd/helm-operator/main.go: add --metrics-addr flag #3440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce24847
cmd/helm-operator/main.go: add --metrics-addr flag
joelanford Jul 16, 2020
706d68e
internal/plugins/helm/v1/scaffolds/templates/metricsauth/auth_proxy_p…
joelanford Jul 16, 2020
79e3882
internal/plugins/helm/v1/scaffolds/templates/manager/config.go: remov…
joelanford Jul 16, 2020
85989b9
empty commit to force ci to start again
joelanford Jul 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions changelog/fragments/helm-metrics-addr.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
entries:
- description: >
Added `--metrics-addr` flag to helm operator to make it configurable, and
changed the default from `:8383` to `:8080`

kind: "change"

# Is this a breaking change?
breaking: true

migration:
header: Default helm operator metrics port changed
body: >
To continue using port 8383, specify `--metrics-addr=:8383` when you start the operator.
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Jul 16, 2020

Choose a reason for hiding this comment

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

WDYT about we improve it and describe where the --metrics-addr=:8383 need to be used (config/default, and config/manager/)?

Copy link
Copy Markdown
Member Author

@joelanford joelanford Jul 16, 2020

Choose a reason for hiding this comment

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

I disagree for a few reasons:

  1. In general, it should be obvious to most operator authors, and kubernetes users how to set flags on their operator.
  2. In fairness, it is somewhat confusing when kustomize enters the picture, so I would propose that we write a general section in a doc somewhere about how to set flags on the manager when using the kustomize templates. This should probably be a kubebuidler doc.
  3. There are lots of flags on operators. We shouldn't have to repeat ourselves in migration guides, docs, etc. anytime we discuss setting flags.

Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Jul 16, 2020

Choose a reason for hiding this comment

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

Far enough.

5 changes: 1 addition & 4 deletions cmd/helm-operator/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ import (

var (
metricsHost = "0.0.0.0"
metricsPort int32 = 8383
operatorMetricsPort int32 = 8686

log = logf.Log.WithName("cmd")
Expand Down Expand Up @@ -77,7 +76,7 @@ func main() {

// Set default manager options
options := manager.Options{
MetricsBindAddress: fmt.Sprintf("%s:%d", metricsHost, metricsPort),
MetricsBindAddress: f.MetricsAddress,
NewClient: func(cache cache.Cache, config *rest.Config, options crclient.Options) (crclient.Client, error) {
c, err := crclient.New(config, options)
if err != nil {
Expand Down Expand Up @@ -184,8 +183,6 @@ func addMetrics(ctx context.Context, cfg *rest.Config, gvks []schema.GroupVersio

// Add to the below struct any other metrics ports you want to expose.
servicePorts := []v1.ServicePort{
{Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP,
TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
{Port: operatorMetricsPort, Name: metrics.CRPortName, Protocol: v1.ProtocolTCP,
TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: operatorMetricsPort}},
}
Expand Down
4 changes: 1 addition & 3 deletions internal/plugins/helm/v1/scaffolds/templates/manager/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,7 @@ spec:
control-plane: controller-manager
spec:
containers:
- args:
- manager
Comment on lines -90 to -91
Copy link
Copy Markdown
Member Author

@joelanford joelanford Jul 16, 2020

Choose a reason for hiding this comment

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

This removal is no directly related to this PR, but needs to happen to avoid problems running the operator in a cluster.

This is an accidental carryover from Go operators that have a manager binary. But helm-operator has a different binary entrypoint: the base image adds the binary and sets the entrypoint for us, so we just need to specify the flags to pass to the helm-operator binary.

image: {{ .Image }}
- image: {{ .Image }}
name: manager
resources:
limits:
Expand Down
7 changes: 3 additions & 4 deletions internal/plugins/helm/v1/scaffolds/templates/metricsauth/auth_proxy_patch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,6 @@ func (f *AuthProxyPatch) SetTemplateDefaults() error {
// todo(camilamacedo86): add the arg --enable-leader-election for the manager
// More info: https://github.com/operator-framework/operator-sdk/issues/3356

// todo(camilamacedo86): add the arg --metrics-addr for the manager
// More info: https://github.com/operator-framework/operator-sdk/issues/3358

const kustomizeAuthProxyPatchTemplate = `# This patch inject a sidecar container which is a HTTP proxy for the
# controller manager, it performs RBAC authorization against the Kubernetes API using SubjectAccessReviews.
apiVersion: apps/v1
Expand All @@ -65,11 +62,13 @@ spec:
image: gcr.io/kubebuilder/kube-rbac-proxy:v0.5.0
args:
- "--secure-listen-address=0.0.0.0:8443"
- "--upstream=http://127.0.0.1:8383/"
- "--upstream=http://127.0.0.1:8080/"
- "--logtostderr=true"
- "--v=10"
ports:
- containerPort: 8443
name: https
- name: manager
args:
- "--metrics-addr=127.0.0.1:8080"
`
6 changes: 6 additions & 0 deletions pkg/helm/flags/flag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ type Flags struct {
ReconcilePeriod time.Duration
WatchesFile string
MaxWorkers int
MetricsAddress string
}

// AddTo - Add the helm operator flags to the the flagset
Expand All @@ -48,4 +49,9 @@ func (f *Flags) AddTo(flagSet *pflag.FlagSet) {
runtime.NumCPU(),
"Maximum number of workers to use",
)
flagSet.StringVar(&f.MetricsAddress,
"metrics-addr",
":8080",
"The address the metric endpoint binds to",
)
}