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

Handle channelName #172

Merged
merged 6 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ install.sh
*.swp
*.swo
*~
\#*\#
.\#*

# TODO dfranz remove this line and the bin folder when tools binaries are moved to their own folder
!bin/.dockerignore
2 changes: 1 addition & 1 deletion api/v1alpha1/operator_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ type OperatorSpec struct {
Version string `json:"version,omitempty"`

//+kubebuilder:validation:MaxLength:=48
//+kubebuilder:validation:Pattern:=^[a-z0-9]+(-[a-z0-9]+)*$
//+kubebuilder:validation:Pattern:=^[a-z0-9]+([\.-][a-z0-9]+)*$
// Channel constraint defintion
Channel string `json:"channel,omitempty"`
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ spec:
channel:
description: Channel constraint defintion
maxLength: 48
pattern: ^[a-z0-9]+(-[a-z0-9]+)*$
pattern: ^[a-z0-9]+([\.-][a-z0-9]+)*$
type: string
packageName:
maxLength: 48
Expand Down
45 changes: 45 additions & 0 deletions controllers/admission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,49 @@ var _ = Describe("Operator Spec Validations", func() {
Expect(err.Error()).To(ContainSubstring("spec.version in body should match '^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(-(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\\.(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?(\\+([0-9a-zA-Z-]+(\\.[0-9a-zA-Z-]+)*))?$'"))
}
})
It("should fail if an invalid channel name is given", func() {
invalidChannels := []string{
"spaces spaces",
"Capitalized",
"camelCase",
"many/invalid$characters+in_name",
"-start-with-hyphen",
"end-with-hyphen-",
".start-with-period",
"end-with-period.",
}
for _, invalidChannel := range invalidChannels {
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Channel: invalidChannel,
}))
Expect(err).To(HaveOccurred(), "expected error for invalid channel '%q'", invalidChannel)
Expect(err.Error()).To(ContainSubstring("spec.channel in body should match '^[a-z0-9]+([\\.-][a-z0-9]+)*$'"))
}
})
It("should pass if a valid channel name is given", func() {
validChannels := []string{
"hyphenated-name",
"dotted.name",
"channel-has-version-1.0.1",
}
for _, validChannel := range validChannels {
op := operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Channel: validChannel,
})
err := cl.Create(ctx, op)
Expect(err).NotTo(HaveOccurred(), "unexpected error creating valid channel '%q': %w", validChannel, err)
err = cl.Delete(ctx, op)
Expect(err).NotTo(HaveOccurred(), "unexpected error deleting valid channel '%q': %w", validChannel, err)
}
})
It("should fail if an invalid channel name length", func() {
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
PackageName: "package",
Channel: "longname01234567890123456789012345678901234567890",
}))
Expect(err).To(HaveOccurred(), "expected error for invalid channel length")
Expect(err.Error()).To(ContainSubstring("spec.channel: Too long: may not be longer than 48"))
})
})
201 changes: 200 additions & 1 deletion controllers/operator_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ var _ = Describe("Operator Controller Test", func() {
var pkgName string
BeforeEach(func() {
By("initializing cluster state")
pkgName = fmt.Sprintf("exists-%s", rand.String(6))
pkgName = "prometheus"
operator = &operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
Spec: operatorsv1alpha1.OperatorSpec{
Expand Down Expand Up @@ -594,6 +594,205 @@ var _ = Describe("Operator Controller Test", func() {
})

})
When("the operator specifies a channel with version that exist", func() {
var pkgName string
var pkgVer string
var pkgChan string
BeforeEach(func() {
By("initializing cluster state")
pkgName = "prometheus"
pkgVer = "0.47.0"
Copy link
Member

Choose a reason for hiding this comment

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

Should we leave the version unset for the base spec.channel tests so we can isolate the behavior we're expecting and avoid side effects from setting the version?

(We should probably also have tests where both are set to test for interactions, but I think that's outside the scope of this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's easy enough to copy/paste the test w/o the package version.

pkgChan = "beta"
operator = &operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
Spec: operatorsv1alpha1.OperatorSpec{
PackageName: pkgName,
Version: pkgVer,
Channel: pkgChan,
},
}
err := cl.Create(ctx, operator)
Expect(err).NotTo(HaveOccurred())
})
It("sets resolution success status", func() {
By("running reconcile")
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
Expect(res).To(Equal(ctrl.Result{}))
Expect(err).NotTo(HaveOccurred())

By("fetching updated operator after reconcile")
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())

By("checking the expected conditions")
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
Expect(cond).NotTo(BeNil())
Expect(cond.Status).To(Equal(metav1.ConditionUnknown))
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonInstallationStatusUnknown))
Expect(cond.Message).To(ContainSubstring("waiting for BundleDeployment"))

By("fetching the bundled deployment")
bd := &rukpakv1alpha1.BundleDeployment{}
Expect(cl.Get(ctx, types.NamespacedName{Name: opKey.Name}, bd)).NotTo(HaveOccurred())
Expect(bd.Spec.ProvisionerClassName).To(Equal("core-rukpak-io-plain"))
Expect(bd.Spec.Template.Spec.ProvisionerClassName).To(Equal("core-rukpak-io-registry"))
Expect(bd.Spec.Template.Spec.Source.Type).To(Equal(rukpakv1alpha1.SourceTypeImage))
Expect(bd.Spec.Template.Spec.Source.Image).NotTo(BeNil())
Expect(bd.Spec.Template.Spec.Source.Image.Ref).To(Equal("quay.io/operatorhubio/prometheus@sha256:5b04c49d8d3eff6a338b56ec90bdf491d501fe301c9cdfb740e5bff6769a21ed"))
})
})
When("the operator specifies a package that exists within a channel but no version specified", func() {
var pkgName string
var pkgVer string
var pkgChan string
BeforeEach(func() {
By("initializing cluster state")
pkgName = "prometheus"
pkgChan = "beta"
operator = &operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
Spec: operatorsv1alpha1.OperatorSpec{
PackageName: pkgName,
Version: pkgVer,
Channel: pkgChan,
},
}
err := cl.Create(ctx, operator)
Expect(err).NotTo(HaveOccurred())
})
It("sets resolution success status", func() {
By("running reconcile")
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
Expect(res).To(Equal(ctrl.Result{}))
Expect(err).NotTo(HaveOccurred())

By("fetching updated operator after reconcile")
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())

By("checking the expected conditions")
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
Expect(cond).NotTo(BeNil())
Expect(cond.Status).To(Equal(metav1.ConditionUnknown))
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonInstallationStatusUnknown))
Expect(cond.Message).To(ContainSubstring("waiting for BundleDeployment"))
Copy link
Member

@joelanford joelanford Apr 19, 2023

Choose a reason for hiding this comment

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

For the success cases, can we also fetch the expected Bundle Deployment and check the .spec.source.image.ref?


By("fetching the bundled deployment")
bd := &rukpakv1alpha1.BundleDeployment{}
Expect(cl.Get(ctx, types.NamespacedName{Name: opKey.Name}, bd)).NotTo(HaveOccurred())
Expect(bd.Spec.ProvisionerClassName).To(Equal("core-rukpak-io-plain"))
Expect(bd.Spec.Template.Spec.ProvisionerClassName).To(Equal("core-rukpak-io-registry"))
Expect(bd.Spec.Template.Spec.Source.Type).To(Equal(rukpakv1alpha1.SourceTypeImage))
Expect(bd.Spec.Template.Spec.Source.Image).NotTo(BeNil())
Expect(bd.Spec.Template.Spec.Source.Image.Ref).To(Equal("quay.io/operatorhubio/prometheus@sha256:5b04c49d8d3eff6a338b56ec90bdf491d501fe301c9cdfb740e5bff6769a21ed"))
})
})
When("the operator specifies a package version in a channel that does not exist", func() {
var pkgName string
var pkgVer string
var pkgChan string
BeforeEach(func() {
By("initializing cluster state")
pkgName = "prometheus"
pkgVer = "0.47.0"
pkgChan = "alpha"
operator = &operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
Spec: operatorsv1alpha1.OperatorSpec{
PackageName: pkgName,
Version: pkgVer,
Channel: pkgChan,
},
}
err := cl.Create(ctx, operator)
Expect(err).NotTo(HaveOccurred())
})
It("sets resolution failure status", func() {
By("running reconcile")
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
Expect(res).To(Equal(ctrl.Result{}))
Expect(err).To(MatchError(fmt.Sprintf("package '%s' at version '%s' in channel '%s' not found", pkgName, pkgVer, pkgChan)))

By("fetching updated operator after reconcile")
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())

By("checking the expected conditions")
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
Expect(cond).NotTo(BeNil())
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionFailed))
Expect(cond.Message).To(Equal(fmt.Sprintf("package '%s' at version '%s' in channel '%s' not found", pkgName, pkgVer, pkgChan)))
})
})
When("the operator specifies a package in a channel that does not exist", func() {
var pkgName string
var pkgChan string
BeforeEach(func() {
By("initializing cluster state")
pkgName = "prometheus"
pkgChan = "alpha"
operator = &operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
Spec: operatorsv1alpha1.OperatorSpec{
PackageName: pkgName,
Channel: pkgChan,
},
}
err := cl.Create(ctx, operator)
Expect(err).NotTo(HaveOccurred())
})
It("sets resolution failure status", func() {
By("running reconcile")
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
Expect(res).To(Equal(ctrl.Result{}))
Expect(err).To(MatchError(fmt.Sprintf("package '%s' in channel '%s' not found", pkgName, pkgChan)))

By("fetching updated operator after reconcile")
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())

By("checking the expected conditions")
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
Expect(cond).NotTo(BeNil())
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionFailed))
Expect(cond.Message).To(Equal(fmt.Sprintf("package '%s' in channel '%s' not found", pkgName, pkgChan)))
})
})
When("the operator specifies a package version that does not exist in the channel", func() {
var pkgName string
var pkgVer string
var pkgChan string
BeforeEach(func() {
By("initializing cluster state")
pkgName = "prometheus"
pkgVer = "0.57.0"
pkgChan = "beta"
operator = &operatorsv1alpha1.Operator{
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
Spec: operatorsv1alpha1.OperatorSpec{
PackageName: pkgName,
Version: pkgVer,
Channel: pkgChan,
},
}
err := cl.Create(ctx, operator)
Expect(err).NotTo(HaveOccurred())
})
It("sets resolution failure status", func() {
By("running reconcile")
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
Expect(res).To(Equal(ctrl.Result{}))
Expect(err).To(MatchError(fmt.Sprintf("package '%s' at version '%s' in channel '%s' not found", pkgName, pkgVer, pkgChan)))

By("fetching updated operator after reconcile")
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())

By("checking the expected conditions")
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
Expect(cond).NotTo(BeNil())
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionFailed))
Expect(cond.Message).To(Equal(fmt.Sprintf("package '%s' at version '%s' in channel '%s' not found", pkgName, pkgVer, pkgChan)))
})
})
AfterEach(func() {
verifyInvariants(ctx, operator)

Expand Down
3 changes: 3 additions & 0 deletions internal/resolution/variable_sources/olm/olm.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,8 @@ func (o *OLMVariableSource) requiredPackageFromOperator(operator *operatorsv1alp
if operator.Spec.Version != "" {
opts = append(opts, required_package.InVersionRange(operator.Spec.Version))
}
if operator.Spec.Channel != "" {
opts = append(opts, required_package.InChannel(operator.Spec.Channel))
}
return required_package.NewRequiredPackage(operator.Spec.PackageName, opts...)
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,20 @@ func InVersionRange(versionRange string) RequiredPackageOption {
}
}

func InChannel(channelName string) RequiredPackageOption {
return func(r *RequiredPackageVariableSource) error {
if channelName != "" {
r.channelName = channelName
r.predicates = append(r.predicates, predicates.InChannel(channelName))
}
return nil
}
}

type RequiredPackageVariableSource struct {
packageName string
versionRange string
channelName string
predicates []input.Predicate
}

Expand Down Expand Up @@ -99,8 +110,14 @@ func (r *RequiredPackageVariableSource) notFoundError() error {
// context: we originally wanted to support version ranges and take the highest version that satisfies the range
// during the upstream call on the 2023-04-11 we decided to pin the version instead. But, we'll keep version range
// support under the covers in case we decide to pivot back.
if r.versionRange != "" && r.channelName != "" {
return fmt.Errorf("package '%s' at version '%s' in channel '%s' not found", r.packageName, r.versionRange, r.channelName)
}
if r.versionRange != "" {
return fmt.Errorf("package '%s' at version '%s' not found", r.packageName, r.versionRange)
}
if r.channelName != "" {
return fmt.Errorf("package '%s' in channel '%s' not found", r.packageName, r.channelName)
}
return fmt.Errorf("package '%s' not found", r.packageName)
}