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

MON-3749: enable request headers flags for metrics server #2293

Merged
merged 2 commits into from Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 89 additions & 2 deletions pkg/manifests/manifests.go
Expand Up @@ -2050,7 +2050,7 @@ func (f *Factory) MetricsServerRoleBindingAuthReader() (*rbacv1.RoleBinding, err
return f.NewRoleBinding(f.assets.MustNewAssetSlice(MetricsServerRoleBindingAuthReader))
}

func (f *Factory) MetricsServerDeployment(kubeletCABundle *v1.ConfigMap, tlsSecret, metricsClientCert *v1.Secret) (*appsv1.Deployment, error) {
func (f *Factory) MetricsServerDeployment(apiAuthSecretName string, kubeletCABundle *v1.ConfigMap, servingCASecret, metricsClientCert *v1.Secret, requestheader map[string]string) (*appsv1.Deployment, error) {
slashpai marked this conversation as resolved.
Show resolved Hide resolved
dep, err := f.NewDeployment(f.assets.MustNewAssetSlice(MetricsServerDeployment))
if err != nil {
return nil, err
Expand All @@ -2075,7 +2075,7 @@ func (f *Factory) MetricsServerDeployment(kubeletCABundle *v1.ConfigMap, tlsSecr
// Hash the TLS secret and propagate it as a annotation to the
// deployment's pods to trigger a new rollout when the TLS certificate/key
// are rotated.
dep.Spec.Template.Annotations["monitoring.openshift.io/serving-ca-secret-hash"] = hashByteMap(tlsSecret.Data)
dep.Spec.Template.Annotations["monitoring.openshift.io/serving-ca-secret-hash"] = hashByteMap(servingCASecret.Data)

// Hash the metrics client cert and propagate it as a annotation to the
// deployment's pods to trigger a new rollout when the metrics client cert
Expand All @@ -2087,6 +2087,36 @@ func (f *Factory) MetricsServerDeployment(kubeletCABundle *v1.ConfigMap, tlsSecr
return dep, nil
}

r := newErrMapReader(requestheader)
slashpai marked this conversation as resolved.
Show resolved Hide resolved

var (
requestheaderAllowedNames = strings.Join(r.slice("requestheader-allowed-names"), ",")
requestheaderExtraHeadersPrefix = strings.Join(r.slice("requestheader-extra-headers-prefix"), ",")
requestheaderGroupHeaders = strings.Join(r.slice("requestheader-group-headers"), ",")
requestheaderUsernameHeaders = strings.Join(r.slice("requestheader-username-headers"), ",")
)

if r.Error() != nil {
return nil, fmt.Errorf("value not found in extension api server authentication configmap: %w", r.err)
}

containers[idx].Args = append(containers[idx].Args,
"--client-ca-file=/etc/client-ca-bundle/client-ca-file",
"--requestheader-client-ca-file=/etc/client-ca-bundle/requestheader-client-ca-file",
"--requestheader-allowed-names="+requestheaderAllowedNames,
"--requestheader-extra-headers-prefix="+requestheaderExtraHeadersPrefix,
"--requestheader-group-headers="+requestheaderGroupHeaders,
"--requestheader-username-headers="+requestheaderUsernameHeaders,
)

podSpec.Containers[0].VolumeMounts = append(podSpec.Containers[0].VolumeMounts,
v1.VolumeMount{
Name: "client-ca-bundle",
ReadOnly: true,
MountPath: "/etc/client-ca-bundle",
},
)

if err := validateAuditProfile(config.Audit.Profile); err != nil {
return nil, err
}
Expand All @@ -2100,6 +2130,17 @@ func (f *Factory) MetricsServerDeployment(kubeletCABundle *v1.ConfigMap, tlsSecr
"--audit-log-compress=true",
)

podSpec.Volumes = append(podSpec.Volumes,
v1.Volume{
Name: "client-ca-bundle",
VolumeSource: v1.VolumeSource{
Secret: &v1.SecretVolumeSource{
SecretName: apiAuthSecretName,
},
},
},
)

if len(config.NodeSelector) > 0 {
podSpec.NodeSelector = config.NodeSelector
}
Expand All @@ -2119,6 +2160,52 @@ func (f *Factory) MetricsServerDeployment(kubeletCABundle *v1.ConfigMap, tlsSecr
return dep, nil
}

func (f *Factory) MetricsServerSecret(tlsSecret *v1.Secret, apiAuthConfigmap *v1.ConfigMap) (*v1.Secret, error) {
data := make(map[string]string)

for k, v := range tlsSecret.Data {
data[k] = string(v)
}

for k, v := range apiAuthConfigmap.Data {
data[k] = v
}

r := newErrMapReader(data)

var (
clientCA = r.value("client-ca-file")
requestheaderClientCA = r.value("requestheader-client-ca-file")
tlsCA = r.value("tls.crt")
tlsKey = r.value("tls.key")
)

if r.Error() != nil {
return nil, fmt.Errorf("value not found in extension api server authentication configmap: %w", r.err)
}

h := fnv.New64()
h.Write([]byte(clientCA + requestheaderClientCA + tlsCA + tlsKey))
hash := strconv.FormatUint(h.Sum64(), 32)

return &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: f.namespace,
Name: fmt.Sprintf("metrics-server-%s", hash),
Labels: map[string]string{
"monitoring.openshift.io/name": "metrics-server",
"monitoring.openshift.io/hash": hash,
},
},
Data: map[string][]byte{
"client-ca-file": []byte(clientCA),
"requestheader-client-ca-file": []byte(requestheaderClientCA),
"tls.crt": []byte(tlsCA),
"tls.key": []byte(tlsKey),
Comment on lines +2203 to +2204
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where we mount/set these two.

Copy link
Member

Choose a reason for hiding this comment

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

- mountPath: /etc/tls/private
name: secret-metrics-server-tls

},
}, nil
}

func (f *Factory) MetricsServerPodDisruptionBudget() (*policyv1.PodDisruptionBudget, error) {
return f.NewPodDisruptionBudget(f.assets.MustNewAssetSlice(MetricsServerPodDisruptionBudget))
}
Expand Down
17 changes: 14 additions & 3 deletions pkg/manifests/manifests_test.go
Expand Up @@ -2814,7 +2814,13 @@ metricsServer:
"tls.key": []byte("foo"),
},
}
d, err := f.MetricsServerDeployment(kubeletCABundle, servingCASecret, metricsClientSecret)
apiAuthConfigMapData := map[string]string{
"requestheader-allowed-names": "",
"requestheader-extra-headers-prefix": "",
"requestheader-group-headers": "",
"requestheader-username-headers": "",
}
d, err := f.MetricsServerDeployment("foo", kubeletCABundle, servingCASecret, metricsClientSecret, apiAuthConfigMapData)
slashpai marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -2969,8 +2975,13 @@ metricsServer:
"tls.key": []byte("foo"),
},
}

d, err := f.MetricsServerDeployment(kubeletCABundle, servingCASecret, metricsClientSecret)
apiAuthConfigMapData := map[string]string{
"requestheader-allowed-names": "",
"requestheader-extra-headers-prefix": "",
"requestheader-group-headers": "",
"requestheader-username-headers": "",
}
d, err := f.MetricsServerDeployment("foo", kubeletCABundle, servingCASecret, metricsClientSecret, apiAuthConfigMapData)
slashpai marked this conversation as resolved.
Show resolved Hide resolved

if test.err != nil || err != nil {
// fail only if the error isn't what is expected
Expand Down
46 changes: 45 additions & 1 deletion pkg/tasks/metricsserver.go
Expand Up @@ -155,7 +155,32 @@ func (t *MetricsServerTask) create(ctx context.Context) error {
}
}

dep, err := t.factory.MetricsServerDeployment(cacm, scas, mcs)
tlsSecret, err := t.client.WaitForSecretByNsName(ctx, types.NamespacedName{Namespace: t.namespace, Name: "metrics-server-tls"})
if err != nil {
return fmt.Errorf("failed to wait for metrics-server-tls secret: %w", err)
}

apiAuthConfigmap, err := t.client.WaitForConfigMapByNsName(ctx, types.NamespacedName{Namespace: "kube-system", Name: "extension-apiserver-authentication"})
if err != nil {
return fmt.Errorf("failed to wait for kube-system/extension-apiserver-authentication configmap: %w", err)
}

secret, err := t.factory.MetricsServerSecret(tlsSecret, apiAuthConfigmap)
if err != nil {
return fmt.Errorf("failed to create metrics-server secret: %w", err)
}

err = t.deleteOldMetricsServerSecrets(secret.Labels["monitoring.openshift.io/hash"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to the PR: Couldn't this be replaced by a hash on the Deployment itself as we do for the certs?
So we have only one way of "reloading the Deployment if the secret changes".
(I'd create a ticket for that)
(cc @simonpasquier in case you know that this logic is needed for other things than reloading)

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure either why this was there for prometheus-adapter as well. Happy to move to similar logic we have for other secrets if this is not needed

Copy link
Member

Choose a reason for hiding this comment

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

+1 on doing that since we already use the other logic within MS.

if err != nil {
return fmt.Errorf("deleting old metrics-server secrets failed: %w", err)
}

err = t.client.CreateOrUpdateSecret(ctx, secret)
if err != nil {
return fmt.Errorf("reconciling MetricsServer Secret failed: %w", err)
}

dep, err := t.factory.MetricsServerDeployment(secret.Name, cacm, scas, mcs, apiAuthConfigmap.Data)
if err != nil {
return fmt.Errorf("initializing MetricsServer Deployment failed: %w", err)
}
Expand Down Expand Up @@ -258,3 +283,22 @@ func (t *MetricsServerTask) removePrometheusAdapterResources(ctx context.Context
// TODO(slashpai): Add steps to remove other resources if any
return nil
}

func (t *MetricsServerTask) deleteOldMetricsServerSecrets(newHash string) error {
secrets, err := t.client.KubernetesInterface().CoreV1().Secrets(t.namespace).List(t.ctx, metav1.ListOptions{
LabelSelector: "monitoring.openshift.io/name=metrics-server,monitoring.openshift.io/hash!=" + newHash,
})

if err != nil {
return fmt.Errorf("error listing metrics-server secrets: %w", err)
}

for i := range secrets.Items {
err := t.client.KubernetesInterface().CoreV1().Secrets(t.namespace).Delete(t.ctx, secrets.Items[i].Name, metav1.DeleteOptions{})
if err != nil {
return fmt.Errorf("error deleting secret: %s: %w", secrets.Items[i].Name, err)
}
}

return nil
}