-
Notifications
You must be signed in to change notification settings - Fork 401
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
OCPBUGS-36344: Add CIP relevant mirrors to sigstore attachement cfg #4449
OCPBUGS-36344: Add CIP relevant mirrors to sigstore attachement cfg #4449
Conversation
Skipping CI for Draft Pull Request. |
f771a5e
to
7a610ae
Compare
@QiWang19: This pull request references Jira Issue OCPBUGS-36344, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
/jira refresh |
@QiWang19: This pull request references Jira Issue OCPBUGS-36344, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (schoudha@redhat.com), skipping review request. 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 openshift-eng/jira-lifecycle-plugin repository. |
/test all |
@QiWang19: This pull request references Jira Issue OCPBUGS-36344, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (schoudha@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
@mtrmac could you review? |
7a610ae
to
60fd9c8
Compare
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.
Overall it seems to me that doing this with the ICSP/ITMS/IDMS data is harder than it needs to be; instead, the code could consume the already-generated registries.conf
, or maybe this could be built along with registries.conf
in runtime-utils/pkg/registries
.
Something vaguely like:
registries.EditRegistriesConf(config, …)
tmpFile := marshalAndWriteTempFile(config)
systemContext := types.SystemContext{SystemRegistriesConfPath: tmpFile, SystemRegistriesConfDirPath: "/dev/null"}
attachmentScopes := Set[string].New()
for policyScope in allPolicyScopesFromClusterImagePolicy{
parent := sysregistriesv2.FindRegistry(systemContext, policyScope)
addPhysicalLocations(attachmentScopes, parent, policyScope)
for reg in config.Registries {
scope = reg.Prefix ?? reg.Location
if registries.ScopeIsNestedInsideScope(scope, policyScope) && scope != policyScope {
nested := sysregistriesv2.FindRegistry(systemContext, scope) // Finds (a preprocessed version of) reg
addPhysicalLocations(attachmentScopes, nested, scope)
}
}
createRegistriesD(attachmentScopes)
// ---
func addPhysicalLocations(dest *Set[string], reg Registry, scope string) {
// Ugly… we need to look up endpoints for a taged and a digested reference, to account for ITMS/IDMS
endpoints := reg.PullSourcesFromReference(scope + ":tag")
endpoints += reg.PullSourcesFromReference(scope + "@sha256:aaaa…")
dest.Add(endpoints)
}
There are various ugly parts in there, admittedly, maybe c/image should gain a few more APIs (“load data from in-memory config”, “return all endpoints ignoring tag/digest restrictions”, …)
Arguably, it might not be strictly necessary to be precise in the So, I think, at the very least, it should not be hard-coded enabled via |
4f979d0
to
60d9968
Compare
@mtrmac Could you review? |
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.
A fairly brief skim so that I’m not blocking progress; I didn’t read the tests, I’ll try to do that tomorrow.
if registriesTOML == nil { | ||
continue | ||
} | ||
parentReg, err := sysregistriesv2.FindRegistry(&types.SystemContext{SystemRegistriesConfPath: tmpFile.Name()}, policyScope) |
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 think this should set SystemRegistriesConfDirPath
to something non-existent or not-a-directory, so that the controller doesn’t read current configuration of the node/container where it is currently running.
(In both places, ideally by sharing the SystemContext
)
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.
Thanks for the explanation.
registriesDockerConfig[scope] = sigstoreAttachment | ||
|
||
for policyScope := range clusterScopePolicies { | ||
registriesDockerConfig[policyScope] = sigstoreAttachment |
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.
(Non-blocking: The entry for the primary repo is not necessary if NeverContactSource
… but it also doesn’t really hurt; and otherwise we would need to special-case the “no configuration for registry” code path, i.e. this is a bit simpler than being 100% precise.)
scope = strings.Replace(scope, "*", dummyPrefix, 1) | ||
} | ||
|
||
scopeRef, err := reference.Parse(scope) |
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.
[As a general rule, prefer ParseNormalizedNamed
. It shouldn’t make a difference for valid scopes, … but do we actually validate that? IsValidRegistriesConfScope
seems rather loose, and I’m not sure that there is any other location validating scopes more strictly.
I’m … worried what happens in the “scope == host name” case — AFAICS reference.Parse
turns that into "[nothing]/$input"
. But it might work well enough.
OTOH with ParseNormalizedNamed
, the dummyPath
part would really have to happen before this parsing. See elsewhere.]
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.
On second thought, ParseNormalizedNamed("docker.io/library")
would produce an unwanted result (or we would have to work around that even more); reference.Parse()
is really better here.
if digestRef, digested := scopeRef.(reference.Digested); digested { | ||
namedRef, err := reference.ParseNamed(digestRef.String()) | ||
if err != nil { | ||
return fmt.Errorf("error parsing scope digested reference %s: %w", digestRef.String(), err) | ||
} | ||
scope = reference.TrimNamed(namedRef).String() | ||
|
||
} | ||
if tagRef, tagged := scopeRef.(reference.Tagged); tagged { | ||
namedRef, err := reference.ParseNamed(tagRef.String()) | ||
if err != nil { | ||
return fmt.Errorf("error parsing scope tagged reference %s: %w", tagRef.String(), err) | ||
} | ||
scope = reference.TrimNamed(namedRef).String() | ||
} |
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 think this can be just scope = scopeRef.Name()
, or scopeRef = reference.TrimNamed(scopeRef)
, without specifically checking for tagged/digested.
scope = reference.TrimNamed(namedRef).String() | ||
} | ||
if !strings.Contains(scope, "/") { | ||
scope += dummyPath |
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.
Ugh… I didn’t realize there’s this corner case.
It’s safe to do because we have already looked up reg
, but, AFAICS, if we add dummyPath
here, we must also strip it from the returned endpoints.
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.
… or, alternatively, unconditionally add dummyPath
, and unconditionally remove it. Either way, ugly…
digestRef, err := reference.ParseNamed(scope + dummyDigest) | ||
if err != nil { | ||
return fmt.Errorf("error parsing digest name for scope %s: %w", scope, err) | ||
} | ||
tagRef, err := reference.ParseNamed(scope + dummyTag) | ||
if err != nil { | ||
return fmt.Errorf("error parsing tag name for scope %s: %w", scope, err) | ||
} |
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.
(Non-blocking? This does work, but it does a bit more parsing than necessary. I think:
// dummyPrefix
if !strings.Contains(scope, "/") { // implies no tag and no digest, so this should be safe
scope += dummyPath
addedDummy = true
}
repo := reference.ParseNormalizedNamed(scope).TrimNamed()
digestRef = reference.WithDigest(repo, digest.Parse(dummyDigest))
tagRef = reference.WithTag(repo, dummyTag)
should work.
endpoint := s.Endpoint.Location | ||
if endpoint == "" { | ||
endpoint = reg.Prefix | ||
} |
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.
AFAICS this should be strings.TrimSuffix(s.Reference.Name(), dummyPathIfUsed)
; s.Endpoint
is the “root” of the mirror, not the final remapped repo (consider an original scope
of example.com/foo/bar
, and a mirror defined for example.com/foo
)
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.
A more complete review: the two stand-outs are not setting SystemRegistriesConfDirPath
and the Endpoint
vs. Reference
difference; the rest is mostly cosmetic.
If we absolutely had to, I’d be fine with merging as is and fixing that shortly after, e.g. in the immediately following sprint.
`docker: | ||
'*.example.com': | ||
use-sigstore-attachments: true | ||
a-a1.mirror/a1: |
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.
a.com/a1/a2
is mirrored to a-a1.mirror/a2/a2
, so that should be the scope. (See Reference
vs. Endpoint
in the previous review brach.)
`docker: | ||
'*.example.com': | ||
use-sigstore-attachments: true | ||
'*.scope': |
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 don’t think this should be added (probably Endpoint
vs. Reference
?)
use-sigstore-attachments: true | ||
'*.scope': | ||
use-sigstore-attachments: true | ||
'*.x.example.com': |
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.
This doesn’t hurt but also doesn’t need to be added — *.x.example.com
is a subset of *.example.com
use-sigstore-attachments: true | ||
a.com/a1/a2@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: | ||
use-sigstore-attachments: true | ||
a.example.com: |
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.
Doesn’t hurt but doesn’t need to be added.
855ca0d
to
78f043b
Compare
if hasWildcard { | ||
if endpoint == strings.Replace(originalScope, "*", dummyPrefix, 1) { | ||
continue | ||
} | ||
} |
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.
@mtrmac Could you review this? We also need to add a check for dummyPrefix
.
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’m afraid that my suggestion to use the c/image matching code has not been simpler — certainly not much simpler.)
To be explicit, the situation is a config entry with
prefix: *.example.com
location
not set — OCP never does that for wild-carded scopes- and perhaps some mirrors, perhaps not, e.g. this entry might exist because there was an “insecure” bool set
And in that case the source reference is returned as is.
Here, unlike the added dummyPath
, I’m worried about collisions with user-defined names (e.g. a mirroring setup from *.example.com
to matched.example.com
). I don’t think just a string comparison can resolve that.
It seems to me that this needs a condition && s.Endpoint.Location == ""
to rule out this case: mirror endpoints must, by definition, have a mirror location
. So if we have Location
set, the host name was user-supplied; if Location
is unset, the host name comes from dummyPrefix
.
(Alternatively, I think the Location == ""
check could be the only condition here, but then all primary endpoints would be ruled out and would have to be added at the top level. That would actually match the addScopeMirrorsSigstoreRegistriesdConfig
function name a bit better — it would be adding only mirrors, and that would be good enough for currently OCP-generated configs, OTOH it would be less correct for processing registries.conf
in general, because top-level redirection (`prefix: "logical"; location: "actualServer") would not be handled. I think we can handle top-level redirection here “for free”, so it’s better to do so than to risk future surprises.)
Please make sure there’s a unit test for this situation, and document what the situation and what the condition is checking for. (Something vaguely like “if we had to add a dummyPrefix
to process a wildcard Registry
entry, we only want to configure the mirror endpoints, if any; not to add a new entry for exactly dummyPrefix
+originalScope
”).
Also, this code would now depend on the caller unconditionally configuring attachments for registriesDockerConfig[policyScope]
— that also seems worth having a comment.
scope = reg.Prefix | ||
} | ||
if runtimeutils.ScopeIsNestedInsideScope(scope, policyScope) && scope != policyScope { | ||
nestedReg, err := sysregistriesv2.FindRegistry(&types.SystemContext{SystemRegistriesConfPath: tmpFile.Name()}, scope) |
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.
This place also needs updating with SystemRegistriesConfDirPath
.
(Please create a single SystemContext
variable to share that.)
return fmt.Errorf("error parsing scope %s: %w", scope, err) | ||
} | ||
|
||
repo := reference.TrimNamed(scopeRef.(reference.Named)) |
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.
Non-blocking: This cast probably can’t fail, but double-checking might be easier than worrying about crashes.
require.Len(t, gotRegistriesCfg.Docker, len(tc.expectedSigstoreRegistriesConfigScopes)) | ||
for _, scope := range tc.expectedSigstoreRegistriesConfigScopes { |
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.
assert.ElementsMatch(…, expected, maps.Keys(gotRegistriesCfg.Docker))
would check not just a subset relationship, but equality.
21e2187
to
f1c20bd
Compare
Spec: apicfgv1.ImageDigestMirrorSetSpec{ | ||
ImageDigestMirrors: []apicfgv1.ImageDigestMirrors{ | ||
{Source: "a.example.com", Mirrors: []apicfgv1.ImageMirror{"a-example.mirror"}}, | ||
{Source: "*.x.example.com", Mirrors: []apicfgv1.ImageMirror{"matched.example.mirror", "star-x.example.mirror"}}, |
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.
@mtrmac PTAL. Test cases for #4449 (comment)
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.
Thanks.
Also a test for policy scope foo.example.com/ns/repo
vs *.example.com
-> our.mirror/example
(expected scopes foo.example.com/ns/repo. our.mirror/example/ns/repo
) might be a interesting because it might be closer to real-world configurations (with fairly coarse mirror maps but fairly small policy scopes); I don’t expect it would change code coverage…
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.
We got foo.example.com/ns/repo
, our.mirror/example/ns/repo
, our.mirror/example
https://github.com/openshift/machine-config-operator/pull/4449/files#diff-e75cbabf033d7cdc22ac1877e6e0134549c848c7226729274f806c3ab943f37eR1366
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.
Thanks!
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.
Implementation LGTM
sigstoreAttachment = dockerConfig{ | ||
UseSigstoreAttachments: true, | ||
} | ||
sysContext = &types.SystemContext{ |
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.
(This is only used on the registriesTOML != nil
path, so it can be initialized there. A context with SystemRegistriesConfPath
set but SystemRegistriesConfPath
does not make all that much sense. But this works fine as is.)
Add icsp/idms/itms mirrors of CIP scope to /etc/containers/registries.d, so sigstore attachment will be used during the image pull and verification. Signed-off-by: Qi Wang <qiwan@redhat.com>
f1c20bd
to
861d9af
Compare
/lgtm |
The unit test failure seems not to be obviously related, but I didn’t investigate beyond reading the backtrace. |
/test unit |
/assign @saschagrunert |
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.
/retest
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac, QiWang19, saschagrunert 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 |
@QiWang19: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
35ce1c1
into
openshift:master
@QiWang19: Jira Issue OCPBUGS-36344: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-36344 has been moved to the MODIFIED state. 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 openshift-eng/jira-lifecycle-plugin repository. |
[ART PR BUILD NOTIFIER] This PR has been included in build ose-machine-config-operator-container-v4.17.0-202407111341.p0.g35ce1c1.assembly.stream.el9 for distgit ose-machine-config-operator. |
Close: #4446
- What I did
- How to verify it
Cluster 4.17.0-0.ci.test-2024-07-08-173847 has default ICSP:
Apply CIP:
Pull from mirror, check the log:
Looking for sigstore attachments
- Description for the changelog
Add icsp/idms/itms mirrors of CIP scope to /etc/containers/registries.d, so sigstore attachment will be used during the image pull and verification.