-
Notifications
You must be signed in to change notification settings - Fork 26
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
NETOBSERV-1070: Fix reconcile from DISABLED to AUTO configuration #365
Conversation
@OlivierCazade: This pull request references NETOBSERV-1070 which is a valid jira issue. In response to this:
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. |
Codecov Report
@@ Coverage Diff @@
## main #365 +/- ##
==========================================
- Coverage 53.53% 53.37% -0.17%
==========================================
Files 45 45
Lines 5329 5349 +20
==========================================
+ Hits 2853 2855 +2
- Misses 2272 2288 +16
- Partials 204 206 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
flpServiceMonitorObject.Spec.Endpoints[0].TLSConfig = &monitoringv1.TLSConfig{ | ||
SafeTLSConfig: monitoringv1.SafeTLSConfig{ | ||
ServerName: serverName, | ||
InsecureSkipVerify: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is where you plan a follow-up? did you already create the jira ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have multiple case here that should be addressed:
- the certificate is a valid certificate signed by a public authority
- the user provide a CA (we need to add a field for that) certificate and we must validate the certificate with this CA
- the user want to skip verification and we should have an option to configure this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
can you just open a follow-up bug ticket if it's not already done? (for the insecure-verify)
/approve @jotak the ticket is here: |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OlivierCazade 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 |
Two bug correction here:
Service annotations not being updated during reconcile, this had two effects:
ServiceMonitor now adapt to the TLS configuration