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

Implement upgrade edges support #275

Merged
merged 2 commits into from
Aug 3, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
4 changes: 4 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,14 @@ kind-cluster-cleanup: $(KIND) ## Delete the kind cluster
$(KIND) delete cluster --name ${KIND_CLUSTER_NAME}

kind-load-test-artifacts: $(KIND) ## Load the e2e testdata container images into a kind cluster
$(CONTAINER_RUNTIME) build $(TESTDATA_DIR)/bundles/registry-v1/prometheus-operator.v0.37.0 -t localhost/testdata/bundles/registry-v1/prometheus-operator:v0.37.0
$(CONTAINER_RUNTIME) build $(TESTDATA_DIR)/bundles/registry-v1/prometheus-operator.v0.47.0 -t localhost/testdata/bundles/registry-v1/prometheus-operator:v0.47.0
$(CONTAINER_RUNTIME) build $(TESTDATA_DIR)/bundles/registry-v1/prometheus-operator.v0.65.1 -t localhost/testdata/bundles/registry-v1/prometheus-operator:v0.65.1
$(CONTAINER_RUNTIME) build $(TESTDATA_DIR)/bundles/plain-v0/plain.v0.1.0 -t localhost/testdata/bundles/plain-v0/plain:v0.1.0
$(CONTAINER_RUNTIME) build $(TESTDATA_DIR)/catalogs -f $(TESTDATA_DIR)/catalogs/test-catalog.Dockerfile -t localhost/testdata/catalogs/test-catalog:e2e
$(KIND) load docker-image localhost/testdata/bundles/registry-v1/prometheus-operator:v0.37.0 --name $(KIND_CLUSTER_NAME)
$(KIND) load docker-image localhost/testdata/bundles/registry-v1/prometheus-operator:v0.47.0 --name $(KIND_CLUSTER_NAME)
$(KIND) load docker-image localhost/testdata/bundles/registry-v1/prometheus-operator:v0.65.1 --name $(KIND_CLUSTER_NAME)
$(KIND) load docker-image localhost/testdata/bundles/plain-v0/plain:v0.1.0 --name $(KIND_CLUSTER_NAME)
$(KIND) load docker-image localhost/testdata/catalogs/test-catalog:e2e --name $(KIND_CLUSTER_NAME)

Expand Down
1 change: 1 addition & 0 deletions config/samples/operators_v1alpha1_operator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,4 @@ metadata:
name: operator-sample
spec:
packageName: argocd-operator
version: 0.6.0
3 changes: 3 additions & 0 deletions internal/controllers/variable_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ func NewVariableSource(cl client.Client) variablesources.NestedVariableSource {
func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
return variablesources.NewOperatorVariableSource(cl, inputVariableSource), nil
},
func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
return variablesources.NewBundleDeploymentVariableSource(cl, inputVariableSource), nil
},
func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
return variablesources.NewBundlesAndDepsVariableSource(inputVariableSource), nil
},
Expand Down
64 changes: 42 additions & 22 deletions internal/resolution/entities/bundle_entity.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,6 @@

// ----

type ChannelProperties struct {
property.Channel
Replaces string `json:"replaces,omitempty"`
Skips []string `json:"skips,omitempty"`
SkipRange string `json:"skipRange,omitempty"`
}

type propertyRequirement bool

const (
Expand All @@ -46,6 +39,10 @@

type GVK property.GVK

type Replaces struct {
Replaces string `json:"replaces"`
}

func (g GVK) String() string {
return fmt.Sprintf(`group:"%s" version:"%s" kind:"%s"`, g.Group, g.Version, g.Kind)
}
Expand All @@ -64,15 +61,16 @@
*input.Entity

// these properties are lazy loaded as they are requested
bundlePackage *property.Package
providedGVKs []GVK
requiredGVKs []GVKRequired
requiredPackages []PackageRequired
channelProperties *ChannelProperties
semVersion *semver.Version
bundlePath string
mediaType string
mu sync.RWMutex
bundlePackage *property.Package
providedGVKs []GVK
requiredGVKs []GVKRequired
requiredPackages []PackageRequired
channel *property.Channel
replaces *Replaces
semVersion *semver.Version
bundlePath string
mediaType string
mu sync.RWMutex
}

func NewBundleEntity(entity *input.Entity) *BundleEntity {
Expand Down Expand Up @@ -121,14 +119,34 @@
if err := b.loadChannelProperties(); err != nil {
return "", err
}
return b.channelProperties.ChannelName, nil
return b.channel.ChannelName, nil
}

func (b *BundleEntity) ChannelProperties() (*ChannelProperties, error) {
func (b *BundleEntity) Channel() (*property.Channel, error) {
if err := b.loadChannelProperties(); err != nil {
return nil, err
}
return b.channelProperties, nil
return b.channel, nil
}

func (b *BundleEntity) Replaces() (string, error) {
if err := b.loadReplaces(); err != nil {
return "", err

Check warning on line 134 in internal/resolution/entities/bundle_entity.go

View check run for this annotation

Codecov / codecov/patch

internal/resolution/entities/bundle_entity.go#L134

Added line #L134 was not covered by tests
}
return b.replaces.Replaces, nil
}

func (b *BundleEntity) loadReplaces() error {
b.mu.Lock()
defer b.mu.Unlock()
if b.replaces == nil {
replaces, err := loadFromEntity[Replaces](b.Entity, "olm.replaces", optional)
if err != nil {
return fmt.Errorf("error determining replaces for entity '%s': %w", b.ID, err)

Check warning on line 145 in internal/resolution/entities/bundle_entity.go

View check run for this annotation

Codecov / codecov/patch

internal/resolution/entities/bundle_entity.go#L145

Added line #L145 was not covered by tests
}
b.replaces = &replaces
}
return nil
}

func (b *BundleEntity) BundlePath() (string, error) {
Expand Down Expand Up @@ -228,12 +246,12 @@
func (b *BundleEntity) loadChannelProperties() error {
b.mu.Lock()
defer b.mu.Unlock()
if b.channelProperties == nil {
channel, err := loadFromEntity[ChannelProperties](b.Entity, property.TypeChannel, required)
if b.channel == nil {
channel, err := loadFromEntity[property.Channel](b.Entity, property.TypeChannel, required)
if err != nil {
return fmt.Errorf("error determining bundle channel properties for entity '%s': %w", b.ID, err)
}
b.channelProperties = &channel
b.channel = &channel
}
return nil
}
Expand All @@ -255,6 +273,8 @@
deserializedProperty := *new(T)
propertyValue, ok := entity.Properties[propertyName]
if ok {
// TODO: In order to avoid invalid properties we should use a decoder that only allows the properties we expect.
// ie. decoder.DisallowUnknownFields()
if err := json.Unmarshal([]byte(propertyValue), &deserializedProperty); err != nil {
return deserializedProperty, fmt.Errorf("property '%s' ('%s') could not be parsed: %w", propertyName, propertyValue, err)
}
Expand Down
85 changes: 72 additions & 13 deletions internal/resolution/entities/bundle_entity_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,28 +205,24 @@ var _ = Describe("BundleEntity", func() {
})
})

Describe("ChannelProperties", func() {
Describe("Channel", func() {
It("should return the bundle channel properties if present", func() {
entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{
"olm.channel": `{"channelName":"beta","priority":0, "replaces": "bundle.v1.0.0", "skips": ["bundle.v0.9.0", "bundle.v0.9.6"], "skipRange": ">=0.9.0 <=0.9.6"}`,
})
bundleEntity := olmentity.NewBundleEntity(entity)
channelProperties, err := bundleEntity.ChannelProperties()
channelProperties, err := bundleEntity.Channel()
Expect(err).ToNot(HaveOccurred())
Expect(*channelProperties).To(Equal(olmentity.ChannelProperties{
Channel: property.Channel{
ChannelName: "beta",
Priority: 0,
},
Replaces: "bundle.v1.0.0",
Skips: []string{"bundle.v0.9.0", "bundle.v0.9.6"},
SkipRange: ">=0.9.0 <=0.9.6",
}))
Expect(*channelProperties).To(Equal(property.Channel{
ChannelName: "beta",
Priority: 0,
},
))
})
It("should return an error if the property is not found", func() {
entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{})
bundleEntity := olmentity.NewBundleEntity(entity)
channelProperties, err := bundleEntity.ChannelProperties()
channelProperties, err := bundleEntity.Channel()
Expect(channelProperties).To(BeNil())
Expect(err.Error()).To(Equal("error determining bundle channel properties for entity 'operatorhub/prometheus/0.14.0': required property 'olm.channel' not found"))
})
Expand All @@ -235,7 +231,7 @@ var _ = Describe("BundleEntity", func() {
"olm.channel": "badChannelPropertiesStructure",
})
bundleEntity := olmentity.NewBundleEntity(entity)
channelProperties, err := bundleEntity.ChannelProperties()
channelProperties, err := bundleEntity.Channel()
Expect(channelProperties).To(BeNil())
Expect(err.Error()).To(Equal("error determining bundle channel properties for entity 'operatorhub/prometheus/0.14.0': property 'olm.channel' ('badChannelPropertiesStructure') could not be parsed: invalid character 'b' looking for beginning of value"))
})
Expand Down Expand Up @@ -269,6 +265,27 @@ var _ = Describe("BundleEntity", func() {
})
})

Describe("Replaces", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

It("should return the replaces property if present", func() {
entity := input.NewEntity("test", map[string]string{
"olm.replaces": `{"replaces": "test.v0.2.0"}`,
})
bundleEntity := olmentity.NewBundleEntity(entity)
replaces, err := bundleEntity.Replaces()
Expect(err).ToNot(HaveOccurred())
Expect(replaces).To(Equal("test.v0.2.0"))
})
It("should not return an error if the property is not found", func() {
entity := input.NewEntity("test", map[string]string{
"olm.thingy": `{"whatever":"this"}`,
})
bundleEntity := olmentity.NewBundleEntity(entity)
replaces, err := bundleEntity.Replaces()
Expect(replaces).To(BeEmpty())
Expect(err).NotTo(HaveOccurred())
})
})

Describe("MediaType", func() {
It("should return the bundle mediatype property if present", func() {
entity := input.NewEntity("operatorhub/prometheus/0.14.0", map[string]string{
Expand Down Expand Up @@ -296,4 +313,46 @@ var _ = Describe("BundleEntity", func() {
Expect(err.Error()).To(Equal("error determining bundle mediatype for entity 'operatorhub/prometheus/0.14.0': property 'olm.bundle.mediatype' ('badtype') could not be parsed: invalid character 'b' looking for beginning of value"))
})
})

// Increase test coverage
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I would remove this or add a TODO if this is meant to be done in the future, non-blocking though.

Describe("GVKRequired properties", func() {
It("should return the GVKRequired properties", func() {
gvk := olmentity.GVKRequired{
Group: "foo.io",
Kind: "Foo",
Version: "v1",
}
Expect(gvk.AsGVK().Version).To(Equal("v1"))
Expect(gvk.AsGVK().Group).To(Equal("foo.io"))
Expect(gvk.AsGVK().Kind).To(Equal("Foo"))
})
It("should return the GVKRequired properties as a string", func() {
gvk := olmentity.GVKRequired{
Group: "foo.io",
Kind: "Foo",
Version: "v1",
}
Expect(gvk.String()).To(Equal(`group:"foo.io" version:"v1" kind:"Foo"`))
})
})
Describe("GVK properties", func() {
It("should return the gvk properties", func() {
gvk := olmentity.GVK{
Group: "foo.io",
Kind: "Foo",
Version: "v1",
}
Expect(gvk.Version).To(Equal("v1"))
Expect(gvk.Group).To(Equal("foo.io"))
Expect(gvk.Kind).To(Equal("Foo"))
})
It("should return the gvk properties as a string", func() {
gvk := olmentity.GVK{
Group: "foo.io",
Kind: "Foo",
Version: "v1",
}
Expect(gvk.String()).To(Equal(`group:"foo.io" version:"v1" kind:"Foo"`))
})
})
})
7 changes: 7 additions & 0 deletions internal/resolution/entitysources/catalogdsource.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ func (es *CatalogdEntitySource) Iterate(ctx context.Context, fn input.IteratorFu
return nil
}

type replacesProperty struct {
Replaces string `json:"replaces"`
}

func getEntities(ctx context.Context, client client.Client) (input.EntityList, error) {
entityList := input.EntityList{}
bundleMetadatas, packageMetdatas, err := fetchMetadata(ctx, client)
Expand Down Expand Up @@ -106,6 +110,9 @@ func getEntities(ctx context.Context, client client.Client) (input.EntityList, e
if catalogScopedEntryName == bundle.Name {
channelValue, _ := json.Marshal(property.Channel{ChannelName: ch.Name, Priority: 0})
props[property.TypeChannel] = string(channelValue)
// TODO(jmprusi): Add the proper PropertyType for this
Copy link
Contributor

Choose a reason for hiding this comment

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

should we address this todo?

Copy link
Member

Choose a reason for hiding this comment

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

I know the following suggestion is a bigger refactor, and I'm not suggesting we do it now, but I think we should refactor things such that there are:

  • variable source for bundles
  • variable source for channels

Bundle entities would no longer carry upgrade edge or channel membership information. Rather, channel variables would incorporate bundle membership and upgrade edges.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, something like my suggestion should happen almost immediately after this PR lands so that we set ourselves up for a more extensible future where different channel implementations can exist and we don't have to keep adding synthetic properties to bundles (which might have undesirable effects if/when we support more arbitrary contraint definitions in the future)

replacesValue, _ := json.Marshal(replacesProperty{Replaces: b.Replaces})
props["olm.replaces"] = string(replacesValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we move this to a constant? (olm.replaces)

entity := input.Entity{
ID: deppy.IdentifierFromString(fmt.Sprintf("%s%s%s", bundle.Name, bundle.Spec.Package, ch.Name)),
Properties: props,
Expand Down
22 changes: 22 additions & 0 deletions internal/resolution/util/predicates/predicates.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,25 @@ func ProvidesGVK(gvk *olmentity.GVK) input.Predicate {
return false
}
}

func WithBundleImage(bundleImage string) input.Predicate {
return func(entity *input.Entity) bool {
bundleEntity := olmentity.NewBundleEntity(entity)
bundlePath, err := bundleEntity.BundlePath()
if err != nil {
return false
}
return bundlePath == bundleImage
}
}

func Replaces(bundleID string) input.Predicate {
return func(entity *input.Entity) bool {
bundleEntity := olmentity.NewBundleEntity(entity)
replaces, err := bundleEntity.Replaces()
if err != nil {
return false
}
return replaces == bundleID
}
}
55 changes: 55 additions & 0 deletions internal/resolution/util/predicates/predicates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ var _ = Describe("Predicates", func() {
Expect(predicates.WithPackageName("mypackage")(entity)).To(BeTrue())
Expect(predicates.WithPackageName("notmypackage")(entity)).To(BeFalse())
})
It("should return false when the entity does not have a package name", func() {
entity := input.NewEntity("test", map[string]string{})
Expect(predicates.WithPackageName("mypackage")(entity)).To(BeFalse())
})
})

Describe("InSemverRange", func() {
Expand All @@ -39,6 +43,11 @@ var _ = Describe("Predicates", func() {
Expect(predicates.InSemverRange(inRange)(entity)).To(BeTrue())
Expect(predicates.InSemverRange(notInRange)(entity)).To(BeFalse())
})
It("should return false when the entity does not have a version", func() {
entity := input.NewEntity("test", map[string]string{})
inRange := semver.MustParseRange(">=1.0.0")
Expect(predicates.InSemverRange(inRange)(entity)).To(BeFalse())
})
})

Describe("InChannel", func() {
Expand All @@ -49,6 +58,10 @@ var _ = Describe("Predicates", func() {
Expect(predicates.InChannel("stable")(entity)).To(BeTrue())
Expect(predicates.InChannel("unstable")(entity)).To(BeFalse())
})
It("should return false when the entity does not have a channel", func() {
entity := input.NewEntity("test", map[string]string{})
Expect(predicates.InChannel("stable")(entity)).To(BeFalse())
})
})

Describe("ProvidesGVK", func() {
Expand All @@ -67,5 +80,47 @@ var _ = Describe("Predicates", func() {
Kind: "Baz",
})(entity)).To(BeFalse())
})
It("should return false when the entity does not provide a gvk", func() {
entity := input.NewEntity("test", map[string]string{})
Expect(predicates.ProvidesGVK(&olmentity.GVK{
Group: "foo.io",
Version: "v1",
Kind: "Foo",
})(entity)).To(BeFalse())
})
})

Describe("WithBundleImage", func() {
It("should return true when the entity provides the same bundle image", func() {
entity := input.NewEntity("test", map[string]string{
olmentity.PropertyBundlePath: `"registry.io/repo/image@sha256:1234567890"`,
})
Expect(predicates.WithBundleImage("registry.io/repo/image@sha256:1234567890")(entity)).To(BeTrue())
Expect(predicates.WithBundleImage("registry.io/repo/image@sha256:0987654321")(entity)).To(BeFalse())
})
It("should return false when the entity does not provide a bundle image", func() {
entity := input.NewEntity("test", map[string]string{})
Expect(predicates.WithBundleImage("registry.io/repo/image@sha256:1234567890")(entity)).To(BeFalse())
})
})

Describe("Replaces", func() {
It("should return true when the entity provides the replaces property", func() {
entity := input.NewEntity("test", map[string]string{
"olm.replaces": `{"replaces": "test.v0.2.0"}`,
})
Expect(predicates.Replaces("test.v0.2.0")(entity)).To(BeTrue())
Expect(predicates.Replaces("test.v0.1.0")(entity)).To(BeFalse())
})
It("should return false when the entity does not provide a replaces property", func() {
entity := input.NewEntity("test", map[string]string{})
Expect(predicates.Replaces("test.v0.2.0")(entity)).To(BeFalse())
})
It("should return false when the replace value is not a valid json", func() {
entity := input.NewEntity("test", map[string]string{
"olm.replaces": `{"replaces"}`,
})
Expect(predicates.Replaces("test.v0.2.0")(entity)).To(BeFalse())
})
})
})
Loading
Loading