-
Notifications
You must be signed in to change notification settings - Fork 357
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
MON-3814: Follow-up #2293 #2315
base: master
Are you sure you want to change the base?
Conversation
@slashpai: This pull request references MON-3814 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: slashpai 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 |
/assign @machine424 |
pkg/manifests/manifests.go
Outdated
@@ -2106,6 +2106,8 @@ func (f *Factory) MetricsServerDeployment(apiAuthSecretName string, kubeletCABun | |||
"--requestheader-extra-headers-prefix="+requestheaderExtraHeadersPrefix, | |||
"--requestheader-group-headers="+requestheaderGroupHeaders, | |||
"--requestheader-username-headers="+requestheaderUsernameHeaders, | |||
"--tls-cert-file=/etc/client-ca-bundle/tls.crt", |
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.
No test was failing when using /etc/tls/private/xxx
, do they have the same content? Is testing this difficult?
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.
Thinking about it again. I think what we need here is cleanup as we are using it in volume mount
https://github.com/openshift/cluster-monitoring-operator/pull/2293/files#r1557090512
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.
'service.beta.openshift.io/serving-cert-secret-name': 'metrics-server-tls', |
name: 'secret-metrics-server-tls', |
In the updated approach as well it uses the same secret https://github.com/openshift/cluster-monitoring-operator/pull/2293/files#diff-0bbbc0b6e7ee4c4efe218030b64c2ef604fda58d92e51e52c206b6c5c0a4a5a3R158
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.
I updated the PR to remove unused in volume mounts https://github.com/openshift/cluster-monitoring-operator/pull/2293/files#r1557090512
Signed-off-by: Jayapriya Pai <janantha@redhat.com>
6ad247b
to
0c71e7c
Compare
@@ -2162,16 +2162,14 @@ func (f *Factory) MetricsServerSecret(tlsSecret *v1.Secret, apiAuthConfigmap *v1 | |||
var ( | |||
clientCA = r.value("client-ca-file") | |||
requestheaderClientCA = r.value("requestheader-client-ca-file") | |||
tlsCA = r.value("tls.crt") | |||
tlsKey = r.value("tls.key") | |||
) |
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.
removing these as it not used and we mount this in assets already
cluster-monitoring-operator/jsonnet/components/metrics-server.libsonnet
Lines 199 to 200 in 889bb2c
'--tls-cert-file=/etc/tls/private/tls.crt', | |
'--tls-private-key-file=/etc/tls/private/tls.key', |
Follow-up https://github.com/openshift/cluster-monitoring-operator/pull/2293/files#r1557090512
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.
True, and I believe the tlsSecret
can be dropped now.
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.
(i just realized we didin't need that wait. as we already waited for that secret here
cluster-monitoring-operator/pkg/tasks/metricsserver.go
Lines 122 to 128 in 65ee646
scas, err := t.client.WaitForSecretByNsName( | |
ctx, | |
types.NamespacedName{ | |
Namespace: s.Namespace, | |
Name: s.Annotations["service.beta.openshift.io/serving-cert-secret-name"], | |
}, | |
) |
Closing in favour of #2329 |
/close |
@slashpai: Closed this PR. 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. |
Because we put #2329 on hold for now, we can revive this PR if you want. |
I'll |
@machine424: Reopened this PR. 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-sigs/prow repository. |
@slashpai: This pull request references MON-3814 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
I think we can merge this once |
/hold |
@slashpai: 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. |
Follow-up from #2293