Skip to content

Commit b335a78

Browse files
author
Fred Campos
committed
Addressed PR review: inject ObjectMapper, fix username validation for Secret branch, improve tests and docs
1 parent 6110958 commit b335a78

6 files changed

Lines changed: 210 additions & 112 deletions

File tree

‎.gitignore‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ config/
2020
!.idea/misc.xml
2121
!.idea/sqldialects.xml
2222
!.idea/vcs.xml
23+
**/out/
2324

2425
*.iml
2526
*.ipr
@@ -60,5 +61,3 @@ gradle-app.setting
6061

6162
# Quinoa
6263
.quinoa/
63-
64-
**/out/

‎docs/cluster-connection.md‎

Lines changed: 48 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,14 @@ Other Custom Resources (like `Database`, `Role`, `Schema`, `Grant`, `DefaultPriv
77

88
## Spec
99

10-
| Field | Type | Description | Required | Mutable |
11-
|---------------------|---------------------|-----------------------------------------------------------------------|----------|---------|
12-
| `host` | `string` | The hostname of the PostgreSQL instance. | Yes | Yes |
13-
| `port` | `integer` | The port of the PostgreSQL instance (1-65535). | Yes | Yes |
14-
| `database` | `string` | The database to connect to (usually `postgres` for admin operations). | Yes | Yes |
15-
| `adminSecretRef` | `ResourceRef` | Reference to the Kubernetes Secret containing the admin credentials. | No | Yes |
16-
| `adminSecretFileRef`| `ResourceFileRef` | Reference to a file containing the admin credentials. | No | Yes |
17-
| `parameters` | `map[string]string` | Additional connection parameters. | No | Yes |
10+
| Field | Type | Description | Required | Mutable |
11+
|----------------------|----------------------|-----------------------------------------------------------------------|----------|---------|
12+
| `host` | `string` | The hostname of the PostgreSQL instance. | Yes | Yes |
13+
| `port` | `integer` | The port of the PostgreSQL instance (1-65535). | Yes | Yes |
14+
| `database` | `string` | The database to connect to (usually `postgres` for admin operations). | Yes | Yes |
15+
| `adminSecretRef` | `ResourceRef` | Reference to the Kubernetes Secret containing the admin credentials. | No | Yes |
16+
| `adminSecretFileRef` | `FileRef` | Reference to a file containing the admin credentials. | No | Yes |
17+
| `parameters` | `map[string]string` | Additional connection parameters. | No | Yes |
1818

1919
> **Note:** Exactly one of `adminSecretRef` or `adminSecretFileRef` must be provided.
2020
@@ -27,53 +27,15 @@ Other Custom Resources (like `Database`, `Role`, `Schema`, `Grant`, `DefaultPriv
2727

2828
The referenced secret must be of type `kubernetes.io/basic-auth` and contain the keys `username` and `password`.
2929

30-
### ResourceFileRef (`adminSecretFileRef`)
30+
### FileRef (`adminSecretFileRef`)
3131

3232
| Field | Type | Description | Required |
3333
|--------|----------|----------------------------------------------------------------|----------|
3434
| `path` | `string` | The path to the file containing the admin credentials. | Yes |
3535

36-
Use this option when the credentials are mounted as a file instead of a Kubernetes Secret. The file must be a JSON object with the keys `username` and `password`, for example `{"username": "postgres", "password": "password"}`.
36+
Use this option when the credentials are mounted as a file instead of a Kubernetes Secret.
3737

38-
39-
### Examples
40-
41-
#### Using a Kubernetes Secret (`adminSecretRef`)
42-
43-
```yaml
44-
apiVersion: v1
45-
kind: Secret
46-
metadata:
47-
name: my-db-secret
48-
type: kubernetes.io/basic-auth
49-
stringData:
50-
username: postgres
51-
password: password
52-
```
53-
54-
```yaml
55-
apiVersion: postgresql.aboutbits.it/v1
56-
kind: ClusterConnection
57-
metadata:
58-
name: my-postgres-connection
59-
spec:
60-
adminSecretRef:
61-
name: my-db-secret
62-
host: localhost
63-
port: 5432
64-
database: postgres
65-
# Example parameters
66-
parameters:
67-
ApplicationName: "k8s-operator" # Helps identify this connection in Postgres logs
68-
#sslmode: "require" # Enforce SSL encryption
69-
#connectTimeout: "10" # Timeout in seconds for connection attempts
70-
```
71-
72-
### Using a file reference (`adminSecretFileRef`)
73-
74-
Instead of a Kubernetes Secret, you can mount a JSON credentials file into the operator pod and reference its path. This is useful when credentials are managed externally.
75-
76-
#### File format
38+
### File format
7739

7840
The file must contain JSON with the following fields:
7941

@@ -100,7 +62,7 @@ spec:
10062
template:
10163
spec:
10264
containers:
103-
- name: operator
65+
- name: postgresql-operator
10466
volumeMounts:
10567
- name: db-credentials
10668
mountPath: /mnt/secrets
@@ -113,7 +75,42 @@ spec:
11375
11476
> **Note:** The volume source can be any type that provides a file.
11577
116-
> **Note:** The Helm chart does not support extra volumes yet.
78+
> **Note:** The Helm chart does not support extra volumes yet.
79+
80+
### Examples
81+
82+
#### Using a Kubernetes Secret (`adminSecretRef`)
83+
84+
```yaml
85+
apiVersion: v1
86+
kind: Secret
87+
metadata:
88+
name: my-db-secret
89+
type: kubernetes.io/basic-auth
90+
stringData:
91+
username: postgres
92+
password: password
93+
```
94+
95+
```yaml
96+
apiVersion: postgresql.aboutbits.it/v1
97+
kind: ClusterConnection
98+
metadata:
99+
name: my-postgres-connection
100+
spec:
101+
adminSecretRef:
102+
name: my-db-secret
103+
host: localhost
104+
port: 5432
105+
database: postgres
106+
# Example parameters
107+
parameters:
108+
ApplicationName: "k8s-operator" # Helps identify this connection in Postgres logs
109+
#sslmode: "require" # Enforce SSL encryption
110+
#connectTimeout: "10" # Timeout in seconds for connection attempts
111+
```
112+
113+
#### Using a file reference (`adminSecretFileRef`)
117114

118115
```yaml
119116
apiVersion: postgresql.aboutbits.it/v1

‎operator/src/main/java/it/aboutbits/postgresql/core/FileRef.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,10 @@
66
import lombok.Setter;
77
import org.jspecify.annotations.NullMarked;
88

9-
/// A reference to a file inside
9+
/// A reference to a file inside the operator container.
1010
///
1111
/// This class is used wherever a CRD spec needs to point to a specific file
12-
/// The [#path] field identifies the file location
13-
/// within the container.
12+
/// The [#path] field identifies the file location within the container.
1413
///
1514
/// ### Example usage in a CR manifest
1615
///

‎operator/src/main/java/it/aboutbits/postgresql/core/KubernetesService.java‎

Lines changed: 42 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,29 @@
33
import com.fasterxml.jackson.databind.ObjectMapper;
44
import io.fabric8.kubernetes.client.KubernetesClient;
55
import it.aboutbits.postgresql.crd.clusterconnection.ClusterConnection;
6-
import it.aboutbits.postgresql.crd.clusterconnection.ClusterConnectionSpec;
76
import jakarta.inject.Singleton;
7+
import lombok.RequiredArgsConstructor;
88
import org.jspecify.annotations.NullMarked;
9+
import org.jspecify.annotations.Nullable;
910

1011
import java.io.IOException;
1112
import java.nio.charset.Charset;
1213
import java.nio.file.Files;
14+
import java.nio.file.NoSuchFileException;
1315
import java.nio.file.Path;
1416
import java.util.Base64;
1517

1618
@Singleton
19+
@RequiredArgsConstructor
1720
@NullMarked
1821
public final class KubernetesService {
19-
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
22+
private final ObjectMapper objectMapper;
23+
24+
private record FileCredentials(
25+
@Nullable String username,
26+
@Nullable String password
27+
) {
28+
}
2029

2130
public static final String SECRET_TYPE_BASIC_AUTH = "kubernetes.io/basic-auth";
2231
public static final String SECRET_DATA_BASIC_AUTH_USERNAME_KEY = "username";
@@ -28,11 +37,16 @@ public Credentials getAdminCredentials(
2837
) {
2938
var spec = clusterConnection.getSpec();
3039
if (spec.getAdminSecretRef() != null) {
31-
return getSecretRefCredentials(
32-
kubernetesClient,
33-
spec.getAdminSecretRef(),
34-
clusterConnection.getMetadata().getNamespace()
35-
);
40+
var secretRef = spec.getAdminSecretRef();
41+
var defaultNamespace = clusterConnection.getMetadata().getNamespace();
42+
var credentials = getSecretRefCredentials(kubernetesClient, secretRef, defaultNamespace);
43+
if (credentials.username() == null) {
44+
var secretNamespace = getSecretNamespace(secretRef, defaultNamespace);
45+
throw new IllegalStateException(
46+
"The Secret reference is missing required data username [secret.namespace=%s, secret.name=%s]".formatted(
47+
secretNamespace, secretRef.getName()));
48+
}
49+
return credentials;
3650
} else if (spec.getAdminSecretFileRef() != null) {
3751
return getSecretFileRefCredentials(spec.getAdminSecretFileRef());
3852
}
@@ -43,36 +57,23 @@ public Credentials getAdminCredentials(
4357
public Credentials getSecretFileRefCredentials(FileRef fileRef) {
4458
var path = Path.of(fileRef.getPath());
4559

46-
if (!Files.exists(path)) {
47-
throw new IllegalStateException("Credential file not found [path=%s]".formatted(path));
48-
}
49-
50-
try {
51-
var content = Files.readString(path);
52-
var json = OBJECT_MAPPER.readTree(content);
53-
54-
var usernameNode = json.get(SECRET_DATA_BASIC_AUTH_USERNAME_KEY);
55-
var username = usernameNode != null && !usernameNode.isNull()
56-
? usernameNode.asText()
57-
: null;
58-
if (username == null) {
59-
throw new IllegalStateException("Credential file is missing required field '%s' [path=%s]".formatted(
60-
SECRET_DATA_BASIC_AUTH_USERNAME_KEY,
61-
path
62-
));
60+
try (var in = Files.newInputStream(path)) {
61+
var file = objectMapper.readValue(in, FileCredentials.class);
62+
if (file.username() == null) {
63+
throw new IllegalStateException(
64+
"Credentials file is missing required field 'username' [path=%s]".formatted(path));
6365
}
64-
65-
var passwordNode = json.get(SECRET_DATA_BASIC_AUTH_PASSWORD_KEY);
66-
if (passwordNode == null || passwordNode.isNull()) {
67-
throw new IllegalStateException("Credential file is missing required field '%s' [path=%s]".formatted(
68-
SECRET_DATA_BASIC_AUTH_PASSWORD_KEY,
69-
path
70-
));
66+
if (file.password() == null) {
67+
throw new IllegalStateException(
68+
"Credentials file is missing required field 'password' [path=%s]".formatted(path));
7169
}
72-
73-
return new Credentials(username, passwordNode.asText());
70+
return new Credentials(file.username(), file.password());
71+
} catch (NoSuchFileException e) {
72+
throw new IllegalStateException(
73+
"Credentials file not found [path=%s]".formatted(path), e);
7474
} catch (IOException e) {
75-
throw new IllegalStateException("Failed to read Credential file [path=%s]".formatted(path), e);
75+
throw new IllegalStateException(
76+
"Failed to read the credentials file [path=%s]".formatted(path), e);
7677
}
7778
}
7879

@@ -81,9 +82,7 @@ public Credentials getSecretRefCredentials(
8182
ResourceRef secretRef,
8283
String defaultNamespace
8384
) {
84-
var secretNamespace = secretRef.getNamespace() != null
85-
? secretRef.getNamespace()
86-
: defaultNamespace;
85+
var secretNamespace = getSecretNamespace(secretRef, defaultNamespace);
8786

8887
var secretName = secretRef.getName();
8988

@@ -141,4 +140,10 @@ public Credentials getSecretRefCredentials(
141140
password
142141
);
143142
}
143+
144+
private String getSecretNamespace(ResourceRef secretRef, String defaultNamespace) {
145+
return secretRef.getNamespace() != null
146+
? secretRef.getNamespace()
147+
: defaultNamespace;
148+
}
144149
}

‎operator/src/test/java/it/aboutbits/postgresql/core/KubernetesServiceTest.java‎

Lines changed: 47 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
import org.junit.jupiter.params.ParameterizedTest;
1818
import org.junit.jupiter.params.provider.ValueSource;
1919

20+
import com.fasterxml.jackson.databind.ObjectMapper;
21+
2022
import java.io.IOException;
2123
import java.nio.charset.Charset;
2224
import java.nio.file.Files;
@@ -30,7 +32,7 @@
3032
@NullMarked
3133
@EnableKubernetesMockClient(crud = true)
3234
class KubernetesServiceTest {
33-
private final KubernetesService service = new KubernetesService();
35+
private final KubernetesService service = new KubernetesService(new ObjectMapper());
3436

3537
@SuppressWarnings("NullAway.Init")
3638
static KubernetesClient client;
@@ -103,6 +105,27 @@ void whenPasswordMissingOrNull_shouldThrow(String json) throws IOException {
103105
.hasMessageContaining("missing required field 'password'");
104106
}
105107

108+
@ParameterizedTest(name = "when field has wrong type {0}, should throw")
109+
@ValueSource(strings = {
110+
"{\"username\": {}, \"password\": \"s3cret\"}",
111+
"{\"username\": [], \"password\": \"s3cret\"}",
112+
"{\"username\": \"admin\", \"password\": {}}",
113+
"{\"username\": \"admin\", \"password\": []}"
114+
})
115+
void whenFieldHasWrongType_shouldThrow(String json) throws IOException {
116+
// given
117+
var file = tempDir.resolve("secret.json");
118+
Files.writeString(file, json);
119+
120+
var fileRef = new FileRef();
121+
fileRef.setPath(file.toString());
122+
123+
// when / then
124+
assertThatThrownBy(() -> service.getSecretFileRefCredentials(fileRef))
125+
.isInstanceOf(IllegalStateException.class)
126+
.hasMessageContaining("Failed to read");
127+
}
128+
106129
@Test
107130
@DisplayName("when file not found, should throw")
108131
void whenFileNotFound_shouldThrow() {
@@ -113,7 +136,7 @@ void whenFileNotFound_shouldThrow() {
113136
// when / then
114137
assertThatThrownBy(() -> service.getSecretFileRefCredentials(fileRef))
115138
.isInstanceOf(IllegalStateException.class)
116-
.hasMessageContaining("Credential file not found");
139+
.hasMessageContaining("Credentials file not found");
117140
}
118141

119142
@Test
@@ -194,23 +217,6 @@ void whenSecretHasNoData_shouldThrow() {
194217
.hasMessageContaining("has no data set");
195218
}
196219

197-
@Test
198-
@DisplayName("when secret missing password, should throw")
199-
void whenSecretMissingPassword_shouldThrow() {
200-
// given
201-
var secret = new SecretBuilder()
202-
.withNewMetadata().withNamespace("my-ns").withName("my-secret").endMetadata()
203-
.withType(KubernetesService.SECRET_TYPE_BASIC_AUTH)
204-
.addToData("username", base64("admin"))
205-
.build();
206-
client.secrets().inNamespace("my-ns").resource(secret).create();
207-
208-
// when / then
209-
assertThatThrownBy(() -> service.getSecretRefCredentials(client, secretRef("my-ns", "my-secret"), "default-ns"))
210-
.isInstanceOf(IllegalStateException.class)
211-
.hasMessageContaining("missing required data password");
212-
}
213-
214220
@Test
215221
@DisplayName("when namespace null, should use default namespace")
216222
void whenNamespaceNull_shouldUseDefaultNamespace() {
@@ -327,6 +333,28 @@ void whenOnlyAdminSecretFileRefSet_shouldDelegateToFileRef() throws IOException
327333
assertThat(result.password()).isEqualTo("file-s3cret");
328334
}
329335

336+
@Test
337+
@DisplayName("when adminSecretRef missing username, should throw with message not NPE")
338+
void whenAdminSecretRefMissingUsername_shouldThrowWithMessage() {
339+
// given
340+
var secret = new SecretBuilder()
341+
.withNewMetadata().withNamespace("my-ns").withName("my-secret").endMetadata()
342+
.withType(KubernetesService.SECRET_TYPE_BASIC_AUTH)
343+
.addToData("password", base64("s3cret"))
344+
.build();
345+
client.secrets().inNamespace("my-ns").resource(secret).create();
346+
347+
var spec = new ClusterConnectionSpec();
348+
spec.setAdminSecretRef(secretRef("my-ns", "my-secret"));
349+
350+
var clusterConnection = buildClusterConnection(spec, "cr-ns");
351+
352+
// when / then
353+
assertThatThrownBy(() -> service.getAdminCredentials(client, clusterConnection))
354+
.isInstanceOf(IllegalStateException.class)
355+
.hasMessageContaining("missing required data username");
356+
}
357+
330358
@Test
331359
@DisplayName("when neither ref set, should throw")
332360
void whenNeitherRefSet_shouldThrow() {

0 commit comments

Comments
 (0)