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

Introduce global.containerRegistryCACertSecret #1967

Merged
merged 2 commits into from
Dec 23, 2022
Merged

Introduce global.containerRegistryCACertSecret #1967

merged 2 commits into from
Dec 23, 2022

Conversation

gcapizzi
Copy link
Contributor

@gcapizzi gcapizzi commented Dec 6, 2022

Is there a related GitHub Issue?

#1920

What is this change about?

This introduces a way to set a custom CA certificate to be used when contacting the container registry. It does so by accepting a Secret containing a ca.crt key and mounting that key as a file under /etc/ssl/certs.

Given /etc/ssl/certs is one of the default locations used by Golang to build its system certificates pool, there's no more need to accept the REGISTRY_CA_FILE environment variable and set a dedicated http.Transport, so this PR removes that code.

Does this PR introduce a breaking change?

Yes, as REGISTRY_CA_FILE won't have any effect after this gets merged.

@gcapizzi
Copy link
Contributor Author

gcapizzi commented Dec 6, 2022

This is not properly tested yet.

I have modified deploy-on-kind.sh so that:

  • twuni/docker-registry was installed with tlsSecretName set to a Secret containing a certificate that I have signed with my own CA certificate;
  • Korifi was installed with global.containerRegistryCACertSecret set to a Secret containing the CA certificate.

I am getting the following error, which I don't really understand:

{"level":"info","ts":"2022-12-06T17:10:28.183923454Z","logger":"http-logger","msg":"request","correlation-id":"99ad93da-b47b-471f-aac4-73fbc6cf3492","url":"/v3/packages/b10204ab-f3eb-46bf-994f-bf357f4b1f4c/upload","method":"POST","remoteAddr":"10.244.0.17:39738","contentLength":921}
{"level":"info","ts":"2022-12-06T17:10:28.265822007Z","logger":"PackageHandler","msg":"Error calling UploadSourceImage","correlation-id":"99ad93da-b47b-471f-aac4-73fbc6cf3492","err":"pushing image ref 'localregistry-docker-registry.default.svc:30050/packages' failed: failed to upload image: HEAD https://localregistry-docker-registry.default.svc:30050/v2/packages/blobs/sha256:6ea850dc4998e09d838430999d44ef93f17a1f15d6db372894b49f01e4e2f487: unexpected status code 401 Unauthorized (HEAD responses have no body, use GET for details)"}
{"level":"info","ts":"2022-12-06T17:10:28.265962198Z","logger":"PackageHandler","msg":"handler returned error","correlation-id":"99ad93da-b47b-471f-aac4-73fbc6cf3492","error":"pushing image ref 'localregistry-docker-registry.default.svc:30050/packages' failed: failed to upload image: HEAD https://localregistry-docker-registry.default.svc:30050/v2/packages/blobs/sha256:6ea850dc4998e09d838430999d44ef93f17a1f15d6db372894b49f01e4e2f487: unexpected status code 401 Unauthorized (HEAD responses have no body, use GET for details)"}
{"level":"info","ts":"2022-12-06T17:10:28.266005943Z","logger":"http-logger","msg":"response","correlation-id":"99ad93da-b47b-471f-aac4-73fbc6cf3492","status":502,"size":131,"durationMillis":82}

Note
Due to a weird behaviour of go-containerregisty, a registry URL containing .local will switch the client to plain HTTP. I have worked around this by using localregistry-docker-registry.default.svc (without the final .cluster.local) everywhere.

@gcapizzi gcapizzi force-pushed the registry-ca branch 5 times, most recently from d6215ca to 24a0757 Compare December 12, 2022 12:35
@gcapizzi gcapizzi marked this pull request as ready for review December 12, 2022 14:52
@gcapizzi gcapizzi marked this pull request as draft December 12, 2022 14:53
@gcapizzi
Copy link
Contributor Author

I was able to test this locally all the way to a Kpack build-init failure. I tried to use cert-injection-webhook to fix it but with no success. I think this is mergeable, as our bit probably works (I was able to manually check the API change as it successfully uploaded the sources, I wasn't able to check the kpack-image-builder change as I never got to a built image). I'll keep this as a draft as me might want to wait until after releasing before merging it.

This allows users to specify a CA cert for the container registry. It
also gets rid of the `REGISTRY_CA_FILE` environment variable and of the
custom transport construction we had in place: instead we just mount the
cert unser `/etc/ssl/certs`, which is one of the dirs automatically
scanned by Golang when building its system CAs pool.

Co-authored-by: Giuseppe Capizzi <gcapizzi@vmware.com>
Copy link
Member

@georgethebeatle georgethebeatle left a comment

Choose a reason for hiding this comment

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

LGTM

@gcapizzi gcapizzi merged commit 8d23d75 into main Dec 23, 2022
@gcapizzi gcapizzi deleted the registry-ca branch December 23, 2022 13:12
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.

3 participants