quay-app(deployment-template): add postgresql client certificate overlay for authentication (PROJQUAY-2417) #796
Conversation
|
@BillDett can someone from the team verify and approve the PR please ? |
|
/test e2e |
|
@dmage: The specified target(s) for
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test ocp-latest-e2e |
kustomize/base/quay.deployment.yaml
Outdated
| projected: | ||
| sources: | ||
| - secret: | ||
| name: postgresql-ca |
There was a problem hiding this comment.
Should secret names be configurable via the QuayRegistry object?
There was a problem hiding this comment.
hmm ... since it's a projected secret, meaning expected to be populated out of two secrets we would need to have three variables ... not sure if it's worth the effort ... your opinion on that ?
There was a problem hiding this comment.
Let's step back, why do we need to two secrets, not just one? Why would customers want to have different secrets for CA and for client certs?
Another moment - as I can see, the canonical way to provide the operator with additional secrets/configuration is to use configBundleSecret. The operator extracts data from the secret and creates necessary objects. That's how extra_ca_certs are configured, for example. Shouldn't we use the same approach?
There was a problem hiding this comment.
I have chosen the secret as projected to handle multiple scenarios:
- where who ever creates the CA bundle (operators,humans,..) create one secret with all necessary files
- where who ever creates the CA bundle creates a CA secret and client cert plus key secret in one k8 secret object
to explain the scenario and why this is working...
- scenario 1 two secrets like done by CNPG or cert-manager
$ oc create namespace quay-test
$ oc -n quay-test create secret tls ca-bundle --cert cert.pem --key privkey.pem
$ oc -n quay-test create secret generic ca --from-file=ca.crt=fullchain.pem
$ oc -n quay-test get deploy/quay -o yaml
...
- name: postgresql-certs
projected:
defaultMode: 420
sources:
- secret:
name: ca # <= first secret
optional: true
- secret:
name: ca-bundle # <= second secret
optional: true
- scenario 2 one secret like done by humans, or OCP internal
$ oc -n quay-test debug deploy/quay -- ls /var/run/secrets/postgresql/
Starting pod/quay-debug ...
ca.crt # <= secret ca
tls.crt # <= secret ca-bundle cert
tls.key # <= secret ca-bundle key
$ oc -n quay-test create secret generic ca-all --from-file=ca.crt=fullchain.pem --from-file=tls.crt=cert.pem --from-file=privkey.pem
$ oc -n quay-test debug deploy/quay -- ls /var/run/secrets/postgresql/
$ oc -n quay-test get deploy/quay -o yaml
...
- name: postgresql-certs
projected:
defaultMode: 420
sources:
- secret:
name: ca-2 # <= something invalid
optional: true
- secret:
name: ca-all # <= all CA files
optional: true
$ oc -n quay-test debug deploy/quay -- ls /var/run/secrets/postgresql/
Starting pod/quay-debug ...
ca.crt # <= secret ca-all
privkey.pem # <= secret ca-all
tls.crt # <= secret ca-all
with the optional: true flag on each secret it builds what ever is available and leave's out if something is missing.
The mentioned bundled approach puts people where certs are handled differently in the position where they need to manually create a merged secret as bundle.
There was a problem hiding this comment.
in regards to extra_ca_certs we cannot use those as from the fs point of view due to permission issues within the process.
Every secret is created by root and therefor owned by uid 0 with to wide opened permissions 0644 (similar to ssh keys). Changing the permissions in the secret isn't sufficient as the process (quay) isn't allowed to read those files with changing the uid (1001) when running. That's why the client_certs.sh script added for the initialization copies those to the ephemeral storage in /.postgresql which is libpg default.
the script as well assumes default kubernets.io/tls secret structure which has:
- ca.crt
- tls.key
- tls.crt
and moves them to the default libpq expected names:
- postgresql.crt
- postgresql.key
- root.crt
using the libpq defaults the docu can be written in an advanced configuration style and in a default way.
- advanced indicating how to choose different certificates as well
DB_URI=postgresql://quay@postgres.quay.svc:5432/quay?sslmode=verify-full&sslkey=/.postgresql/postgresql.key&sslcert=/.postgresql/postgresql.crt&sslrootcert=/.postgresql/root.crt
- as well as default working out of the box (sslcert,sslkey,sslrootcert will be used from libpq defaults)
DB_URI=postgresql://quay@postgres.quay.svc:5432/quay?sslmode=verify-full
Yes, the advanced way will carry the burden that permissions need to be setup similar to what the init script added does and with a persistent storage I do not see that people could not get around that burden ...
There was a problem hiding this comment.
I doubt we have such docs.
First new parameters should be add to the resource QuayRegistry, which is defined at apis/quay/v1/quayregistry_types.go. I would suggest checking out the Kubernetes code to see good examples of API definitions, for example HorizontalPodAutoscalerSpec.
Currently it has only configBundleSecret and components. A new parameter can look like
type QuayRegistrySpec struct {
// quay configures the main Quay application.
// +optional
Quay *QuaySpec `json:"quay,omitempty"`
...
}
// QuaySpec specifies configuration for the main Quay application.
struct QuaySpec {
// postgresTLSSecret is the secret of type kubernetes.io/tls containing the client TLS certificate and key that will be used to connect to Postgres securely.
// +optional
PostgresTLSSecret string `json:"postgresTLSSecret,omitempty"`
}make manifests will update the custom resource definition at bundle/manifests/quayregistries.crd.yaml, you can install these manifests on your cluster via make install.
Wiring this parameters into the reconciliation loop is a bit more complex.
The starting point is
quay-operator/controllers/quay/quayregistry_controller.go
Lines 629 to 632 in f04bfd8
kustomize.Inflate is provided only with the secret that is specified in configBundleSecret, and it doesn't have a Kubernetes client to retrieve other objects from the cluster. Maybe Kustomize is flexible enough, and it's possible to create a projected volume with the secret names. Maybe few more shenanigans are needed at https://github.com/quay/quay-operator/blob/master/pkg/middleware/middleware.go#L173. Or maybe new actions before the kustomize.Inflate call are needed.
There was a problem hiding this comment.
okay thanks @dmage ++ I'll dig into that and hope (fingers crossed) that it will not take too long to get it in a more appropriate way implemented ...
thanks once more for your time and support
There was a problem hiding this comment.
@dmage the best I can get out of kustomize is following
- name: postgres-tls-secrets
projected:
sources:
- name: CA-1a658d93-57ba-434e-975d-017afba11cdd
optional: true
secret: null
- name: TLS-3f17ee00-04f8-4db1-ad4a-75aefc7b34e1
optional: true
secret: null
so names can be generated from another resource like a configmap but unfortunately those sources for kustomize cannot hold structured data like it's necessary for the projected secret ...
I do not have any clue how to proceed to get a projected secret with kustomize into the operator as for the config-bundle secret, only the secret name (metadata.name) has to be changed and no structure needs to be transported into the deployment config.
(simple var assignment in the operator controller, structures cannot be moved like that in kustomize)
// NOTE: Using `vars` in Kustomize is kinda ugly because it's basically templating, so don't abuse them
Vars: []types.Var{
{
Name: "QE_K8S_CONFIG_SECRET",
ObjRef: types.Target{
APIVersion: "v1",
Gvk: resid.Gvk{
Kind: "Secret",
},
Name: "quay-config-secret",
},
},
Furthermore, I was trying to utilize kustomize's replacements functionality as Vars have been deprecated by upstream already ...
Any Idea how we could still proceed ?
There was a problem hiding this comment.
found a way to get projected secrets with kustomize working ... will work on an implementation for the operator
There was a problem hiding this comment.
@dmage I need to admit, I cannot provide a patch to the operator with the necessary logic to provide a projected configMap composed out of code retrieved from the config-editor (or filled from QuayRegistry CR).
what I can provide is the kustomize logic to do so. Please find someone in the Team who can follow up to get the integration for Postgres Client Cert authentification into Quay.
$ cat configmap.yaml
apiVersion: v1
data:
PSQLSOURCES:
- secret:
name: <from-operator-or-config-tool>
optional: true
- secret:
name: <from-operator-or-config-tool>
optional: true
kind: ConfigMap
metadata:
creationTimestamp: null
name: generated
$ cat kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- quay.deployment.yaml
- configmap.yaml
- ...
replacements:
- source:
kind: ConfigMap
fieldPath: data.PSQLSOURCES
targets:
- select:
kind: Deployment
name: quay-app
fieldPaths:
- spec.template.spec.volumes.[name=postgres-tls-secrets].projected.sources
options:
create: true
$ kustomize build .
...
volumes:
- name: config
projected:
sources:
- secret:
name: quay-config-secret
- secret:
name: quay-config-tls
- name: extra-ca-certs
projected:
sources:
- configMap:
name: cluster-service-ca
- configMap:
name: cluster-trusted-ca
- secret:
name: extra-ca-certs
- name: postgres-tls-secrets
projected:
sources:
- secret:
name: <from-operator-or-config-tool>
optional: true
- secret:
name: <from-operator-or-config-tool>
optional: true
- emptyDir:
sizeLimit: 5Mi
name: postgres-certs-store
|
/retest |
|
@michaelalang: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
ocp-latest-e2e failure is legit: |
|
@dmage we have a typo in the secret name can we fix that without a PR or is it necessary to keep track of the changes related ? |
|
I strongly suggest configuring the secret names in the The user experience at the CR level should then look something like this: |
- Fix kustomize path for clairpgupgrade when clair is managed/unmanaged
- Remove base from pgupgrade path to ensure that the file location is parsed properly by kustomize package
Bumps [github.com/jackc/pgx/v4](https://github.com/jackc/pgx) from 4.11.0 to 4.18.2. - [Changelog](https://github.com/jackc/pgx/blob/v4.18.2/CHANGELOG.md) - [Commits](jackc/pgx@v4.11.0...v4.18.2) --- updated-dependencies: - dependency-name: github.com/jackc/pgx/v4 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
adding Postgres client certificate secret,mount overlay
in relation to config-tool extension of Postgres SSL Client authentication (quay/config-tool#214), adding postgresql overlay for mounting a projected secret composed by:
postgresql-capostgresql-client-certsusing the libpg default path for certs
/.postgresas optional mounts if the secrets exist and can be composed.This path is expected to be ephemeral as the Quay init scripts will ensure to get certificate, key and CA in place with proper permissions (PR in quay/quay).