Conversation
|
Tested on graveler |
QuentinBisson
force-pushed
the
add-crossplane-aws
branch
from
b7d71a8 to
a3fc678
Compare
| Get AWS Account ID from AWSCluster identity | ||
| Supports both AWSClusterRoleIdentity and AWSClusterControllerIdentity | ||
| */}} | ||
| {{- define "grafana.crossplane.aws.accountId" -}} |
There was a problem hiding this comment.
Can't we template those variable from values rather than looking them up within the cluster ? This is not a very common pattern across our platform plus it is complex and hard to read.
There was a problem hiding this comment.
I did that for the others so theoretically yes we could but having to define this value in multiple places does not make sense to me considering this will always run in CAPI MCs
There was a problem hiding this comment.
I also don't like the fact that we create a dependency on another object here, whenever this object being lookup change we have to change this. Is there a shared/default config we can use instead of this ?
There was a problem hiding this comment.
There might be yes, but also, we are dependent on crossplane and a lot of other components anyway.
I'm not sure the oidcProvider is defined in shared configs though @giantswarm/team-phoenix can you let us know?
helm/grafana/templates/cnpg/crossplane/aws/bucket-public-access-block.yaml
Show resolved
Hide resolved
|
@TheoBrigitte I added comments on each files |
| {{- if $identity -}} | ||
| {{- /* Extract account ID from roleARN like arn:aws:iam::758407694730:role/... */ -}} | ||
| {{- $roleARN := $identity.spec.roleARN -}} | ||
| {{- $parts := regexSplit "::" $roleARN -1 -}} |
There was a problem hiding this comment.
ARNs cannot be expected to contain ::. They're made up of values joined by a single colon, and the zero-based index 4 would give you the account ID. See existing code.
There's also no need to implement AWSClusterControllerIdentity lookup at all since we don't support this kind in cluster-aws.
There was a problem hiding this comment.
Then in that case, can we find those values in the configs repos? Because the central place for configuration is shared-configs, yet more and more things are being set in other places
There was a problem hiding this comment.
Also, I agree about the lookup but then where can we collect the Cluster Tags to have it applied to the MC crossplane resources?
| {{- $bucketName := .Values.postgresqlCluster.crossplane.aws.bucket.name }} | ||
| {{- $namespace := .Release.Namespace }} | ||
| {{- $tags := include "grafana.crossplane.tags" . | fromYaml }} | ||
| {{- $oidcProvider := include "grafana.crossplane.aws.oidcProvider" . }} |
There was a problem hiding this comment.
We should use our existing ConfigMap for Crossplane-related resources to read the plural OIDC providers list:
$ kubectl get -n org-myorg cm mycluster-crossplane-config -o yaml
apiVersion: v1
data:
values: |
accountID: "150076092023"
awsCluster:
securityGroups:
controlPlane:
id: sg-04f62403141ca0fce
node:
id: sg-0ab5bf55fdf285cbc
vpcId: vpc-0fda586a4105f1148
awsPartition: aws
baseDomain: mycluster.gaws2.gigantic.io
clusterName: mycluster
oidcDomain: irsa.mycluster.gaws2.gigantic.io
oidcDomains:
- irsa.mycluster.gaws2.gigantic.io
region: eu-north-1
[...]
In default apps within cluster-aws, such as aws-nth-bundle, we pull the config map in directly (see here). As Theo commented, that would be a more direct way than lookup (which cannot easily be tested in CI).
Ensure you support multiple URLs since migrated Vintage AWS clusters may still have the old and new OIDC provider.
There was a problem hiding this comment.
The issue is that this app is managed by collections so it should not have to mount this config right? Can we defined it in shared configs instead?
There was a problem hiding this comment.
Can you refer to where grafana-app gets deployed, specifically? Is it via capa-app-collection/kustomize/observability-platform-api.yaml or so? I'm trying to figure out how we can add the relevant config.
There was a problem hiding this comment.
@AndiDog it is now deployed from https://github.com/giantswarm/management-cluster-bases/blob/convert-to-hr-tempo/bases/collections/shared/base/grafana.yaml
FYI, I'm moving forward here because I will have to change the other apps as well anyway but I have no issues improving it with what you find :)
3 participants
Towards https://github.com/giantswarm/giantswarm/issues/35338
This pull request introduces automated S3 bucket provisioning for Grafana Postgresql Backups on AWS (CAPA) clusters using Crossplane, along with supporting changes to the Helm chart and schema. The main focus is on enabling dynamic, configurable, and optionally managed storage infrastructure, including IAM role management and lifecycle policies, with support for both observe-only and full-management modes. Several helper templates and schema extensions are added to facilitate this functionality.