Handles incorrect CatalogSource interval values.

[DhritiShikhar]

Description of the change:

Handles incorrect CatalogSource interval values.

Example:

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: ibm-operator-catalog
  namespace: openshift-marketplace
spec:
  displayName: ibm-operator-catalog 
  publisher: IBM Content
  sourceType: grpc
  image: docker.io/ibmcom/ibm-operator-catalog
  updateStrategy:
    registryPoll:
      interval: 45mError code    ################### incorrect value

Note:
Made changes to the UpdateStrategy unmarshal inside the vendor folder for reviews. Actual PR is here -> operator-framework/api#142

will unhold once this and the api repo PR is lgtmed and changes from api is vendored in here.

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Docs updated or added to /doc
• Commit messages sensible and descriptive

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: DhritiShikhar
To complete the pull request process, please assign awgreene after the PR has been reviewed.
You can assign the PR to them by writing /assign @awgreene in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

Hi @DhritiShikhar. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[DhritiShikhar]

/hold

Made changes to the UpdateStrategy unmarshal inside the vendor folder for reviews. Actual PR is here -> operator-framework/api#142

will unhold once this and the api repo PR is lgtmed and changes from api is vendored in here.

[anik120]

Thanks for the PR @DhritiShikhar 🎉

An e2e here as proof of concept would be nice too.

Also, what you think of the following design:

In catalogsource_types.go we change the RegistryPoll struct to include a new field

type RegistryPoll struct {
	ParsingError error                                                               // new field
	Interval *metav1.Duration `json:"interval,omitempty"`        // existing field
}

so that we can set that field if there is any error while unmarshalling:

func (u *UpdateStrategy) UnmarshalJSON(b []byte) error {

	var data map[string]map[string]string
	err := json.Unmarshal(b, &data)

	registryPoll := &RegistryPoll{}
	pd, err := time.ParseDuration(data["registryPoll"]["interval"])
	if err != nil {
		registryPoll.ParsingError = fmt.Errorf("error parsing spec.updateStrategy.registryPoll.interval. Setting the default value of %v. Error:%v", DefaultRegistryPollDuration, err)
		defaultTime, _ := time.ParseDuration(DefaultRegistryPollDuration)
		registryPoll.Interval = &metav1.Duration{defaultTime}
	} else {
		registryPoll.ParsingError = nil
		registryPoll.Interval = &metav1.Duration{pd}
	}
	u.RegistryPoll = registryPoll
	return nil
}

That we can use here to 1. log the error 2. set a condition in the catsrc, so that the user is aware why the value that they put, eg 20 minutes got turned into 15m:

if out.Spec.UpdateStrategy != nil {
		if out.Spec.UpdateStrategy.RegistryPoll.ParsingError != nil {
			// set catsrc.status.conditions to let user know there was a problem, i.e explain why a default value is being used.
		}
		logger.Debugf("requeuing registry server sync based on polling interval %s", out.Spec.UpdateStrategy.Interval.Duration.String())
		resyncPeriod := reconciler.SyncRegistryUpdateInterval(out, time.Now())
		o.catsrcQueueSet.RequeueAfter(out.GetNamespace(), out.GetName(), queueinformer.ResyncWithJitter(resyncPeriod, 0.1)())
		return
	}

[anik120]

I am not sure I understand why this is needed. Aren't we already parsing the duration and setting it to a correct value while unmarshalling the catsrc object? What exactly are we changing here for the CatalogSource?

[DhritiShikhar]

We parse the duration, we set the correct value to out.Spec.UpdateStrategy.RegistryPoll.Interval but this line is needed to write it back to the original CatalogSource.

[anik120]

/ok-to-test

[DhritiShikhar]

@anik120

• Added ParsingError field and updated the CatalogSource status as per your suggestion

[dinhxuanvu]

@DhritiShikhar Did you test this locally? I have a feeling that this will not work (hopefully I'm wrong). The reason is because this invalid interval value will break the informer so it will fail before it even reaches the check code that you add.

[DhritiShikhar]

@dinhxuanvu Yes, tested this locally.

If the interval value is invalid, it's caught during the Unmarshalling and changed to default value -> operator-framework/api#142

[anik120]

PR's looking to be in a good place. The API PR is failing the CI though, so if we can solve those errors, get that merged, and then vendor those changes in here, this PR should be ready to go in.

[anik120]

/close in favor of #2447