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

WINC-1269: Expose WMCO /metrics endpoint via HTTPS #2388

Merged
merged 4 commits into from
Sep 5, 2024
Merged

WINC-1269: Expose WMCO /metrics endpoint via HTTPS #2388

merged 4 commits into from
Sep 5, 2024

Conversation

mansikulkarni96
Copy link
Member

@mansikulkarni96 mansikulkarni96 commented Aug 20, 2024 •
edited
Loading

Use the new controller-runtime secureAccess flag and filters.WithAuthenticationAndAuthorization when exposing metrics endpoint.
kube-rbac-proxy has been removed from controller-runtime scaffolding and its use is discouraged to drop the dependency on maintaining the image.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 20, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 20, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2024
@mansikulkarni96
Copy link
Member Author

/approve cancel

@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2024
@mansikulkarni96 mansikulkarni96 changed the title WIP: Expose WMCO /metrics endpoint via HTTPS WINC-1269: Expose WMCO /metrics endpoint via HTTPS Aug 22, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 22, 2024 •
edited by openshift-ci bot
Loading

@mansikulkarni96: This pull request references WINC-1269 which is a valid jira issue.

In response to this:

Use secureAccess option when exposing metrics
endpoint and mount the TLS certs created
using TLS secret annotation to the WMCO pod.
The cert and the key will be used for
server authentication.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 22, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 22, 2024 •
edited by openshift-ci bot
Loading

@mansikulkarni96: This pull request references WINC-1269 which is a valid jira issue.

In response to this:

Use the new controller-runtime secureAccess flag and filters.WithAuthenticationAndAuthorization when exposing metrics endpoint.
kube-rbac-proxy has been removed from controller-runtime scaffolding and its use is discouraged to drop the dependency on maintaining the image.

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 openshift-eng/jira-lifecycle-plugin repository.

@mansikulkarni96
Copy link
Member Author

/test aws-e2e-operator

@mansikulkarni96
Copy link
Member Author

WMCO logs
2024-08-22T21:39:04Z INFO controller-runtime.metrics Serving metrics server {"bindAddress": "0.0.0.0:8443", "secure": true}

@wgahnagl
Copy link
Contributor

I think you might've pushed some vendor changes. Should those be there?

saifshaikh48
saifshaikh48 reviewed Aug 26, 2024
View reviewed changes
Copy link
Contributor

@saifshaikh48 saifshaikh48 left a comment

Choose a reason for hiding this comment

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

With this are we able to remove the kube-rbac-proxy deployment (manager_auth_proxy_patch.yaml config file)? I know it's not being used but not sure if it a generated file or a resource that we maintain

@mansikulkarni96
Copy link
Member Author

I think you might've pushed some vendor changes. Should those be there?

@wgahnagl they are generated with make vendor for the import sigs.k8s.io/controller-runtime/pkg/metrics/filters

@mansikulkarni96
Copy link
Member Author

With this are we able to remove the kube-rbac-proxy deployment (manager_auth_proxy_patch.yaml config file)? I know it's not being used but not sure if it a generated file or a resource that we maintain

We are not using it, but we can consider removing it. It came with the Kubebuilder project scaffolding.

@jrvaldes
Copy link
Contributor

With this are we able to remove the kube-rbac-proxy deployment (manager_auth_proxy_patch.yaml config file)? I know it's not being used but not sure if it a generated file or a resource that we maintain

We are not using it, but we can consider removing it. It came with the Kubebuilder project scaffolding.

+1 to removing it as part of this PR

jrvaldes
jrvaldes reviewed Aug 26, 2024
View reviewed changes
Copy link
Contributor

@jrvaldes jrvaldes left a comment

Choose a reason for hiding this comment

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

@mansikulkarni96 thanks for working on this. Mostly LGTM, left minor suggestions. PTAL

cmd/operator/main.go
@@ -156,7 +160,9 @@ func main() {
mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
Scheme: scheme,
Metrics: metricsserver.Options{
BindAddress: fmt.Sprintf("%s:%d", metrics.Host, metrics.Port),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 9182 port is no longer relevant?

https://github.com/openshift/windows-machine-config-operator/blob/master/pkg/metrics/metrics.go#L46

Copy link
Member Author

Choose a reason for hiding this comment

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

That is used by windows-exporter and is secured by https now, this is used by metrics server. But I think we can keep it the same as 9182 can also be a secure port, I ll update.

cmd/operator/main.go Outdated

flag.BoolVar(&debugLogging, "debugLogging", false, "Log debug messages")
flag.StringVar(&metricsAddr, "metrics-bind-address", "0.0.0.0:8443",
"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.

Please mention the proposed default (8443) in the argument description.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit, if you still want to add this

config/manager/manager.yaml Outdated
image: controller:latest
name: manager
imagePullPolicy: IfNotPresent
ports:
- containerPort: 8443
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the port is hard-coded in the manifest; What is the use case of the proposed flag metrics-bind-address ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The flag is already available through controller-runtime's server package, the intention is to avoid hardcoding in code and pass it through the CSV.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mansikulkarni96

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

10 similar comments
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3993fb9 and 2 for PR HEAD d589849 in total

@mansikulkarni96
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2024
@mansikulkarni96
Copy link
Member Author

/test azure-e2e-operator

@mansikulkarni96
Copy link
Member Author

/test azure-e2e-upgrade

@mansikulkarni96
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2024
@mansikulkarni96
Copy link
Member Author

/override ci/prow/azure-e2e-upgrade
test passed de-provision timed out

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Sep 5, 2024

@mansikulkarni96: Overrode contexts on behalf of mansikulkarni96: ci/prow/azure-e2e-upgrade

In response to this:

/override ci/prow/azure-e2e-upgrade
test passed de-provision timed out

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-sigs/prow repository.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Sep 5, 2024

@mansikulkarni96: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit d8fdad4 into openshift:master Sep 5, 2024
21 checks passed
@mansikulkarni96
Copy link
Member Author

/cherry-pick release-4.17

@openshift-cherrypick-robot
Copy link

@mansikulkarni96: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked.

Error messagestatus code 406 not one of [200], body: {"message":"Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead.","errors":[{"resource":"PullRequest","field":"diff","code":"too_large"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#list-pull-requests-files","status":"406"}

In response to this:

/cherry-pick release-4.17

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-sigs/prow repository.

@mansikulkarni96
Copy link
Member Author

/cherry-pick release-4.17

@openshift-cherrypick-robot
Copy link

@mansikulkarni96: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked.

Error messagestatus code 406 not one of [200], body: {"message":"Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead.","errors":[{"resource":"PullRequest","field":"diff","code":"too_large"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#list-pull-requests-files","status":"406"}

In response to this:

/cherry-pick release-4.17

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-sigs/prow repository.

@mansikulkarni96
Copy link
Member Author

/cherry-pick release-4.17

@openshift-cherrypick-robot
Copy link

@mansikulkarni96: Failed to get PR patch from GitHub. This PR will need to be manually cherrypicked.

Error messagestatus code 406 not one of [200], body: {"message":"Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead.","errors":[{"resource":"PullRequest","field":"diff","code":"too_large"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#list-pull-requests-files","status":"406"}

In response to this:

/cherry-pick release-4.17

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-sigs/prow repository.

@mansikulkarni96 mansikulkarni96 mentioned this pull request Sep 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saifshaikh48 saifshaikh48 saifshaikh48 left review comments

@jrvaldes jrvaldes Awaiting requested review from jrvaldes

@sebsoto sebsoto Awaiting requested review from sebsoto

Assignees

@saifshaikh48 saifshaikh48

@jrvaldes jrvaldes

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mansikulkarni96 @openshift-ci-robot @wgahnagl @jrvaldes @saifshaikh48 @openshift-cherrypick-robot @openshift-merge-robot