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.

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

⚠️ Deprecate metrics-bind-addr flag #70

Merged
merged 21 commits into from
Apr 17, 2024

Conversation

gfariasalves-ionos
Copy link
Contributor

@gfariasalves-ionos gfariasalves-ionos commented Mar 14, 2024

What is the purpose of this pull request/Why do we need it?

See: https://cluster-api.sigs.k8s.io/developer/providers/migrations/v1.5-to-v1.6
See: kubernetes-sigs/cluster-api#9264

Checklist:

  • Documentation updated
  • Includes emojis

@gfariasalves-ionos gfariasalves-ionos changed the title Deprefate metrics-bind-addr flag Deprecate metrics-bind-addr flag Mar 14, 2024
go.mod Outdated Show resolved Hide resolved
@piepmatz
Copy link
Contributor

Upgrading controller-runtime to v0.17.2 resolves the linting issues.

Copy link
Contributor

@piepmatz piepmatz left a comment

Choose a reason for hiding this comment

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

  • Don't forget updating the kustomize manifests: --metrics-bind-address is still in use there. And I'd really also go with --insecure-diagnostics for now.
  • However, we should at least provide the RBAC required for secure diagnostics:
// Add RBAC for the authorized diagnostics endpoint.
// +kubebuilder:rbac:groups=authentication.k8s.io,resources=tokenreviews,verbs=create
// +kubebuilder:rbac:groups=authorization.k8s.io,resources=subjectaccessreviews,verbs=create

cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
@piepmatz
Copy link
Contributor

piepmatz commented Apr 4, 2024

Seems like #70 (review) was forgotten?

@gfariasalves-ionos
Copy link
Contributor Author

I did not, just forgot to push ;)

@piepmatz
Copy link
Contributor

piepmatz commented Apr 5, 2024

I did not, just forgot to push ;)

So, maybe you wanna push now?

config/manager/manager.yaml Outdated Show resolved Hide resolved
config/manager/manager.yaml Outdated Show resolved Hide resolved
@piepmatz
Copy link
Contributor

Have you tested the secured metrics port? So were you able to retrieve metrics after creating a role + binding and querying the port as the one the role was bound to?

@gfariasalves-ionos
Copy link
Contributor Author

Yes, I did. I tried exactly what was described in Cluster API documentation

Copy link

sonarcloud bot commented Apr 17, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@gfariasalves-ionos gfariasalves-ionos changed the title Deprecate metrics-bind-addr flag ⚠️ Deprecate metrics-bind-addr flag Apr 17, 2024
@gfariasalves-ionos gfariasalves-ionos enabled auto-merge (squash) April 17, 2024 11:00
@gfariasalves-ionos gfariasalves-ionos merged commit 6a61d9c into main Apr 17, 2024
8 checks passed
@gfariasalves-ionos gfariasalves-ionos deleted the deprecated-metrics-flag branch April 17, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants