-
Notifications
You must be signed in to change notification settings - Fork 118
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
Bug 1701422: use port 443 for registry service #259
Conversation
/assign @dmage |
pkg/operator/controller.go
Outdated
@@ -66,6 +66,10 @@ func NewController(kubeconfig *restclient.Config) (*Controller, error) { | |||
|
|||
p := parameters.Globals{} | |||
|
|||
p.Service.Name = imageregistryv1.ImageRegistryName | |||
p.Service.Port = 443 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this as a parameter may give wrong feeling that this a parameter that can be changed. But at the same time we have fmt.Sprintf("%s.%s.svc", svc.Name, svc.Namespace)
that makes this value impossible to change without additional modifications. It worth mention that we rely on the fact that this port is the default port Docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can drop this (in which case I can drop the TargetPort as well, since that should be the port on the container)
a027a6a
to
c5ea07c
Compare
Use the default HTTPS port for the internal registry service. This lets us drop the port when referencing images on cluster.
return false, fmt.Errorf("expected internal registry hostname %s; got %s", | ||
"image-registry.openshift-image-registry.svc", | ||
cfg.Status.InternalRegistryHostname) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fail fast if the internal registry host name doesn't match what we expect.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, dmage The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
/hold Not clear why we are consistently failing |
/retest |
1 similar comment
/retest |
/hold cancel |
/retest |
got past the auth issue, now seeing a service name collision on imagestream tag import:
|
/retest Please review the full test history for this PR and help us cut down flakes. |
9 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/hold |
@adambkaplan: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
Per thread in aos-devel, we don't plan on landing this until 4.2 |
@adambkaplan: PR needs rebase. 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. |
/close Replaced by #322 |
@adambkaplan: Closed this PR. 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. |
Use the default HTTPS port for the internal registry service.
This lets us drop the port when referencing images on cluster.