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

Support flux managed clusters #1364

Merged
merged 9 commits into from
Sep 19, 2023
Merged

Conversation

QuentinBisson
Copy link
Contributor

@QuentinBisson QuentinBisson commented Aug 30, 2023

Towards: giantswarm/roadmap#2809

To connect to the api server of CAPI Clusters, prometheus-meta-operator is copying the kubeconfig for the cluster (certificate and private key) to be able to establish a TLS connection.

For Flux managed clusters described in this issue giantswarm/roadmap#2670, the kubeconfig contains a token and not a certificate/private key pair so we need to adjust prometheus-meta-operator to be able to configure the Prometheus to use a token to connect to the API server instead of the certificates.

This PR does just that

Checklist

I have:

  • Described why this change is being introduced
  • Separated out refactoring/reformatting in a dedicated PR
  • Updated changelog in CHANGELOG.md

@QuentinBisson QuentinBisson self-assigned this Aug 30, 2023
@QuentinBisson QuentinBisson requested a review from a team as a code owner August 30, 2023 14:47
@QuentinBisson QuentinBisson force-pushed the support-flux-managed-clusters branch 8 times, most recently from 591119b to 78b34b6 Compare August 31, 2023 10:14
@QuentinBisson
Copy link
Contributor Author

Tested on Snail, working fine :)

@@ -1,10 +1,15 @@
[[- define "_apiserver" -]]
[[- if ne .ClusterType "management_cluster" ]]
api_server: https://[[ .APIServerURL ]]
[[- if eq .AuthenticationType "token" ]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a new field named authentication type that explains if the tls config needs to use a token or a cert/key pair

@@ -5,6 +5,8 @@ metadata:
labels:
{{- include "labels.common" . | nindent 4 }}
name: alertmanager-psp
annotations:
seccomp.security.alpha.kubernetes.io/allowedProfileNames: runtime/default
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a missing seccomp annotation that causes issue on snail

bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token
[[ end ]]
Copy link
Contributor

Choose a reason for hiding this comment

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

why these 3 above lines are not embedded in the _apiserver section ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because etcd is a bit special, especially on vintage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the apiserver secion is only for workload clusters because prometheus in the MC have access to the api server "locally"

Copy link
Contributor

Choose a reason for hiding this comment

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

On MC there is always a file with the token ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be yes

QuentinBisson and others added 6 commits September 14, 2023 14:15
Signed-off-by: QuentinBisson <[email protected]>
Signed-off-by: QuentinBisson <[email protected]>
* Change GetOrganization to rely on the namespace

* Rely on namespace label for organization

---------

Co-authored-by: Mohamed Chiheb <[email protected]>
Co-authored-by: QuentinBisson <[email protected]>
* Bump alertmanager and prometheus versions

* Release v4.47.0

---------

Co-authored-by: QuentinBisson <[email protected]>
Signed-off-by: QuentinBisson <[email protected]>
Signed-off-by: QuentinBisson <[email protected]>
@QuentinBisson QuentinBisson force-pushed the support-flux-managed-clusters branch 4 times, most recently from 075d918 to 6f016aa Compare September 14, 2023 12:29
@QuentinBisson QuentinBisson force-pushed the support-flux-managed-clusters branch from 6f016aa to cc829af Compare September 14, 2023 12:32
} else if (secret.Data["crt"] != nil && len(secret.Data["crt"]) > 0) && (secret.Data["key"] != nil && len(secret.Data["key"]) > 0) {
return "certificates", nil
}
return "vintage", nil
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "vintage" mean ?
I see no use case with this authentication type ?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it means the vintage authentication mechanism (for KVM). It's not used but i wanted to avoid setting an empty string here

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment please :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it into an error :)

Signed-off-by: QuentinBisson <[email protected]>
@QuentinBisson QuentinBisson force-pushed the support-flux-managed-clusters branch 6 times, most recently from 5881dea to 4711752 Compare September 18, 2023 12:26
@QuentinBisson QuentinBisson force-pushed the support-flux-managed-clusters branch from 4711752 to ab43a27 Compare September 18, 2023 12:29
@QuentinBisson
Copy link
Contributor Author

@giantswarm/team-atlas The changes should be easier to review :)

@QuentinBisson
Copy link
Contributor Author

Tested on gauss, snail and grizzly

@QuentinBisson QuentinBisson merged commit c968583 into master Sep 19, 2023
2 checks passed
@QuentinBisson QuentinBisson deleted the support-flux-managed-clusters branch September 19, 2023 12:19
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.

4 participants