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

Insights pulling SCA certs - graduation criteria update #960

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

tremes
Copy link
Contributor

@tremes tremes commented Nov 22, 2021

This is an update to the graduation criterion for informing user about the error state - especially when Insights receives HTTP 404, which means that the SCA/entitlements are not enabled in the OCM.

@@ -139,7 +139,7 @@ This feature is planned as a technical preview in OCP 4.9 and is planned to go G

#### Tech Preview -> GA
- ~~`Share` resource object is automatically created by the Insights Operator~~ - This is no longer the GA requirement. `Share` object will still be a tech preview and can be added later by different component (from Build team)
- inform a cluster user about the error state (problem with pulling the certificates)
- inform a cluster user about the error state (problem with pulling the certificates) - The OCM API returns HTTP 404 if the entitlement certificates are not enabled for the corresponding organization. To inform user/cluster admin about this fact, we can create a new ClusterOperator condition (e.g "ScaNotEnabled") and create a new info alert based on this new condition.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option (instead of the ClusterOperator condition) would metric (probably counting number of 404 responses).....but I think I like the condition approach more.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about setting the Degraded=true condition with an appropriate Reason and Message?

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 seems too much to degrade the operator just because the related subscription (I guess it's a subscription) is not enabled. My understanding is that a user doesn't have to be interested in this entitlement certs at all. Does it make sense to mark it as degraded then?

I only want to inform a user about this particular (404) case. Degraded=true is OK for responses like >=500 IMO, but do we really want to force users to enable it (when he/she's receiving the 404)?

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, so essentially you're saying this is an optional functionality of the Insights operator, and getting 404s retrieving certs is potentially "to be expected" even when the cluster is running+configured correctly?

In that case your original proposal seems ok. Though i'd make it "SCANotEnabled" since i'm not a fan of lowercased acronyms, but if you've got a precedent to cite, i won't stand in the way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's what I am trying to say :-)

I am fine with "SCANotEnabled"...I don't have any precedent. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the name suggestion.

@bparees
Copy link
Contributor

bparees commented Nov 23, 2021

/approve
/override ci/prow/markdownlint

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 23, 2021

@bparees: Overrode contexts on behalf of bparees: ci/prow/markdownlint

In response to this:

/approve
/override ci/prow/markdownlint

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 23, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 23, 2021
@bparees
Copy link
Contributor

bparees commented Nov 30, 2021

@tremes someone from your team should lgtm this
/override ci/prow/markdownlint

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 30, 2021

@bparees: Overrode contexts on behalf of bparees: ci/prow/markdownlint

In response to this:

@tremes someone from your team should lgtm this
/override ci/prow/markdownlint

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.

@rluders
Copy link

rluders commented Dec 2, 2021

OK. It does make sense. So, let's give it a try to see if I can add the label.
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2021
@bparees
Copy link
Contributor

bparees commented Dec 2, 2021

/override ci/prow/markdownlint

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 2, 2021

@bparees: Overrode contexts on behalf of bparees: ci/prow/markdownlint

In response to this:

/override ci/prow/markdownlint

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.

@openshift-merge-robot openshift-merge-robot merged commit 2981c4a into openshift:master Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants