Allow customers to specify roles that they want credentials for... #3854

dsessler7 · 2024-02-16T20:03:53Z

in the CrunchyBridgeCluster spec. When specified, PGO will create/update a corresponding Secret and fill with role name, password, and connection URI during the reconcile loop. If role is deleted from spec or secret name is changed, PGO will delete unused secret.

Checklist:

Have you added an explanation of what your changes do and why you'd like them to be included?
Have you updated or added documentation for the change, as applicable?
Have you tested your changes on all related environments with successful results, as applicable?
- Have you added automated tests?

Type of Changes:

What is the current behavior (link to any open issues here)?

There is no user/role functionality.

What is the new behavior (if this is a feature change)?

Breaking change (fix or feature that would cause existing functionality to change)

The customer can now add roles (that correspond to available roles in Bridge) to their spec and specify a secret name and get a Secret in their k8s cluster that contains that role's password and connection URI.

Other Information:

internal/bridge/crunchybridgecluster/postgres.go

pkg/apis/postgres-operator.crunchydata.com/v1beta1/crunchy_bridgecluster_types.go

cbandy · 2024-02-20T18:27:50Z

pkg/apis/postgres-operator.crunchydata.com/v1beta1/crunchy_bridgecluster_types.go

+	// The name of the Secret that will hold the role credentials.
+	// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`
+	// +kubebuilder:validation:Type=string
+	SecretName string `json:"secretName"`


❓ Secret names are DNS subdomains. Is there a validation rule/shortcut for that?

🤔 We can start with a more limited rule (the one here) and expand it later.

I don't see a specific validation tag in the Kubebuilder docs...

According to this, the regex we want is [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*

I would think that the kubebuilder:validation:Format marker would accept an IsDNS1123Subdomain option given that the kubebuilder codebase has functions written specifically for validating this, but it doesn't seem to work and I can't find any documentation for what options they actually accept for the "Format" validation...

AFAIK, format comes from OpenAPIv3. I see references to JSON Schema Validation elsewhere. I'm also not sure what Kubernetes has implemented (or not).

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation →
https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.0.md#schema-object →
https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.0.md#dataTypeFormat

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules →
https://swagger.io/specification/#data-types →
https://tools.ietf.org/html/draft-bhutton-json-schema-validation-00#section-7.3

hostname looks close, but I'm not excited by following 3–5 RFCs down to the early internet. :notsure:

I see the same regex you pasted when I look here. So that plus max-length sounds good to me.

internal/bridge/crunchybridgecluster/crunchybridgecluster_controller.go

internal/bridge/client.go

internal/bridge/crunchybridgecluster/crunchybridgecluster_controller.go

internal/bridge/client.go

internal/bridge/crunchybridgecluster/postgres.go

benjaminjb · 2024-02-22T19:14:44Z

internal/naming/selectors.go

+			LabelCluster: cluster,
+		},
+		MatchExpressions: []metav1.LabelSelectorRequirement{
+			{Key: LabelCrunchyBridgeClusterPostgresRole, Operator: metav1.LabelSelectorOpExists},


This seems fine to me, though might it be simpler to add

naming.LabelRole: RoleCrunchyBridgeClusterPostgresRole

to the MatchLabels?

pkg/apis/postgres-operator.crunchydata.com/v1beta1/crunchy_bridgecluster_types.go

…e CrunchyBridgeCluster spec. When specified, create corresponding Secret and fill with role name, password, and connection URI. If role is deleted from spec or secret name is changed, delete unused secret.

…equest and response payload structs. Add fields to CBC status.

…the same name. Rename some API fields. Use pointers for booleans so false values still show up. Other minor changes.

…alues accepted/returned by bridge API. Co-authored-by: Chris Bandy <[email protected]>

benjaminjb

This all makes sense to me, but I do wish we had some other solution for the secret name only because it seems kind of cumbersome for the operator to

(a) check for secret name uniqueness on the spec,
(b) check for secret name uniqueness in the namespace (ignoring secrets with certain labels -- heck, you could include that in this K8s API list action: list secrets that don't have certain selectors),
(c) and then get the secrets for this cluster to compare against the roles requested.

But I can't really see a way around it. My only other thoughts are

(a) "let's use a UUID" -- which make the secrets hard to use;
(b) "let's transform the disallowed characters from the PG role into allowed characters for K8s" -- which might just collide with another secret. (Unless we add the cluster as part of the secret, and even then, that's not 100% proof against collisions.)

So yeah, I have no other solutions, so won't block on this.

benjaminjb · 2024-02-26T23:20:02Z

internal/bridge/quantity.go

+	"k8s.io/apimachinery/pkg/api/resource"
+)
+
+func FromCPU(n int64) *resource.Quantity {


We're not using this right now, are we?

No, we're not. It would be useful if we ever decide to include CPU on the status... Not sure if I should keep it or just delete it for now.

Yeah, I'm on the fence...

benjaminjb · 2024-02-26T23:35:56Z

internal/bridge/crunchybridgecluster/postgres.go

+	for i := range specRoles {
+		if secretNames[specRoles[i].SecretName] {
+			// Duplicate secretName found, return early with error
+			err := errors.New("There are duplicate Role SecretNames in the spec. SecretNames must be unique.")


I have a vague memory of us preferring fmt.Errorf, let me check on that...

Yeah, I'm seeing more of that.

Okay, I can switch to using that.

Although when I search the repo I see 74 results for errors.New and 55 results for fmt.Errorf...

Oh, good double-check, ok. I wonder why we prefer one over the other in some places?

Probably just the preference of whoever was writing the code...

dsessler7 requested review from ValClarkson and cbandy February 16, 2024 20:04

dsessler7 force-pushed the reconcile_cbc_roles branch from fe34050 to 400131c Compare February 16, 2024 20:33

ValClarkson reviewed Feb 16, 2024

View reviewed changes

internal/bridge/crunchybridgecluster/postgres.go Outdated Show resolved Hide resolved

ValClarkson approved these changes Feb 16, 2024

View reviewed changes

cbandy reviewed Feb 20, 2024

View reviewed changes

dsessler7 force-pushed the reconcile_cbc_roles branch from 775eaca to 40a3e4d Compare February 20, 2024 22:58

dsessler7 requested review from cbandy and ValClarkson February 21, 2024 18:32

ValClarkson approved these changes Feb 21, 2024

View reviewed changes

dsessler7 marked this pull request as ready for review February 21, 2024 22:20