-
-
Notifications
You must be signed in to change notification settings - Fork 17
feat: Add generic database connection mechanism #1163
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
base: main
Are you sure you want to change the base?
Changes from all commits
df6084b
f8a0b62
1dc73a1
c581a5b
32c0ec9
b9a16a9
b51f227
159faac
9bb1e95
c2799fa
69fbb2a
675dcc8
f457c20
7c193c4
9e31680
33ae519
cfa085d
8e05224
49f8ba0
3e391ff
a21b8bd
51cfff6
43730e2
75efe83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| use k8s_openapi::api::core::v1::{EnvVar, EnvVarSource, SecretKeySelector}; | ||
|
|
||
| pub fn env_var_from_secret( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: I think this is not an appropriate name. We are not constructing an env var from a Kubernetes secret (which would need to be looked up), but we instead prepare the env var in such a way that it will source its value from a Secret (which the Kubernetes apiserver or the kubelet is responsible for). |
||
| env_var_name: impl Into<String>, | ||
| secret_name: impl Into<String>, | ||
| secret_key: impl Into<String>, | ||
| ) -> EnvVar { | ||
| EnvVar { | ||
| name: env_var_name.into(), | ||
| value_from: Some(EnvVarSource { | ||
| secret_key_ref: Some(SecretKeySelector { | ||
| name: secret_name.into(), | ||
| key: secret_key.into(), | ||
| ..Default::default() | ||
| }), | ||
| ..Default::default() | ||
| }), | ||
| ..Default::default() | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ use crate::{ | |
| }; | ||
|
|
||
| pub mod container; | ||
| pub mod env; | ||
| pub mod probe; | ||
| pub mod resources; | ||
| pub mod security; | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: I find it awkward that we have |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,56 @@ | ||||||||||||||||
| use schemars::JsonSchema; | ||||||||||||||||
| use serde::{Deserialize, Serialize}; | ||||||||||||||||
| use snafu::{ResultExt, Snafu}; | ||||||||||||||||
|
|
||||||||||||||||
| use crate::databases::{ | ||||||||||||||||
| TemplatingMechanism, | ||||||||||||||||
| drivers::jdbc::{JDBCDatabaseConnection, JDBCDatabaseConnectionDetails}, | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| #[derive(Debug, Snafu)] | ||||||||||||||||
| pub enum Error { | ||||||||||||||||
| #[snafu(display("failed to parse connection URL"))] | ||||||||||||||||
| ParseConnectionUrl { source: url::ParseError }, | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// Connection settings for an embedded [Apache Derby](https://db.apache.org/derby/) database. | ||||||||||||||||
| /// | ||||||||||||||||
| /// Derby is an embedded, file-based Java database engine that requires no separate server process. | ||||||||||||||||
| /// It is typically used for development, testing, or as a lightweight metastore backend (e.g. for | ||||||||||||||||
| /// Apache Hive). | ||||||||||||||||
| #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] | ||||||||||||||||
| #[serde(rename_all = "camelCase")] | ||||||||||||||||
| pub struct DerbyConnection { | ||||||||||||||||
| /// Path on the filesystem where Derby stores its database files. | ||||||||||||||||
| /// | ||||||||||||||||
| /// If not specified, defaults to `/tmp/derby/{unique_database_name}/derby.db`. | ||||||||||||||||
| /// The `{unique_database_name}` part is automatically handled by the operator and is added to | ||||||||||||||||
| /// prevent clashing database files. The `create=true` flag is always appended to the JDBC URL, | ||||||||||||||||
| /// so the database is created automatically if it does not yet exist at this location. | ||||||||||||||||
| pub location: Option<String>, | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: This should be a PathBuf instead. Also, we should validate a few things, e.g. that the path is absolute, doesn't include path traversals ( |
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| impl JDBCDatabaseConnection for DerbyConnection { | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: This should be named |
||||||||||||||||
| fn jdbc_connection_details_with_templating( | ||||||||||||||||
| &self, | ||||||||||||||||
| unique_database_name: &str, | ||||||||||||||||
| _templating_mechanism: &TemplatingMechanism, | ||||||||||||||||
| ) -> Result<JDBCDatabaseConnectionDetails, crate::databases::Error> { | ||||||||||||||||
| let location = self | ||||||||||||||||
| .location | ||||||||||||||||
| .clone() | ||||||||||||||||
| .unwrap_or_else(|| format!("/tmp/derby/{unique_database_name}/derby.db")); | ||||||||||||||||
|
Comment on lines
+39
to
+42
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: We should use
Suggested change
|
||||||||||||||||
| let connection_uri = format!("jdbc:derby:{location};create=true",); | ||||||||||||||||
| let connection_uri = connection_uri.parse().context(ParseConnectionUrlSnafu)?; | ||||||||||||||||
|
|
||||||||||||||||
| Ok(JDBCDatabaseConnectionDetails { | ||||||||||||||||
| // Sadly the Derby driver class name is a bit complicated, e.g. for HMS up to 4.1.x we used | ||||||||||||||||
| // "org.apache.derby.jdbc.EmbeddedDriver", | ||||||||||||||||
| // for HMS 4.2.x we used "org.apache.derby.iapi.jdbc.AutoloadedDriver". | ||||||||||||||||
| driver: "org.apache.derby.jdbc.EmbeddedDriver".to_owned(), | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: These well-known driver names (for JDBC) should live in (associated) constants instead. |
||||||||||||||||
| connection_uri, | ||||||||||||||||
| username_env: None, | ||||||||||||||||
| password_env: None, | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| pub mod derby; | ||
| pub mod mysql; | ||
| pub mod postgresql; | ||
| pub mod redis; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This makes zero sense. We make it look like we take an iterator of items which can be borrowed as
EnvVar, but they are not actually not only borrowed, but cloned under the hood.I would go as far as to call this an anti-pattern.