Skip to content

fix: misleading MissingCRDException exception #553

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

Closed
wants to merge 1 commit into from

Conversation

lburgazzoli
Copy link
Collaborator

@lburgazzoli lburgazzoli commented Sep 22, 2021

Fixes #552

if (404 == ke.getCode()) {
throw new MissingCRDException(null, null);
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@csviri @metacosm removed this because a 404 may also be the result of watching a resource by name or in a namespace that does not exists so assuming 404 is only when a CRD is missing seems wrong

@metacosm
Copy link
Collaborator

We need to find a better way to check whether the 404 occurs because of a missing CRD or for some other reason. The MissingCRDException mechanism is central to how the Quarkus automatically applies missing CRD so we need that exception to be thrown there in this situation.

@lburgazzoli
Copy link
Collaborator Author

lburgazzoli commented Sep 22, 2021

The check for the presence of the controller CRD is performed before (https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java#L165-L175) so I guess this is what Quarkus leverages.

The issue I'm facing is when an event source is registered so yes it may be that a CRD is missing but could be an external CRD so Quarkus won't be able to apply it (I guess).

In my specific case as example the missing CRD was a Strimzi one (because I set up an event source for those types) but the error reported that the CRD for my controller was missing, which was not true.

@metacosm
Copy link
Collaborator

The check for the presence of the controller CRD is performed before (https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java#L165-L175) so I guess this is what Quarkus leverages.

Unfortunately, this isn't enough, which is why I added that code in the first place. However, I agree that it's subpar… I'm working on an hopefully better solution.

@lburgazzoli
Copy link
Collaborator Author

The check for the presence of the controller CRD is performed before (https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java#L165-L175) so I guess this is what Quarkus leverages.

Unfortunately, this isn't enough, which is why I added that code in the first place. However, I agree that it's subpar… I'm working on an hopefully better solution.

Wonder if it would be simpler if Quarkus should just apply the CR if it is configured to do so without checking, what would be the downside ?

@metacosm
Copy link
Collaborator

The check for the presence of the controller CRD is performed before (https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java#L165-L175) so I guess this is what Quarkus leverages.

Unfortunately, this isn't enough, which is why I added that code in the first place. However, I agree that it's subpar… I'm working on an hopefully better solution.

Wonder if it would be simpler if Quarkus should just apply the CR if it is configured to do so without checking, what would be the downside ?

That's a good point and I actually think that's the direction things are going now… The exception was introduced when the extension didn't have the ability to re-apply the CRD on changes but it now has that ability so we might indeed simplify things here… I will have to check. I've been working on improving the exceptions in the fabric8 client as well so will first finish on that path to validate the approach.
That said, the MissingCRDException should be interesting for more than just the Quarkus extension as a diagnostic tool for users as well (because that's quite a common occurrence for beginners with the SDK to forget to apply the CRD to the cluster and, previously, you used to get a cryptic 404 error without any details).

@lburgazzoli
Copy link
Collaborator Author

The check for the presence of the controller CRD is performed before (https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java#L165-L175) so I guess this is what Quarkus leverages.

Unfortunately, this isn't enough, which is why I added that code in the first place. However, I agree that it's subpar… I'm working on an hopefully better solution.

Wonder if it would be simpler if Quarkus should just apply the CR if it is configured to do so without checking, what would be the downside ?

I've been working on improving the exceptions in the fabric8 client as well so will first finish on that path to validate the approach.

+1

That said, the MissingCRDException should be interesting for more than just the Quarkus extension as a diagnostic tool for users as well (because that's quite a common occurrence for beginners with the SDK to forget to apply the CRD to the cluster and, previously, you used to get a cryptic 404 error without any details).

Agree, better exception from fabric8 client would be very useful.

@metacosm
Copy link
Collaborator

See #558

@lburgazzoli
Copy link
Collaborator Author

I guess I can close this one then ?

@metacosm
Copy link
Collaborator

I guess I can close this one then ?

I think so, yes. Just let me know if the fix in #558 works for you. :)

@lburgazzoli
Copy link
Collaborator Author

LGTM

@metacosm
Copy link
Collaborator

Superseded by #558. Thanks for bringing this up!

@metacosm metacosm closed this Sep 23, 2021
@metacosm metacosm deleted the gh-552 branch September 23, 2021 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misleading MissingCRDException exception
2 participants