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

mounting Operator CA in minio not longer required #1847

Merged

Conversation

cniackz
Copy link
Contributor

@cniackz cniackz commented Nov 2, 2023

Objective:

To avoid mounting Operator's CA into MinIO if not required.

Explanation:

This is the continuation of #1437 because of those changes, Tenants no longer need to communicate with Operator webservers over TLS to get startup arguments or create bucket DNS. Also when using cert-manager we don't need to use this CA certificate in the tenant as cert-manager is already doing this for us; so this is redundant, look:

Screenshot 2023-11-02 at 1 39 10 PM

Related PRs:

Related issues:

Question:

And if it is required, why? and where? as I can't find a reason for these lines anymore.

@cniackz cniackz self-assigned this Nov 2, 2023
@pjuarezd
Copy link
Member

pjuarezd commented Nov 2, 2023

Operator still needs to trust the tenants CA to hit the healtchcheck endpoint, specially when a custom certificate is provided to tenants, as long as operator-ca-tls secret is not deprecated and we can still mount it in Operator this is OK.

@cniackz
Copy link
Contributor Author

cniackz commented Nov 2, 2023

Operator still needs to trust the tenants CA to hit the healtchcheck endpoint <--- Yes this is true, and in the case of cert-manager, we do have instructions on how to do this and it works!.

@cniackz
Copy link
Contributor Author

cniackz commented Nov 2, 2023

operator-ca-tls is not deprecated and is still needed, this PR is only avoiding the mount of the CA in this secret as is not needed.

@cniackz
Copy link
Contributor Author

cniackz commented Nov 2, 2023

...we can still mount it in Operator this is OK.

Yes, we can still mount it in operator

@cniackz
Copy link
Contributor Author

cniackz commented Nov 2, 2023

I am just waiting for tests to pass to ask for a review

@feorlen
Copy link
Contributor

feorlen commented Nov 2, 2023

What would make sense to say in the docs? Specifically I'm thinking the sections that include this information, perhaps other locations as well.

https://min.io/docs/minio/kubernetes/upstream/operations/installation.html#kubernetes-tls-certificate-api

The MinIO Operator reads certificates inside the operator-ca-tls secret and syncs this secret within the tenant namespace to trust private certificate authorities, such as when using cert-manager.

https://min.io/docs/minio/kubernetes/upstream/operations/network-encryption.html#self-signed-internal-and-private-certificates

Once the secret is created, Operator creates a copy in each tenant namespace and mounts its certificates in the tenant pods at /tmp/certs/CAs/.

@ravindk89
Copy link
Contributor

If I'm reading the code right @feorlen , this would actually eliminate that behavior entirely.

Moving forward, operator-ca-tls is required in general if using self-signed, internal, private, or public CAs that require intermediate certs. But we no longer need to extract or move certs to the Tenant at all.

The Tenant has what it needs already. The Operator only needs a copy of the CA, wherever that lives.

@feorlen
Copy link
Contributor

feorlen commented Nov 3, 2023

But we no longer need to extract or move certs to the Tenant at all. The Tenant has what it needs already. The Operator only needs a copy of the CA, wherever that lives.

We can condense the current detail and say it's no longer a concern starting with version whatever. Yay. One less thing for people to think about.

Copy link
Contributor

@jiuker jiuker left a comment

Choose a reason for hiding this comment

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

LGTM

@pjuarezd pjuarezd merged commit 7d23f73 into minio:master Nov 14, 2023
25 checks passed
pjuarezd added a commit to pjuarezd/operator that referenced this pull request May 23, 2024
…tificates stored in secrets with the prefix "operator-ca-tls."

* No longer copy the secret `operator-ca-tls` from the operator namespace to the tenants namespace: Since [PR minio#1847](minio#1847), the secret `operator-ca-tls` is no longer mounted in the tenant, so there is no need to keep a copy.
* `queue.NewNamedRateLimitingQueue` is deprecated and has been replaced with the recommended `queue.NewRateLimitingQueueWithConfig`.
* Remove the duplicated method `getTLSSecret` and invoke `getCertificateSecret` instead.
* Rename [generateTLSCert](https://github.com/minio/operator/blob/1c2fa4f402cc2c91c9903e6da6e9a693c92b65e4/pkg/controller/tls.go#L108) to `generateTLSCertificateForService` for better understanding.
* Remove duplicated constants for 'public.crt', 'tls.crt', and 'ca.crt' in the `github.com/minio/operator/pkg/common` namespace.
* Replace hardcoded strings 'public.crt', 'tls.crt', and 'ca.crt' with constants in the `github.com/minio/operator/pkg/certs` namespace.

Signed-off-by: pjuarezd <[email protected]>
pjuarezd added a commit to pjuarezd/operator that referenced this pull request May 24, 2024
…tificates stored in secrets with the prefix "operator-ca-tls."

* No longer copy the secret `operator-ca-tls` from the operator namespace to the tenants namespace: Since [PR minio#1847](minio#1847), the secret `operator-ca-tls` is no longer mounted in the tenant, so there is no need to keep a copy.
* `queue.NewNamedRateLimitingQueue` is deprecated and has been replaced with the recommended `queue.NewRateLimitingQueueWithConfig`.
* Remove the duplicated method `getTLSSecret` and invoke `getCertificateSecret` instead.
* Rename [generateTLSCert](https://github.com/minio/operator/blob/1c2fa4f402cc2c91c9903e6da6e9a693c92b65e4/pkg/controller/tls.go#L108) to `generateTLSCertificateForService` for better understanding.
* Remove duplicated constants for 'public.crt', 'tls.crt', and 'ca.crt' in the `github.com/minio/operator/pkg/common` namespace.
* Replace hardcoded strings 'public.crt', 'tls.crt', and 'ca.crt' with constants in the `github.com/minio/operator/pkg/certs` namespace.

Signed-off-by: pjuarezd <[email protected]>
pjuarezd added a commit to pjuarezd/operator that referenced this pull request May 24, 2024
…tificates stored in secrets with the prefix "operator-ca-tls."

* No longer copy the secret `operator-ca-tls` from the operator namespace to the tenants namespace: Since [PR minio#1847](minio#1847), the secret `operator-ca-tls` is no longer mounted in the tenant, so there is no need to keep a copy.
* `queue.NewNamedRateLimitingQueue` is deprecated and has been replaced with the recommended `queue.NewRateLimitingQueueWithConfig`.
* Remove the duplicated method `getTLSSecret` and invoke `getCertificateSecret` instead.
* Rename [generateTLSCert](https://github.com/minio/operator/blob/1c2fa4f402cc2c91c9903e6da6e9a693c92b65e4/pkg/controller/tls.go#L108) to `generateTLSCertificateForService` for better understanding.
* Remove duplicated constants for 'public.crt', 'tls.crt', and 'ca.crt' in the `github.com/minio/operator/pkg/common` namespace.
* Replace hardcoded strings 'public.crt', 'tls.crt', and 'ca.crt' with constants in the `github.com/minio/operator/pkg/certs` namespace.

Signed-off-by: pjuarezd <[email protected]>
pjuarezd added a commit that referenced this pull request May 28, 2024
* Listen for secret changes in the operator namespace and trust TLS certificates stored in secrets with the prefix "operator-ca-tls."

* No longer copy the secret `operator-ca-tls` from the operator namespace to the tenants namespace: Since [PR #1847](#1847), the secret `operator-ca-tls` is no longer mounted in the tenant, so there is no need to keep a copy.
* `queue.NewNamedRateLimitingQueue` is deprecated and has been replaced with the recommended `queue.NewRateLimitingQueueWithConfig`.
* Remove the duplicated method `getTLSSecret` and invoke `getCertificateSecret` instead.
* Rename [generateTLSCert](https://github.com/minio/operator/blob/1c2fa4f402cc2c91c9903e6da6e9a693c92b65e4/pkg/controller/tls.go#L108) to `generateTLSCertificateForService` for better understanding.
* Remove duplicated constants for 'public.crt', 'tls.crt', and 'ca.crt' in the `github.com/minio/operator/pkg/common` namespace.
* Replace hardcoded strings 'public.crt', 'tls.crt', and 'ca.crt' with constants in the `github.com/minio/operator/pkg/certs` namespace.

Signed-off-by: pjuarezd <[email protected]>

---------

Signed-off-by: pjuarezd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants