bootstrapping secrets from management cluster to leaf clusters #18
Conversation
ahsayde
force-pushed
the
secret-sync-experiment
branch
from
e4282e7 to
785cb93
Compare
ahsayde
changed the title
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| type SecretSyncSpec struct { |
There was a problem hiding this comment.
Can we document these?
This has a direct impact on the output from the CRD (and the documentation generated from the CRD).
See https://github.com/fluxcd/source-controller/blob/main/api/v1beta2/gitrepository_types.go for a great example.
api/v1alpha2/secretsync_types.go
Outdated
| } | ||
|
|
||
| // SetClusterSecretVersion set secret's ResourceVersion | ||
| func (s *SecretSyncStatus) SetClusterSecretVersion(cluster, secret, version string) { |
There was a problem hiding this comment.
Maybe we could document what these parameters are for?
What is "cluster" and "secret" in this case?
Given that they're repeated in the parameters below, maybe they are a DataClump ?
There was a problem hiding this comment.
i found that we don't need to use the secret name in the key since the CR reference only one secret
so the key will be just the cluster name
controllers/secretsync_controller.go
Outdated
| //+kubebuilder:rbac:groups=capi.weave.works,resources=secretsyncs,verbs=get;list;watch;create;update;patch;delete | ||
| //+kubebuilder:rbac:groups=capi.weave.works,resources=secretsyncs/status,verbs=get;update;patch | ||
| //+kubebuilder:rbac:groups=capi.weave.works,resources=secretsyncs/finalizers,verbs=update | ||
| //+kubebuilder:rbac:groups=batch,resources=jobs,verbs=get;list;watch;create;update;patch;delete |
There was a problem hiding this comment.
Do you need the permissions on jobs here?
|
|
||
| if !cluster.DeletionTimestamp.IsZero() { | ||
| logger.Info("skipping cluster", "cluster", cluster.Name, "namespace", req.Namespace, "reason", "Deleted") | ||
| continue |
There was a problem hiding this comment.
Great logging here!
One thing to be careful of, is that the context logger already has namespace as a key?
I think in this case it's fine, because the namespaces are the same, but something to be aware of, we could be losing data here.
controllers/secretsync_controller.go
Outdated
| secretSync.Status.SetClusterSecretVersion(cluster.Name, secret.Name, secret.ResourceVersion) | ||
| } | ||
|
|
||
| if err := r.Status().Patch(ctx, &secretSync, patch); err != nil { |
There was a problem hiding this comment.
Since the original bootstrapper was written, Flux has added the Patch Helper
https://github.com/fluxcd/pkg/blob/main/runtime/patch/patch.go#L63
This simplifies things a bit (it will retry changes).
controllers/secretsync_controller.go
Outdated
| if err := cl.Create(ctx, &newSecret); err != nil { | ||
| if apierrors.IsAlreadyExists(err) { | ||
| if err := cl.Update(ctx, &newSecret); err != nil { | ||
| return fmt.Errorf("failed to update secret %w", err) |
There was a problem hiding this comment.
We should try and standardise the logging output
You have https://github.com/weaveworks/cluster-bootstrap-controller/pull/18/files#diff-9eed6d96f1d71606048d5b4a5652b2b150abef0bb061c7c74fe49a104ab25d42R100 and
https://github.com/weaveworks/cluster-bootstrap-controller/pull/18/files#diff-9eed6d96f1d71606048d5b4a5652b2b150abef0bb061c7c74fe49a104ab25d42R110 and
https://github.com/weaveworks/cluster-bootstrap-controller/pull/18/files#diff-9eed6d96f1d71606048d5b4a5652b2b150abef0bb061c7c74fe49a104ab25d42R225
How about failed to ...: %w" with any additional data before the : ?
api/v1alpha2/secretsync_types.go
Outdated
| // TargetNamespace specifies the namespace which the secret should be bootstrapped in | ||
| // The default value is the namespace of the referenced secret | ||
| //+optional | ||
| TargetNamespace string `json:"targetNamespace"` |
api/v1alpha2/secretsync_types.go
Outdated
| type SecretSyncSpec struct { | ||
| ClusterSelector metav1.LabelSelector `json:"clusterSelector"` | ||
| SecretRef v1.LocalObjectReference `json:"secretRef"` | ||
| // ClusterSelector specifies the label selector to match clusters with |
There was a problem hiding this comment.
How about something like this?
// Label selector for Clusters. The Clusters that are
// selected by this will be the ones affected by this SecretSync.
// It must match the Cluster labels. This field is immutable.
// Label selector cannot be empty.
ClusterSelector metav1.LabelSelector `json:"clusterSelector"`
controllers/secretsync_controller.go
Outdated
| return ctrl.Result{}, nil | ||
| } | ||
| if err := patchHelper.Patch(ctx, &secretSync); err != nil { | ||
| return ctrl.Result{}, err |
There was a problem hiding this comment.
Is there any more information we can wrap this error in?
See weaveworks/weave-gitops-enterprise#2331
Bootstrapping secret from management cluster to leaf clusters
To do
see https://github.com/weaveworks/weave-gitops-private/pull/101/files#diff-b3fa087fff01a7268952b24df072a5163b7cfa0aa295216b2d736b6e2da31a11R297-R354