Make ShadowLink sourceCluster partially mutable #1160
Conversation
andrewstucki
requested review from
RafalKorepta,
chrisseto and
gene-redpanda
as code owners
|
Looks like last week the fields we're intentionally considering immutable were made mutable: redpanda-data/redpanda#27986 -- looks like I'll need to update this. |
chrisseto
left a comment
chrisseto
left a comment
There was a problem hiding this comment.
Blocking on the validation rules (Pretty sure I've had enough coffee this morning...)
| var ErrUnsupportedSASLMechanism = errors.New("unsupported SASL mechanism") | ||
|
|
||
| // KafkaAPISpec configures client configuration settings for connecting to Redpanda brokers. | ||
| // +kubebuilder:validation:XValidation:rule="has(self.tls) == has(oldSelf.tls)",message="kafka tls settings are immutable" |
There was a problem hiding this comment.
Wouldn't the reasoning for making authentication mutable also apply to TLS settings? e.g. what if I rotate my certs or fat finger a certificate name?
On the whole, I'm wonder if we should allow cluster source (Or at least the static configuration part) to be fully mutable for shadow links as shadowlinks themselves will perform the appropriate checks to make sure the underlying cluster hasn't changed.
There was a problem hiding this comment.
Yeah, so previously TLS and bootstrap servers were explicitly immutable in core. Now they aren't, so this should change.
That said, we still need to keep the target ShadowCluster immutable for the same reasons that we keep the other CRD ClusterSources immutable, we can only guarantee cleanup and consistency if we ensure you're referencing the same cluster between reconciliations.
There was a problem hiding this comment.
Makes sense! Just to summarize: StaticConfig can generally be mutable (No strong opinion on brokers list) but cluster references should be immutable.
Any opinion on applying that to Topics/Schemas/Etc?
There was a problem hiding this comment.
no real opinions initially, I kept them as immutable for now, because:
- that's how it's been
- this allows us to still clean things up properly in targeted clusters
The reason that the SourceCluster is fine being mutable in ShadowLinks is that basically the parameters are just used to initialize a connection in the ShadowCluster, there's basically nothing in the SourceCluster that actually knows about the ShadowCluster and it's treated as a read-only source. All of the configuration writes actually happen inside of the ShadowCluster so we can still clean that properly if we treat that as immutable.
|
This PR is stale because it has been open 5 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
This moves around CEL-based validations for various
ClusterSourcefields so thatSourceClusterfor ShadowLinks is partially mutable, mainly in order to allow for a shadow link's authentication block to its source cluster to be modified. OnShadowClusterand any other CRDs where we're creating a resource in the cluster, theClusterSourceis still fully immutable, additionally, TLS-settings and bootstrap servers ofSourceClusterare immutable per the core implementation.