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.

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

Jump to bottom

adding logic to deployablebyolm to validate packagename #1167

Merged
merged 1 commit into from
May 20, 2024
Merged

adding logic to deployablebyolm to validate packagename #1167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 15 additions & 3 deletions internal/policy/operator/deployable_by_olm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
apiruntime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/validation"
ctrl "sigs.k8s.io/controller-runtime"
crclient "sigs.k8s.io/controller-runtime/pkg/client"
)
Expand Down Expand Up @@ -214,6 +215,8 @@ func checkImageSource(ctx context.Context, operatorImages []string) bool {
}

func (p *DeployableByOlmCheck) operatorMetadata(ctx context.Context, bundleRef image.ImageReference) (*operatorData, error) {
logger := logr.FromContextOrDiscard(ctx)

// retrieve the operator metadata from bundle image
annotationsFileName := filepath.Join(bundleRef.ImageFSPath, "metadata", "annotations.yaml")
annotationsFile, err := os.Open(annotationsFileName)
Expand Down Expand Up @@ -248,13 +251,22 @@ func (p *DeployableByOlmCheck) operatorMetadata(ctx context.Context, bundleRef i
installModes[val.Type] = val
}

// validating that package name complies with DNS-1035 labeling constraints
// ensuring that CatalogSources can be created/referenced in cluster and reconciles properly
// if there is an error we need to prefix the name so the install/reconciles can continue
appName := packageName
if msgs := validation.IsDNS1035Label(packageName); len(msgs) != 0 {
logger.V(log.DBG).Info(fmt.Sprintf("package name %s does not comply with DNS-1035, prefixing to avoid errors", packageName))
appName = "p-" + packageName
Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be re-run through the DNS label validation. Prefixing the value just fixes one possible invalid scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komish Are you sure? In my testing, and our tests, this fixes both issues that are thrown. Also, if we re-validate, and it fails, then what do we do?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the package is called 1app_package

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this is non-blocking. You're addressing a specific failure that exists, and what I'm asking is a potential (but non-existent) future state. We already have an example in the wild that needs this workaround, but we haven't had an example of what I'm asking you to do yet, and it's unlikely we would given certification would fail if the CatalogSource doesn't come up.

LGTM given

Copy link
Contributor Author

@acornett21 acornett21 May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think if the above is the case, we'd want this to fail, since that name probably can't be used in other processes. Right now, we really are just trying to solve for the one case for 3scale.

}

return &operatorData{
CatalogImage: catalogImage,
Channel: channel,
PackageName: packageName,
App: packageName,
InstallNamespace: packageName,
TargetNamespace: packageName + "-target",
App: appName,
InstallNamespace: appName,
TargetNamespace: appName + "-target",
InstallModes: installModes,
}, nil
}
Expand Down
6 changes: 3 additions & 3 deletions internal/policy/operator/deployable_by_olm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ var _ = Describe("DeployableByOLMCheck", func() {

// changing the namespace since OwnNamespace operators CSV get applied to `InstallNamespace`
ownCSV := csv.DeepCopy()
ownCSV.Namespace = "testPackage"
ownCSV.Namespace = "p-testPackage"

deployableByOLMCheck.client = clientBuilder.
WithObjects(ownCSV).
Expand Down Expand Up @@ -108,8 +108,8 @@ var _ = Describe("DeployableByOLMCheck", func() {
BeforeEach(func() {
badSub := sub
Expect(deployableByOLMCheck.client.Get(testcontext, crclient.ObjectKey{
Name: "testPackage",
Namespace: "testPackage",
Name: "p-testPackage",
Namespace: "p-testPackage",
}, &badSub)).To(Succeed())
badSub.Status.InstalledCSV = ""
Expect(deployableByOLMCheck.client.Update(testcontext, &badSub, &crclient.UpdateOptions{})).To(Succeed())
Expand Down
10 changes: 5 additions & 5 deletions internal/policy/operator/operator_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ var pods = corev1.PodList{
var csv = operatorsv1alpha1.ClusterServiceVersion{
ObjectMeta: metav1.ObjectMeta{
Name: "csv-v0.0.0",
Namespace: "testPackage-target",
Namespace: "p-testPackage-target",
},
Spec: operatorsv1alpha1.ClusterServiceVersionSpec{},
Status: operatorsv1alpha1.ClusterServiceVersionStatus{
Expand Down Expand Up @@ -152,8 +152,8 @@ var secret = corev1.Secret{

var sub = operatorsv1alpha1.Subscription{
ObjectMeta: metav1.ObjectMeta{
Name: "testPackage",
Namespace: "testPackage",
Name: "p-testPackage",
Namespace: "p-testPackage",
},
Status: operatorsv1alpha1.SubscriptionStatus{
InstalledCSV: "csv-v0.0.0",
Expand All @@ -162,8 +162,8 @@ var sub = operatorsv1alpha1.Subscription{

var og = operatorsv1.OperatorGroup{
ObjectMeta: metav1.ObjectMeta{
Name: "testPackage",
Namespace: "testPackage",
Name: "p-testPackage",
Namespace: "p-testPackage",
},
Status: operatorsv1.OperatorGroupStatus{
LastUpdated: nil,
Expand Down