OCPBUGS-54298: Installer should validate mirror registry pull secret#10239
Conversation
|
@yunjiang29: This pull request references Jira Issue OCPBUGS-54298, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@yunjiang29: This pull request references Jira Issue OCPBUGS-54298, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
b6c199f to
e902733
Compare
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-4.22-amd64-nightly-aws-ipi-disc-priv-sts-ep-fips-f14 |
|
@yunjiang29: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e7df98e0-f5bd-11f0-84b6-fc38317e86bc-0 |
|
|
|
@yunjiang29: This pull request references Jira Issue OCPBUGS-54298, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by @gpei
|
|
@gpei: This PR has been marked as verified by DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
tthvo
left a comment
There was a problem hiding this comment.
Great, thank you! I just have a few suggestions :D
| if errors.Is(err, dockerref.ErrNameNotCanonical) { | ||
| host, port, err := net.SplitHostPort(repository) | ||
| if err != nil { | ||
| return repository, nil | ||
| } | ||
| return net.JoinHostPort(host, port), nil | ||
| } |
There was a problem hiding this comment.
I think this part eventually returns the registry value itself, right?
- If
registryishost:port, split successfully but then rejoin to the same original value. - Otherwise fails, and return the original value.
How about simplifying it a tiny bit?
// extractRegistryHost extracts the registry host (with port if any) from a repository string.
// For example: "registry.example.com:5000/namespace/repo" -> "registry.example.com:5000".
// Returns an error if the repository string cannot be parsed as either a named reference or a host.
func extractRegistryHost(repository string) (string, error) {
ref, err := dockerref.ParseNamed(repository)
if err != nil {
// ErrNameNotCanonical indicates the input is not a fully-qualified repository reference
// (e.g., "registry.example.com:5000" without a path, or short names like "ocp/release").
// In these cases, return the input as-is.
if errors.Is(err, dockerref.ErrNameNotCanonical) {
return repository, nil
}
return "", err
}
return dockerref.Domain(ref), nil
}| var ps imagePullSecret | ||
| validPullSecret := false | ||
| mirrorHostsMissingSecret := []string{} | ||
|
|
||
| if err := validate.ImagePullSecret(pullSecret); err == nil { | ||
| if err := json.Unmarshal([]byte(pullSecret), &ps); err == nil { | ||
| validPullSecret = true | ||
| } | ||
| } |
There was a problem hiding this comment.
I notice we have duplicate logic for validateImageContentSources and validateImageDigestSources :D
How about extracting it into its own validation func? This way, if we want to return error later, we can just easily do so.
// validateMirrorCredentials checks if mirror registry hosts are present in the pull secret
func validateMirrorCredentials(mirrors []string, pullSecret string) field.ErrorList {
allErrs := field.ErrorList{}
var ps imagePullSecret
if err := validate.ImagePullSecret(pullSecret); err != nil {
return allErrs
}
if err := json.Unmarshal([]byte(pullSecret), &ps); err != nil {
return allErrs
}
missingHosts := sets.New[string]()
for _, mirror := range mirrors {
mirrorHost, err := extractRegistryHost(mirror)
if err != nil {
continue // Skip if we can't extract the host
}
if _, found := ps.Auths[mirrorHost]; !found {
missingHosts.Insert(mirrorHost)
}
}
for host := range missingHosts {
// Log warnings for registries without credentials
// FIXME: We should instead report it as errors
logrus.Warnf("Mirror registry %q is not found in pullSecret", host)
}
return allErrs
}Then we can call the function, for example:
func validateImageDigestSources(groups []types.ImageDigestSource, pullSecret string, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
var allMirrors []string
for gidx, group := range groups {
// ... code-omitted ...
for midx, mirror := range group.Mirrors {
// ... code-omitted ...
allMirrors = append(allMirrors, mirror)
}
// ... code-omitted ...
}
allErrs = append(allErrs, validateMirrorCredentials(allMirrors, pullSecret)...)
return allErrs
}There was a problem hiding this comment.
@tthvo thanks for you detailed feedback, I've incorporated your suggestions
// Log warnings for registries without credentials
// FIXME: We should instead report it as errors
logrus.Warnf("Mirror registry %q is not found in pullSecret", host)
As per's Zane's suggestion described in the bug, a warning is good for the first version (we can upgrade to an error later), the warning is sufficient to help us debugging.
| }(), | ||
| }, | ||
| { | ||
| name: "valid imageContentSources with mirror not in pull secret", |
There was a problem hiding this comment.
nit: I think we might want to be clear that it does not throw the error and it just shows warning, but technically invalid right? How about naming them like:
imageContentSources with mirror not in pull secret - warning only
e902733 to
f7d69af
Compare
tthvo
left a comment
There was a problem hiding this comment.
/approve
LGTM! We need to fix some strict golint complaints though 👇
pkg/types/validation/installconfig.go:1723:1: Comment should end in a period (godot)
// validateMirrorCredentials checks if mirror registry hosts are present in the pull secret
^
|
/cc @patrickdillon @zaneb |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tthvo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Log a warning if mirror registry host not present in pullSecret.
f7d69af to
ad04ec1
Compare
|
Tested again and it works as expected like #10239 (comment). /verified by @gpei |
|
@gpei: This PR has been marked as verified by DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
/test okd-scos-images |
|
/override ci/prow/okd-scos-images |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/okd-scos-images DetailsIn 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-sigs/prow repository. |
|
@yunjiang29: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
@yunjiang29: Jira Issue Verification Checks: Jira Issue OCPBUGS-54298 Jira Issue OCPBUGS-54298 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
Log a warning if mirror registry host not present in pullSecret: