Skip to content

[OCPNODE-553] Migrate ICSP to ImageDigest,TagMirrorSet CRDs #15

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 2 commits into from
Sep 26, 2022
Merged

[OCPNODE-553] Migrate ICSP to ImageDigest,TagMirrorSet CRDs #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb75088
Add support ImageDigest,TagMirrorSet CRDs
QiWang19 Aug 16, 2022
f2400d4
bump c/image
QiWang19 Sep 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,31 +3,30 @@ module github.com/openshift/runtime-utils
go 1.18

require (
github.com/BurntSushi/toml v0.3.1
github.com/containers/image/v5 v5.11.0
github.com/BurntSushi/toml v1.2.0
github.com/containers/image/v5 v5.22.0
github.com/openshift/api v0.0.0-20220901185337-0b39f81154fa
github.com/openshift/build-machinery-go v0.0.0-20220720161851-9b4f0386f6b0
github.com/stretchr/testify v1.7.0
github.com/stretchr/testify v1.8.0
)

require (
github.com/containers/storage v1.28.1 // indirect
github.com/containers/storage v1.42.0 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/docker/go-units v0.4.0 // indirect
github.com/go-logr/logr v1.2.3 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/google/gofuzz v1.1.0 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/moby/sys/mountinfo v0.4.1 // indirect
github.com/moby/sys/mountinfo v0.6.2 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/opencontainers/go-digest v1.0.0 // indirect
github.com/opencontainers/image-spec v1.0.2-0.20190823105129-775207bd45b6 // indirect
github.com/opencontainers/runc v1.0.0-rc93 // indirect
github.com/opencontainers/runtime-spec v1.0.3-0.20200929063507-e6143ca7d51d // indirect
github.com/pkg/errors v0.9.1 // indirect
github.com/opencontainers/image-spec v1.0.3-0.20220114050600-8b9d41f48198 // indirect
github.com/opencontainers/runc v1.1.3 // indirect
github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/sirupsen/logrus v1.8.1 // indirect
github.com/sirupsen/logrus v1.9.0 // indirect
github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635 // indirect
golang.org/x/net v0.0.0-20220722155237-a158d28d115b // indirect
golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f // indirect
Expand Down
894 changes: 857 additions & 37 deletions go.sum
View file
Open in desktop

Large diffs are not rendered by default.

240 changes: 178 additions & 62 deletions pkg/registries/registries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"strings"

"github.com/containers/image/v5/pkg/sysregistriesv2"
apicfgv1 "github.com/openshift/api/config/v1"
apioperatorsv1alpha1 "github.com/openshift/api/operator/v1alpha1"
)

Expand Down Expand Up @@ -35,81 +36,159 @@ func ScopeIsNestedInsideScope(subScope, superScope string) bool {
return match
}

// rdmContainsARealMirror returns true if set.Mirrors contains at least one entry that is not set.Source.
func rdmContainsARealMirror(set *apioperatorsv1alpha1.RepositoryDigestMirrors) bool {
for _, mirror := range set.Mirrors {
if mirror != set.Source {
// mirrorsContainsARealMirror returns true if mirrors contains at least one entry that is not source.
func mirrorsContainsARealMirror(source string, mirrors []apicfgv1.ImageMirror) bool {
for _, mirror := range mirrors {
if string(mirror) != source {
return true
}
}
return false
}

// mergedMirrorSets processes icspRules and returns a set of RepositoryDigestMirrors, one for each Source value,
// ordered consistently with the preference order of the individual entries (if possible)
// E.g. given mirror sets (B, C) and (A, B), it will combine them into a single (A, B, C) set.
func mergedMirrorSets(icspRules []*apioperatorsv1alpha1.ImageContentSourcePolicy) ([]apioperatorsv1alpha1.RepositoryDigestMirrors, error) {
disjointSets := map[string]*[]*apioperatorsv1alpha1.RepositoryDigestMirrors{} // Key == Source
for _, icsp := range icspRules {
for i := range icsp.Spec.RepositoryDigestMirrors {
set := &icsp.Spec.RepositoryDigestMirrors[i]
if !rdmContainsARealMirror(set) {
continue // No mirrors (or mirrors that only repeat the authoritative source) is not really a mirror set.
}
ds, ok := disjointSets[set.Source]
if !ok {
ds = &[]*apioperatorsv1alpha1.RepositoryDigestMirrors{}
disjointSets[set.Source] = ds
// mirrorSet collects data from mirror setting CRDs (ImageDigestMirrorSet, ImageTagMirrorSet)
type mirrorSets struct {
disjointSets map[string]*[][]string // Key == Source
mirrorBlockSource map[string]bool // key == Source
}

func newMirrorSets() *mirrorSets {
return &mirrorSets{
disjointSets: map[string]*[][]string{},
mirrorBlockSource: map[string]bool{},
}
}

func (sets *mirrorSets) addMirrorSet(source string, mirrorSourcePolicy apicfgv1.MirrorSourcePolicy, mirrors []apicfgv1.ImageMirror) {
if !mirrorsContainsARealMirror(source, mirrors) {
return // No mirrors (or mirrors that only repeat the authoritative source) is not really a mirror set. Ignore mirrorSourcePolicy intentionally.
}
strMirrors := []string{}
for _, m := range mirrors {
strMirrors = append(strMirrors, (string(m)))
}
Comment on lines +66 to +69
Copy link
Contributor

@mtrmac mtrmac Sep 5, 2022

Choose a reason for hiding this comment

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

Absolutely non-blocking: This copy is probably avoidable, perhaps with some more casts in the merging implementation.

(It’s a bit weird for ICSP to do a copy to convert to ImageMirror, only for this code to convert back to strings.)

But the cost is trivial either way, I’m not worried about it at all.

if mirrorSourcePolicy == apicfgv1.NeverContactSource {
sets.mirrorBlockSource[source] = true
}
ds, ok := sets.disjointSets[source]
if !ok {
ds = &[][]string{}
sets.disjointSets[source] = ds
}
*ds = append(*ds, strMirrors)
}

// mergedMirrors generates deterministic order of mirrors for a given source
func (sets *mirrorSets) mergedMirrors(source string) ([]string, error) {
topoGraph := newTopoGraph()
for _, mirrors := range *sets.disjointSets[source] {
for i := 0; i+1 < len(mirrors); i++ {
topoGraph.AddEdge(mirrors[i], mirrors[i+1])
}
sourceInGraph := false
for _, m := range mirrors {
if m == source {
sourceInGraph = true
break
}
*ds = append(*ds, set)
}
if !sourceInGraph {
// mirrorSets.addMirrorSet guarantees len(set.Mirrors) > 0.
topoGraph.AddEdge(mirrors[len(mirrors)-1], source)
}
// Every node in topoGraph, including source, is implicitly added by topoGraph.AddEdge (every mirror set contains at least one non-source mirror,
// so there are no unconnected nodes that we would have to add separately from the edges).
}
sortedRepos, err := topoGraph.Sorted()
if err != nil {
return nil, err
}
if sortedRepos[len(sortedRepos)-1] == source {
// We don't need to explicitly include source in the list, it will be automatically tried last per the semantics of sysregistriesv2. Mirrors.
sortedRepos = sortedRepos[:len(sortedRepos)-1]
}

if err != nil {
return nil, err
}
return sortedRepos, nil
}

type mergedMirrorSet struct {
source string
mirrors []string
mirrorSourcePolicy apicfgv1.MirrorSourcePolicy
Copy link
Contributor

@mtrmac mtrmac Sep 5, 2022

Choose a reason for hiding this comment

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

Absolutely non-blocking: This could be a blockSource bool, to remove the need to worry about unknown string values. OTOH this string might be more useful in the test data.

}

// mergedMirrorSets converts the set of mirrors to slice of mergedMirrorSet
func mergedMirrorSets(sets *mirrorSets) ([]mergedMirrorSet, error) {
// Sort the sets of mirrors by Source to ensure deterministic output
sources := []string{}
for key := range disjointSets {
for key := range sets.disjointSets {
sources = append(sources, key)
}
// collects the mirror sources and sorted in increasing order
sort.Strings(sources)
// Convert the sets of mirrors
res := []apioperatorsv1alpha1.RepositoryDigestMirrors{}
res := []mergedMirrorSet{}
for _, source := range sources {
ds := disjointSets[source]
topoGraph := newTopoGraph()
for _, set := range *ds {
for i := 0; i+1 < len(set.Mirrors); i++ {
topoGraph.AddEdge(set.Mirrors[i], set.Mirrors[i+1])
}
sourceInGraph := false
for _, m := range set.Mirrors {
if m == source {
sourceInGraph = true
break
}
}
if !sourceInGraph {
// The build of mirrorSets guarantees len(set.Mirrors) > 0.
topoGraph.AddEdge(set.Mirrors[len(set.Mirrors)-1], source)
}
// Every node in topoGraph, including source, is implicitly added by topoGraph.AddEdge (every mirror set contains at least one non-source mirror,
// so there are no unconnected nodes that we would have to add separately from the edges).
}
sortedRepos, err := topoGraph.Sorted()
mirrors, err := sets.mergedMirrors(source)
if err != nil {
return nil, err
}
if sortedRepos[len(sortedRepos)-1] == source {
// We don't need to explicitly include source in the list, it will be automatically tried last per the semantics of sysregistriesv2. Mirrors.
sortedRepos = sortedRepos[:len(sortedRepos)-1]
item := mergedMirrorSet{
source: source,
mirrors: mirrors,
}
res = append(res, apioperatorsv1alpha1.RepositoryDigestMirrors{
Source: source,
Mirrors: sortedRepos,
})
if sets.mirrorBlockSource[source] {
item.mirrorSourcePolicy = apicfgv1.NeverContactSource
}
res = append(res, item)
}
return res, nil
}

// mergedTagMirrorSets processes itmsRules and returns a set of mergedMirrorSet, one for each Source value,
// ordered consistently with the preference order of the individual entries (if possible)
// E.g. given mirror sets (B, C) and (A, B), it will combine them into a single (A, B, C) set.
func mergedTagMirrorSets(itmsRules []*apicfgv1.ImageTagMirrorSet) ([]mergedMirrorSet, error) {
tagMirrorSets := newMirrorSets()
for _, itms := range itmsRules {
for _, set := range itms.Spec.ImageTagMirrors {
tagMirrorSets.addMirrorSet(set.Source, set.MirrorSourcePolicy, set.Mirrors)
}
}
return mergedMirrorSets(tagMirrorSets)
}

// mergedDigestMirrorSets processes idmsRules and returns a set of mergedMirrorSet, one for each Source value,
// ordered consistently with the preference order of the individual entries (if possible)
// E.g. given mirror sets (B, C) and (A, B), it will combine them into a single (A, B, C) set.
func mergedDigestMirrorSets(idmsRules []*apicfgv1.ImageDigestMirrorSet) ([]mergedMirrorSet, error) {
digestMirrorSets := newMirrorSets()
for _, idms := range idmsRules {
for _, set := range idms.Spec.ImageDigestMirrors {
digestMirrorSets.addMirrorSet(set.Source, set.MirrorSourcePolicy, set.Mirrors)
}
}
return mergedMirrorSets(digestMirrorSets)
}

func mergedICSPMirrorSets(icspRules []*apioperatorsv1alpha1.ImageContentSourcePolicy) ([]mergedMirrorSet, error) {
repositoryMirrorSets := newMirrorSets()
for _, icsp := range icspRules {
for _, set := range icsp.Spec.RepositoryDigestMirrors {
imgMirrors := []apicfgv1.ImageMirror{}
for _, m := range set.Mirrors {
imgMirrors = append(imgMirrors, apicfgv1.ImageMirror(m))
}
// leave MirrorSourcePolicy blank, it will follow the default AllowContactingSource
repositoryMirrorSets.addMirrorSet(set.Source, "", imgMirrors)
}
}
return mergedMirrorSets(repositoryMirrorSets)
}

// mirrorsAdjustedForNestedScope returns mirrors from mirroredScope, updated
// so that they can be configured in a nested subScope, without any change in the
// semantics of the mirrors.
Expand Down Expand Up @@ -150,14 +229,19 @@ func registryScope(reg *sysregistriesv2.Registry) string {
// - Mark scope entries in insecureScopes as insecure (TLS is not required, and TLS certificate verification is not required when TLS is used)
// - Mark scope entries in blockedScopes as blocked (any attempts to access them fail)
// - Implement ImageContentSourcePolicy rules in icspRules.
// - Implement ImageDigestMirrorSet rules in idmsRules.
// - Implement ImageTagMirrorSet rules in itmsRules.
// "scopes" can be any of whole registries, which means that the configuration applies to everything on that registry, including any possible separately-configured
// namespaces/repositories within that registry.
// or can be wildcard entries, which means that we accept wildcards in the form of *.example.registry.com for insecure and blocked registries only. We do not
// accept them for mirror configuration.
// A valid scope is in the form of registry/namespace...[/repo] (can also refer to sysregistriesv2.Registry.Prefix)
// NOTE: Validation of wildcard entries is done before EditRegistriesConfig is called in the MCO code.
func EditRegistriesConfig(config *sysregistriesv2.V2RegistriesConf, insecureScopes, blockedScopes []string, icspRules []*apioperatorsv1alpha1.ImageContentSourcePolicy) error {

func EditRegistriesConfig(config *sysregistriesv2.V2RegistriesConf, insecureScopes, blockedScopes []string, icspRules []*apioperatorsv1alpha1.ImageContentSourcePolicy,
idmsRules []*apicfgv1.ImageDigestMirrorSet, itmsRules []*apicfgv1.ImageTagMirrorSet) error {
if err := RejectMultiUpdateMirrorSetObjs(icspRules, idmsRules, itmsRules); err != nil {
return err
}
// addRegistryEntry creates a Registry object corresponding to scope.
// NOTE: The pointer is valid only until the next getRegistryEntry call.
addRegistryEntry := func(scope string) *sysregistriesv2.Registry {
Expand Down Expand Up @@ -187,17 +271,35 @@ func EditRegistriesConfig(config *sysregistriesv2.V2RegistriesConf, insecureScop
return addRegistryEntry(scope)
}

mirrorSets, err := mergedMirrorSets(icspRules)
addMirrorsToRegistries := func(mergedMirrorSets []mergedMirrorSet, pullFromMirror string) {
for _, mirrorItem := range mergedMirrorSets {
reg := getRegistryEntry(mirrorItem.source)
if mirrorItem.mirrorSourcePolicy == apicfgv1.NeverContactSource {
reg.Blocked = true
}
for _, mirror := range mirrorItem.mirrors {
reg.Mirrors = append(reg.Mirrors, sysregistriesv2.Endpoint{Location: mirror, PullFromMirror: pullFromMirror})
}
}
}

digestMirrorSets, err := mergedDigestMirrorSets(idmsRules)
if err != nil {
return err
}
for _, mirrorSet := range mirrorSets {
reg := getRegistryEntry(mirrorSet.Source)
reg.MirrorByDigestOnly = true
for _, mirror := range mirrorSet.Mirrors {
reg.Mirrors = append(reg.Mirrors, sysregistriesv2.Endpoint{Location: mirror})
}
addMirrorsToRegistries(digestMirrorSets, sysregistriesv2.MirrorByDigestOnly)

icspMirrorSets, err := mergedICSPMirrorSets(icspRules)
if err != nil {
return err
}
addMirrorsToRegistries(icspMirrorSets, sysregistriesv2.MirrorByDigestOnly)

tagMirrorSets, err := mergedTagMirrorSets(itmsRules)
if err != nil {
return err
}
addMirrorsToRegistries(tagMirrorSets, sysregistriesv2.MirrorByTagOnly)

// Add the blocked registry entries to the registries list so that we can find sub-scopes of insecure registries and set both the
// blocked and insecure flags accordingly.
Expand Down Expand Up @@ -238,8 +340,10 @@ func EditRegistriesConfig(config *sysregistriesv2.V2RegistriesConf, insecureScop
}
}
}
for _, mirrorSet := range mirrorSets {
mirroredReg := getRegistryEntry(mirrorSet.Source)

allMirrorSets := append(icspMirrorSets, append(digestMirrorSets, tagMirrorSets...)...)
for _, mirrorSet := range allMirrorSets {
mirroredReg := getRegistryEntry(mirrorSet.source)
mirroredScope := registryScope(mirroredReg)
for i := range config.Registries {
reg := &config.Registries[i]
Expand All @@ -248,7 +352,6 @@ func EditRegistriesConfig(config *sysregistriesv2.V2RegistriesConf, insecureScop
// So, if there is any mirror defined for a more specific sub-scope of mirrorSet.Source,
// it must already exist with non-empty reg.Mirrors.
if scope != mirroredScope && ScopeIsNestedInsideScope(scope, mirroredScope) && len(reg.Mirrors) == 0 {
reg.MirrorByDigestOnly = mirroredReg.MirrorByDigestOnly
updated, err := mirrorsAdjustedForNestedScope(mirroredScope, scope, mirroredReg.Mirrors)
Copy link
Contributor

@mtrmac mtrmac Sep 1, 2022

Choose a reason for hiding this comment

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

A test case for this in the insecure+blocked scopes inside a configured mirror situation would be nice.

if err != nil {
return err
Expand Down Expand Up @@ -278,3 +381,16 @@ func IsValidRegistriesConfScope(scope string) bool {
}
return false
}

// RejectMultiUpdateMirrorSetObjs returns error if icsp objects exist with imagedigestmirrorset or imagetagmirrorset objects
// to avoid existing mirror settings get updated by others
func RejectMultiUpdateMirrorSetObjs(icspRules []*apioperatorsv1alpha1.ImageContentSourcePolicy,
Copy link
Contributor

@mtrmac mtrmac Sep 1, 2022

Choose a reason for hiding this comment

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

So the new plan is again not to have a controller sync between the old/new objects, and for every consumer to read both the old and new values?

(That’s fine, I’m just trying to keep up.)

Copy link
Contributor

@mtrmac mtrmac Sep 1, 2022

Choose a reason for hiding this comment

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

Oh, you have actually confirmed exactly that elsewhere already.

idmsRules []*apicfgv1.ImageDigestMirrorSet, itmsRules []*apicfgv1.ImageTagMirrorSet) error {
if len(icspRules) > 0 && len(idmsRules) > 0 {
return fmt.Errorf("error: both imagecontentsourcepolicy and imagedigestmirrorset exist")
}
if len(icspRules) > 0 && len(itmsRules) > 0 {
return fmt.Errorf("error: both imagecontentsourcepolicy and imagetagmirrorset exist")
}
return nil
}
Loading