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

Updating cert-manager instructions #1842

Closed

Conversation

cniackz
Copy link
Contributor

@cniackz cniackz commented Oct 31, 2023

To fix: #1839

Explanation:

Our pods are expecting operator-ca-tls secret to contain public.crt but in our instructions we are passing down ca.crt instead and hence we see this error:

Screenshot 2023-10-31 at 10 40 59 AM

Solution:

Remove the secret from both namespaces and re-generate with proper and expected name public.crt and then all is back to normal with our cert-manager example we offer in our repo.

Additional Information:

cert-manager example got broken in Operator's version 5.0.8 under PR #1716

Bugfix reload CA cert in `operator-ca-tls` secret (#1716)

About 2 months ago, since then cert-manager example isn't working.
back then the issue was:

E1031 17:03:21.136276       1 main-controller.go:697] error syncing 'tenant-certmanager/myminio': missing 'public.crt' in minio-operator/operator-ca-tls secret

Then that issue got fixed but new one introduced:

MountVolume.SetUp failed for volume "myminio-tls" : references non-existent secret key: public.crt

And solution is this PR

ravindk89
ravindk89 previously approved these changes Oct 31, 2023
shtripat
shtripat previously approved these changes Oct 31, 2023
Copy link
Contributor

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

lgtm

cesnietor
cesnietor previously approved these changes Oct 31, 2023
feorlen
feorlen previously approved these changes Oct 31, 2023
@cniackz
Copy link
Contributor Author

cniackz commented Oct 31, 2023

@pjuarezd please review this and merge if you think this is the right solution 🙏 and thank you my friend!.

pjuarezd
pjuarezd previously approved these changes Oct 31, 2023
@chancez
Copy link

chancez commented Oct 31, 2023

Based on my testing, this does 'fix' the issue with the tenant being unable to start, but this breaks the ability for the operator to verify the tenant's TLS certificate.

I suspect the issue is in the original PRs that changed this months ago. As I stated in the comments on my original issue: none of this really makes sense to me. Why does the tenant need the CA? It's the operator that needs the CA, so why is it even being mounted into the tenant pod? I suspect the operator still needs the ca.crt in the secret to validate the CA, and the problem is that the tenant is mounting the operator-tls-ca secret in the first place, not that it should include public.crt.

@chancez
Copy link

chancez commented Oct 31, 2023

Also, instead of relying on 🤞 could we instead have a CI workflow that tests minio with TLS & cert-manager?

@cniackz cniackz force-pushed the correct-cert-manager-instructions branch from 9d24702 to fa504f6 Compare October 31, 2023 21:30
@cniackz
Copy link
Contributor Author

cniackz commented Oct 31, 2023

@chancez, I believe you're correct on this. Could you please give it another try? I've updated the PR to use 'tls.crt' instead and tested it with our 'cert-manager' example, and it worked in that manner as well.

Notice I am getting the tls.crt this time, documentation could look like: '{.data.tls\.crt}'

@cniackz
Copy link
Contributor Author

cniackz commented Oct 31, 2023

Screenshot 2023-10-31 at 5 38 03 PM

@chancez
Copy link

chancez commented Oct 31, 2023

@cniackz that isn't it, I wasn't having any problems extracting the CA. In my case I don't even need to do the first step, I already have the CA because I'm managing my own CA, so the first step isn't the issue and the latest changes don't fix the problem.

As I also mentioned earlier, there seems to be a potentially misunderstanding of how this is supposed to work, because there's no reason for the tenant to be mounting the CA from the operator-tls-ca file anyways, which is the reason things aren't working. The docs do not need updating at all, the docs are correct, it's the code that needs fixing.

To be clear, you shouldn't even need to do this dance of extracting anything from the tenant, I already have the CA. I precreate the operator-tls-ca before I even install my minio-operator and the tenant since I already have the CA ahead of time.

With your latest update, when you're extracting the tls.crt, you are not even using the CA anymore, just the server cert. This means you're trusting a single certificate, rather than all certificates signed by the CA. That just...completely defeats the point of a CA. The whole point of a CA is to have a central trust anchor. I do not want to have to add every every individual server certificate for every instance of minio, and then have to restart minio-operator, that doesn't make sense.

Try configuring some buckets like I provided in the original issue I filed. You should see the operator is unable to make the bucket because of TLS errors.

@cniackz
Copy link
Contributor Author

cniackz commented Nov 2, 2023

I finally see your point, yeah. This will require code change to fix that. Ok, I am going to close this PR for now. And find the peace of code that need to be fixed so we don't replicate the CA to the Tenant. Thank you for your time and feedback @chancez

@cniackz cniackz closed this Nov 2, 2023
@cniackz
Copy link
Contributor Author

cniackz commented Nov 2, 2023

The fix should address this: Operator does need the CA and that is ok, is still needed and docs are fine with it. Now, our current operator is propagating the CA to the Tenant which is what we don't actually need for this particular case. We need to find what part of the code is doing that and perform the change without breaking anything else. <--- TODO!.

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.

tenant install failed - references non-existent secret key: public.crt
7 participants