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

Add an ability to use non-https registries #152

Merged
merged 5 commits into from
May 6, 2024
Merged

Conversation

allanger
Copy link
Contributor

@allanger allanger commented Apr 24, 2024

In order to make the "per WC" cache work, we need to use http://127.0.0.1 in a containerd config, because the caching registry is exposed as a node port.

It would also be possible to use tls, but then we need to have a cert that is trusted by containerd, and it doesn's sount that easy

Issue: https://github.com/giantswarm/giantswarm/issues/30596

What does this PR do?

This PR is supposed to let use http registries mirrors, when it's explicitly provided via endpoints

Currently, when I try to set http://127.0.0.1:5000 as a mirror, I'm getting a config that contains https://http://127.0.0.1.

What is the effect of this change to users?

It should not affect users.

Any background context you can provide?

It would make per WC cache deployment a way easier

What is needed from the reviewers?

Check, if this change doesn't affect users unless they explicitly want to be affected

Do the docs need to be updated?

I don't think so

Should this change be mentioned in the release notes?

I don't think so

  • CHANGELOG.md has been updated (if it exists)

@allanger allanger requested a review from a team as a code owner April 24, 2024 12:52
@allanger allanger self-assigned this Apr 24, 2024
@allanger allanger marked this pull request as draft April 24, 2024 12:55
@allanger allanger marked this pull request as ready for review April 24, 2024 12:57
@njuettner
Copy link
Member

@giantswarm/team-turtles what about setting this as a hardcoded string? Meaning adding "http://127.0.0.1" directly underneath the range?

That would at least prevent adding more non-https value endpoints. So we basically don't add additional endpoints via https and http.

Thoughts?

@weseven
Copy link
Contributor

weseven commented Apr 24, 2024

@giantswarm/team-turtles what about setting this as a hardcoded string? Meaning adding "http://127.0.0.1" directly underneath the range?

That would at least prevent adding more non-https value endpoints. So we basically don't add additional endpoints via https and http.

Thoughts?

Makes sense to me, I can't think of other use cases for using http registries.

@allanger
Copy link
Contributor Author

what about setting this as a hardcoded string?

We won't be able to hardcode the port though, would you be ok with something like that:

#values
localMirror:
  enable: true
  port: 33445
# template
...
{{ - if .Values.localMirror.enabled -}}
"http://127.0.0.1:{{ .Values.localMirror.port }}
{{- end -}}
...

@allanger
Copy link
Contributor Author

But with that values structure, I'm a bit uncertain where to put that localCache entry and what it should look like when creating a cluster via happa, do you have ideas?

In order to make the "per WC" cache work, we need to use
http://127.0.0.1 in a containerd config, because the caching registry is
exposed as a node port.

It would also be possible to use tls, but then we need to have a cert
that is trusted by containerd, and it doesn's sount that easy

Issue: https://github.com/giantswarm/giantswarm/issues/30596
In order to prevent users from using http registries, I've added a new
values entry, that can configure http mirror only for local cache.

Issue: https://github.com/giantswarm/giantswarm/issues/30596
@allanger
Copy link
Contributor Author

@giantswarm/team-turtles Is it better now?

@allanger
Copy link
Contributor Author

allanger commented May 2, 2024

@weseven or @njuettner
May I please ask you to review it again?

@weseven
Copy link
Contributor

weseven commented May 2, 2024

LGTM, only nitpicks would be naming of values:

  • instead of localCache I would use localRegistryCache
  • instead of mirrors I'd use mirroredRegistries
    But I don't feel strongly about these, so not blocking.

Not sure if .internal it's the right place: I think so, but better wait for @njuettner confirmation on that before merging.

@njuettner
Copy link
Member

Agree it makes sense to name them as Daniel suggested
localCache ->localRegistryCache
mirrors -> mirroredRegistries

to make it more explicit 🙂

Other than that LGTM, feel free merge it once you changed the names

Copy link

github-actions bot commented May 6, 2024

There were differences in the rendered Helm template, please check! ⚠️

Output
=== Differences when rendered with values file helm/cluster/ci/test-required-values.yaml ===

/data/registry-config.toml  (v1/Secret/awesome-registry-configuration)
  ± value change
    - dmVyc2lvbiA9IDIKCiMgcmVjb21tZW5kZWQgZGVmYXVsdHMgZnJvbSBodHRwczovL2dpdGh1Yi5jb20vY29udGFpbmVyZC9jb250YWluZXJkL2Jsb2IvbWFpbi9kb2NzL29wcy5tZCNiYXNlLWNvbmZpZ3VyYXRpb24KIyBzZXQgY29udGFpbmVyZCBhcyBhIHN1YnJlYXBlciBvbiBsaW51eCB3aGVuIGl0IGlzIG5vdCBydW5uaW5nIGFzIFBJRCAxCnN1YnJlYXBlciA9IHRydWUKIyBzZXQgY29udGFpbmVyZCdzIE9PTSBzY29yZQpvb21fc2NvcmUgPSAtOTk5CmRpc2FibGVkX3BsdWdpbnMgPSBbXQpbcGx1Z2lucy4iaW8uY29udGFpbmVyZC5ydW50aW1lLnYxLmxpbnV4Il0KIyBzaGltIGJpbmFyeSBuYW1lL3BhdGgKc2hpbSA9ICJjb250YWluZXJkLXNoaW0iCiMgcnVudGltZSBiaW5hcnkgbmFtZS9wYXRoCnJ1bnRpbWUgPSAicnVuYyIKIyBkbyBub3QgdXNlIGEgc2hpbSB3aGVuIHN0YXJ0aW5nIGNvbnRhaW5lcnMsIHNhdmVzIG9uIG1lbW9yeSBidXQKIyBsaXZlIHJlc3RvcmUgaXMgbm90IHN1cHBvcnRlZApub19zaGltID0gZmFsc2UKCltwbHVnaW5zLiJpby5jb250YWluZXJkLmdycGMudjEuY3JpIi5jb250YWluZXJkLnJ1bnRpbWVzLnJ1bmNdCiMgc2V0dGluZyBydW5jLm9wdGlvbnMgdW5zZXRzIHBhcmVudCBzZXR0aW5ncwpydW50aW1lX3R5cGUgPSAiaW8uY29udGFpbmVyZC5ydW5jLnYyIgpbcGx1Z2lucy4iaW8uY29udGFpbmVyZC5ncnBjLnYxLmNyaSIuY29udGFpbmVyZC5ydW50aW1lcy5ydW5jLm9wdGlvbnNdClN5c3RlbWRDZ3JvdXAgPSBmYWxzZQpbcGx1Z2lucy4iaW8uY29udGFpbmVyZC5ncnBjLnYxLmNyaSJdCnNhbmRib3hfaW1hZ2UgPSAiZ3NvY2kuYXp1cmVjci5pby9naWFudHN3YXJtL3BhdXNlOjMuOSIKCltwbHVnaW5zLiJpby5jb250YWluZXJkLmdycGMudjEuY3JpIi5yZWdpc3RyeV0KICBbcGx1Z2lucy4iaW8uY29udGFpbmVyZC5ncnBjLnYxLmNyaSIucmVnaXN0cnkubWlycm9yc10KICAgIFtwbHVnaW5zLiJpby5jb250YWluZXJkLmdycGMudjEuY3JpIi5yZWdpc3RyeS5taXJyb3JzLiJkb2NrZXIuaW8iXQogICAgICBlbmRwb2ludCA9IFsiaHR0cHM6Ly9yZWdpc3RyeS0xLmRvY2tlci5pbyIsImh0dHBzOi8vZ2lhbnRzd2FybS5henVyZWNyLmlvIixdCltwbHVnaW5zLiJpby5jb250YWluZXJkLmdycGMudjEuY3JpIi5yZWdpc3RyeS5jb25maWdzXQogIFtwbHVnaW5zLiJpby5jb250YWluZXJkLmdycGMudjEuY3JpIi5yZWdpc3RyeS5jb25maWdzLiJyZWdpc3RyeS0xLmRvY2tlci5pbyIuYXV0aF0KICAgICAgYXV0aCA9ICJaMmxoYm5SemQyRnliVHB6ZFhCbGNsOXpaV055WlhSZmNHRnpjM2R2Y21RPSIK
    + dmVyc2lvbiA9IDIKCiMgcmVjb21tZW5kZWQgZGVmYXVsdHMgZnJvbSBodHRwczovL2dpdGh1Yi5jb20vY29udGFpbmVyZC9jb250YWluZXJkL2Jsb2IvbWFpbi9kb2NzL29wcy5tZCNiYXNlLWNvbmZpZ3VyYXRpb24KIyBzZXQgY29udGFpbmVyZCBhcyBhIHN1YnJlYXBlciBvbiBsaW51eCB3aGVuIGl0IGlzIG5vdCBydW5uaW5nIGFzIFBJRCAxCnN1YnJlYXBlciA9IHRydWUKIyBzZXQgY29udGFpbmVyZCdzIE9PTSBzY29yZQpvb21fc2NvcmUgPSAtOTk5CmRpc2FibGVkX3BsdWdpbnMgPSBbXQpbcGx1Z2lucy4iaW8uY29udGFpbmVyZC5ydW50aW1lLnYxLmxpbnV4Il0KIyBzaGltIGJpbmFyeSBuYW1lL3BhdGgKc2hpbSA9ICJjb250YWluZXJkLXNoaW0iCiMgcnVudGltZSBiaW5hcnkgbmFtZS9wYXRoCnJ1bnRpbWUgPSAicnVuYyIKIyBkbyBub3QgdXNlIGEgc2hpbSB3aGVuIHN0YXJ0aW5nIGNvbnRhaW5lcnMsIHNhdmVzIG9uIG1lbW9yeSBidXQKIyBsaXZlIHJlc3RvcmUgaXMgbm90IHN1cHBvcnRlZApub19zaGltID0gZmFsc2UKCltwbHVnaW5zLiJpby5jb250YWluZXJkLmdycGMudjEuY3JpIi5jb250YWluZXJkLnJ1bnRpbWVzLnJ1bmNdCiMgc2V0dGluZyBydW5jLm9wdGlvbnMgdW5zZXRzIHBhcmVudCBzZXR0aW5ncwpydW50aW1lX3R5cGUgPSAiaW8uY29udGFpbmVyZC5ydW5jLnYyIgpbcGx1Z2lucy4iaW8uY29udGFpbmVyZC5ncnBjLnYxLmNyaSIuY29udGFpbmVyZC5ydW50aW1lcy5ydW5jLm9wdGlvbnNdClN5c3RlbWRDZ3JvdXAgPSBmYWxzZQpbcGx1Z2lucy4iaW8uY29udGFpbmVyZC5ncnBjLnYxLmNyaSJdCnNhbmRib3hfaW1hZ2UgPSAiZ3NvY2kuYXp1cmVjci5pby9naWFudHN3YXJtL3BhdXNlOjMuOSIKCltwbHVnaW5zLiJpby5jb250YWluZXJkLmdycGMudjEuY3JpIi5yZWdpc3RyeV0KICBbcGx1Z2lucy4iaW8uY29udGFpbmVyZC5ncnBjLnYxLmNyaSIucmVnaXN0cnkubWlycm9yZWRSZWdpc3RyaWVzXQogICAgW3BsdWdpbnMuImlvLmNvbnRhaW5lcmQuZ3JwYy52MS5jcmkiLnJlZ2lzdHJ5Lm1pcnJvcmVkUmVnaXN0cmllcy4iZG9ja2VyLmlvIl0KICAgICAgZW5kcG9pbnQgPSBbImh0dHBzOi8vcmVnaXN0cnktMS5kb2NrZXIuaW8iLCJodHRwczovL2dpYW50c3dhcm0uYXp1cmVjci5pbyIsXQpbcGx1Z2lucy4iaW8uY29udGFpbmVyZC5ncnBjLnYxLmNyaSIucmVnaXN0cnkuY29uZmlnc10KICBbcGx1Z2lucy4iaW8uY29udGFpbmVyZC5ncnBjLnYxLmNyaSIucmVnaXN0cnkuY29uZmlncy4icmVnaXN0cnktMS5kb2NrZXIuaW8iLmF1dGhdCiAgICAgIGF1dGggPSAiWjJsaGJuUnpkMkZ5YlRwemRYQmxjbDl6WldOeVpYUmZjR0Z6YzNkdmNtUT0iCg==
  

@piontec piontec merged commit d97abd8 into main May 6, 2024
12 checks passed
@piontec piontec deleted the allow-http-mirror branch May 6, 2024 12:52
nprokopic added a commit that referenced this pull request May 14, 2024
@nprokopic nprokopic mentioned this pull request May 14, 2024
1 task
nprokopic added a commit that referenced this pull request May 14, 2024
* Revert "Add an ability to use non-https registries (#152)"

This reverts commit d97abd8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants