Repository navigation
Conversation
c3e3121 to
3be4832
Compare
3be4832 to
7cb677e
Compare
labrenbe
changed the title
586debd to
22536cd
Compare
labrenbe
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
| Retain, | ||
|
|
||
| /// Delete the resource in the product. | ||
| Delete, |
There was a problem hiding this comment.
Question regarding a specific scenario: I guess it would be possible for someone to create a KafkaTopic object with a Kafka topic they theoretically shouldn't have permissions for and then just delete that object, in turn deleting that Kafka topic in the cluster. Would that be covered with authorization somehow? That scenario would be relevant for different user groups managing different topics in the same cluster but I think Kafka has also some system topics?
There was a problem hiding this comment.
In general I believe this is always a security risk with declarative resources in products.
For me the most important defense is definitely k8s RBAC on the KafkaCluster CRD preventing unauthorized users from taking creating KafkaTopics with arbitrary names. Also the principle used by the agent inside Kafka should have limitations on what it can and can't do.
Specifically for Kafka I'm also considering adding a validation on the topic name rejecting the prefix __ which is used by some internal topics.
There was a problem hiding this comment.
If we have a topic per cluster, we could store the topic ID in the cluster resource? Only if these match (or is set) we know it is managed by us?
For the internal topics: We explicitly offer naming the topic different from the custom resource name. Why is that? E.g. with Trino catalogs, the name is the one of the resource (with options to tweak).
https://docs.stackable.tech/home/stable/trino/usage-guide/catalogs/#_catalog_name_tweaking
|
Claude noted down some possible lifecycle problems/decisions:
Not sure if this is part of this PR or not, but it seemed general enough to mention it in the operator-rs changes, if this provides some mechanism for all the operators. |
Yes, the idea is to have the operator clean up after a KafkaCluster (or the whole Namespace) has been deleted and there is no agent left to clean up finalizers. As the cluster is being deleted anyway the operator only has to remove finalizers and doesn't need to take action in Kafka. For me this logic is a candidate to extract into |
| let status = match update.status { | ||
| ConditionStatus::True => "True", | ||
| ConditionStatus::False => "False", | ||
| ConditionStatus::Unknown => "Unknown", | ||
| } |
There was a problem hiding this comment.
This could be done via strum and IntoStaticStr instead of explicitly writing it?
| Retain, | ||
|
|
||
| /// Delete the resource in the product. | ||
| Delete, |
There was a problem hiding this comment.
If we have a topic per cluster, we could store the topic ID in the cluster resource? Only if these match (or is set) we know it is managed by us?
For the internal topics: We explicitly offer naming the topic different from the custom resource name. Why is that? E.g. with Trino catalogs, the name is the one of the resource (with options to tweak).
https://docs.stackable.tech/home/stable/trino/usage-guide/catalogs/#_catalog_name_tweaking
| pub struct ResourceOperation { | ||
| /// Stops the agent from changing the resource in the product. The status is still updated. | ||
| #[serde(default)] | ||
| pub reconciliation_paused: bool, | ||
| } | ||
| } |
There was a problem hiding this comment.
I am not sure about this. We never really update anything in the custom resource except the status anyways? If i want to get rid of the agent for testing / upgrading etc, i think we can just set the enabled = false flag?
What is the purpose, and why dont we then also support the "stopped" (scale to 0, same as enabled = false) flag?
| #[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub enum TlsClientCredential { | ||
| /// An AutoTLS SecretClass used to provision the certificate. |
There was a problem hiding this comment.
It doesn't need to be autoTls, could also be a static TLS cert using k8sSearch
| /// An AutoTLS SecretClass used to provision the certificate. | |
| /// The name of the SecretClass providing the certificate used for authentication at the procuct. |
|
I was wondering what the CRD looks like, can we please add it to the CRD preview? diff --git a/crates/stackable-operator/crds/DummyCluster.yaml b/crates/stackable-operator/crds/DummyCluster.yaml
index 0f3a9683..25acd2b8 100644
--- a/crates/stackable-operator/crds/DummyCluster.yaml
+++ b/crates/stackable-operator/crds/DummyCluster.yaml
@@ -416,6 +416,49 @@ spec:
kerberosRealmName:
description: A validated kerberos realm name type, for use in CRDs.
type: string
+ managementAccess:
+ nullable: true
+ properties:
+ authentication:
+ oneOf:
+ - required:
+ - anonymous
+ - required:
+ - tls
+ properties:
+ anonymous:
+ description: 'No credential: the product does not require authentication.'
+ type: object
+ tls:
+ description: 'Source of a TLS client certificate: a secret-operator SecretClass or a static Secret.'
+ oneOf:
+ - required:
+ - secretClassName
+ - required:
+ - secretName
+ properties:
+ secretClassName:
+ description: An AutoTLS SecretClass used to provision the certificate.
+ maxLength: 253
+ minLength: 1
+ pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
+ type: string
+ secretName:
+ description: |-
+ A static Secret holding the certificate in the keys `tls.crt` and `tls.key` (PEM), e.g. as
+ a Secret of type `kubernetes.io/tls`.
+ maxLength: 253
+ minLength: 1
+ pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
+ type: string
+ type: object
+ type: object
+ enabled:
+ type: boolean
+ required:
+ - authentication
+ - enabled
+ type: object
nodes:
description: |-
This struct represents a role - e.g. HDFS datanodes or Trino workers. It has a key-value-map containing
diff --git a/crates/xtask/src/crd/dummy.rs b/crates/xtask/src/crd/dummy.rs
index 67cbbfe1..e2edf0f5 100644
--- a/crates/xtask/src/crd/dummy.rs
+++ b/crates/xtask/src/crd/dummy.rs
@@ -115,6 +115,7 @@ pub mod versioned {
client_authentication_details: authentication::core::v1alpha1::ClientAuthenticationDetails<
oidc::v1alpha1::ClientAuthenticationMethodOption,
>,
+ management_access: Option<DummyManagementAccess>,
}
#[derive(Debug, Default, PartialEq, Fragment, JsonSchema)]
@@ -186,6 +187,22 @@ pub mod versioned {
pub struct DummyClusterStatus {
pub conditions: Vec<ClusterCondition>,
}
+
+ #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Eq, Serialize)]
+ #[serde(rename_all = "camelCase")]
+ #[schemars(crate = "stackable_operator::schemars")]
+ pub struct DummyManagementAccess {
+ enabled: bool,
+ authentication: DummyManagementAccessAuthentication,
+ }
+
+ #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Eq, Serialize)]
+ #[serde(rename_all = "camelCase")]
+ #[schemars(crate = "stackable_operator::schemars")]
+ pub enum DummyManagementAccessAuthentication {
+ Anonymous(stackable_operator::crd::management_access::v1alpha1::Anonymous),
+ Tls(stackable_operator::crd::management_access::v1alpha1::TlsClientCredential)
+ }
}
#[cfg(test)] |
Description
Adds shared utilities required for stackabletech/kafka-operator#1028.
Depends on https://github.com/stackabletech/decisions/issues/98 and https://github.com/stackabletech/decisions/issues/99.
Definition of Done Checklist
Author
Reviewer
Acceptance