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

drop extension / update crd #147

Merged
merged 19 commits into from
Mar 6, 2023
27 changes: 23 additions & 4 deletions coredb-operator/src/controller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use kube::{
};

use crate::{
extensions::create_extensions, postgres_exporter_role::create_postgres_exporter_role,
extensions::manage_extensions, postgres_exporter_role::create_postgres_exporter_role,
secret::reconcile_secret,
};
use k8s_openapi::{
Expand Down Expand Up @@ -66,7 +66,7 @@ pub struct CoreDBSpec {
#[serde(default = "defaults::default_uid")]
pub uid: i32,
#[serde(default = "defaults::default_extensions")]
pub enabledExtensions: Vec<String>,
pub extensions: Vec<Extension>,
}

/// The status object of `CoreDB`
Expand All @@ -86,6 +86,25 @@ pub struct Context {
pub metrics: Metrics,
}

#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize, PartialEq)]
pub struct Extension {
pub name: String,
pub version: String,
pub enabled: bool,
pub schema: String,
}

impl Default for Extension {
fn default() -> Self {
Extension {
name: "pg_stat_statements".to_owned(),
version: "1.9".to_owned(),
enabled: true,
schema: "postgres".to_owned(),
}
}
}
Comment on lines +97 to +106
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
impl Default for Extension {
fn default() -> Self {
Extension {
name: "pg_stat_statements".to_owned(),
version: "1.9".to_owned(),
enabled: true,
schema: "postgres".to_owned(),
}
}
}

I think the default should be an empty vec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whats the basis for this being empty? This is the default method and it is explicit. You only gets used if you're calling ::Default().

Copy link
Member

Choose a reason for hiding this comment

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

What's an example case for using this? I could see us using it as a default CRD value if we wanted this extension enabled whenever the user provides nothing in spec.extensions (like you had previously configured), but we want that to be empty in that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I use ::Default() a lot in tests or wherever I want to construct a default set of values for struct.

Copy link
Member

Choose a reason for hiding this comment

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

👍 seems reasonable!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

default_extensions is an empty vec regardless of implementing Default on the Extension


#[instrument(skip(ctx, cdb), fields(trace_id))]
async fn reconcile(cdb: Arc<CoreDB>, ctx: Arc<Context>) -> Result<Action> {
let trace_id = telemetry::get_trace_id();
Expand Down Expand Up @@ -174,8 +193,8 @@ impl CoreDB {
return Ok(Action::requeue(Duration::from_secs(1)));
}

create_extensions(self, ctx.clone()).await.expect(&format!(
"Error creating extensions on CoreDB {}",
manage_extensions(self, ctx.clone()).await.expect(&format!(
"Error updating extensions on CoreDB {}",
self.metadata.name.clone().unwrap()
));

Expand Down
4 changes: 3 additions & 1 deletion coredb-operator/src/defaults.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use k8s_openapi::{api::core::v1::ResourceRequirements, apimachinery::pkg::api::resource::Quantity};
use std::collections::BTreeMap;

use crate::Extension;

pub fn default_replicas() -> i32 {
1
}
Expand Down Expand Up @@ -44,6 +46,6 @@ pub fn default_postgres_exporter_image() -> String {
"quay.io/prometheuscommunity/postgres-exporter:v0.11.1".to_owned()
}

pub fn default_extensions() -> Vec<String> {
pub fn default_extensions() -> Vec<Extension> {
vec![]
}
49 changes: 33 additions & 16 deletions coredb-operator/src/extensions.rs
Original file line number Diff line number Diff line change
@@ -1,32 +1,49 @@
use crate::{Context, CoreDB, Error};
use regex::Regex;
use std::sync::Arc;
use tracing::debug;
use tracing::{debug, info, warn};

pub async fn create_extensions(cdb: &CoreDB, ctx: Arc<Context>) -> Result<(), Error> {
pub async fn manage_extensions(cdb: &CoreDB, ctx: Arc<Context>) -> Result<(), Error> {
let client = ctx.client.clone();
let extensions = &cdb.spec.enabledExtensions;
let extensions = &cdb.spec.extensions;
let re = Regex::new(r"[a-zA-Z][0-9a-zA-Z_-]*$").unwrap();

// TODO(ianstanton) Some extensions will fail to create. We need to handle and surface any errors.
// Logging result at debug level for now.

// iterate through list of extensions and run CREATE EXTENSION <extension-name> for each
for ext in extensions {
if !re.is_match(ext) {
debug!("Extension {} is not formatted properly. Skipping creation.", ext)
let ext_name = ext.name.as_str();
if !re.is_match(ext_name) {
warn!(
"Extension {} is not formatted properly. Skipping operation.",
ext_name
)
} else {
debug!("Creating extension: {}", ext);
// this will no-op if we've already created the extension
let result = cdb
.psql(
format!("CREATE EXTENSION {};", ext),
"postgres".to_owned(),
client.clone(),
)
.await
.unwrap();
debug!("Result: {}", result.stdout.clone().unwrap());
if ext.enabled {
info!("Creating extension: {}", ext_name);
// this will no-op if we've already created the extension
let result = cdb
.psql(
format!("CREATE EXTENSION IF NOT EXISTS {ext_name};"),
ChuckHend marked this conversation as resolved.
Show resolved Hide resolved
"postgres".to_owned(),
client.clone(),
)
.await
.unwrap();
debug!("Result: {}", result.stdout.clone().unwrap());
} else {
info!("Dropping extension: {}", ext_name);
let result = cdb
.psql(
format!("DROP EXTENSION IF EXISTS {ext_name};"),
"postgres".to_owned(),
client.clone(),
)
.await
.unwrap();
debug!("Result: {}", result.stdout.clone().unwrap());
}
}
}
Ok(())
Expand Down
39 changes: 37 additions & 2 deletions coredb-operator/tests/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ mod test {
},
"spec": {
"replicas": replicas,
"enabledExtensions": ["postgis"]
"extensions": [{"name": "postgis", "enabled": true, "version": "1.1.1", "schema": "public"}]
}
});
let params = PatchParams::apply("coredb-integration-test");
Expand Down Expand Up @@ -255,6 +255,41 @@ mod test {
let result_stdout = run_command_in_container(pods.clone(), test_pod_name.clone(), command).await;
assert!(result_stdout.contains("pg_up 1"));
println!("Found metrics when curling the metrics service");

// Assert we can drop an extension after its been created
let coredb_json = serde_json::json!({
"apiVersion": API_VERSION,
"kind": kind,
"metadata": {
"name": name
},
"spec": {
"replicas": replicas,
"extensions": [{"name": "postgis", "enabled": false, "version": "1.1.1", "schema": "public"}]
}
});

// Apply crd with extension disabled
let params = PatchParams::apply("coredb-integration-test");
let patch = Patch::Apply(&coredb_json);
let coredb_resource = coredbs.patch(name, &params, &patch).await.unwrap();

// give it time to drop
thread::sleep(Duration::from_millis(5000));

// Assert extension no longer created
let result = coredb_resource
.psql(
"select extname from pg_catalog.pg_extension;".to_string(),
"postgres".to_string(),
client.clone(),
)
.await
.unwrap();

println!("{}", result.stdout.clone().unwrap());
// assert does not contain postgis
assert!(!result.stdout.clone().unwrap().contains("postgis"));
}

#[tokio::test]
Expand Down Expand Up @@ -298,7 +333,7 @@ mod test {
},
"spec": {
"replicas": 1,
"enabledExtensions": ["postgis"]
"extensions": [{"name": "postgis", "enabled": true, "version": "1.1.1", "schema": "public"}],
}
});
let params = PatchParams::apply("coredb-integration-test");
Expand Down
18 changes: 16 additions & 2 deletions coredb-operator/yaml/crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,24 @@ spec:

This provides a hook for generating the CRD yaml (in crdgen.rs)
properties:
enabledExtensions:
extensions:
default: []
items:
type: string
properties:
enabled:
type: boolean
name:
type: string
schema:
type: string
version:
type: string
required:
- enabled
- name
- schema
- version
type: object
type: array
image:
default: quay.io/coredb/postgres:c03124e
Expand Down
7 changes: 5 additions & 2 deletions coredb-operator/yaml/sample-coredb.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,8 @@ metadata:
name: sample-coredb
spec:
replicas: 1
enabledExtensions:
- postgis
extensions:
- name: "pg_stat_statements"
enabled: true
version: "1.9"
schema: "postgres"