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

fix: avoid NPE is given spec is null #1899

Merged
merged 4 commits into from
May 11, 2023
Merged

fix: avoid NPE is given spec is null #1899

merged 4 commits into from
May 11, 2023

Conversation

metacosm
Copy link
Collaborator

Also optimizes case where resource is a CustomResource.
Fixes #1897

Also optimizes case where resource is a CustomResource.
Fixes #1897
@metacosm metacosm self-assigned this May 11, 2023
@metacosm metacosm requested a review from csviri May 11, 2023 09:27
@csviri
Copy link
Collaborator

csviri commented May 11, 2023

I think we should add also an integration test for namespace before release. But if you want I can do that in a separate PR.

@sonarcloud
Copy link

sonarcloud bot commented May 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

73.1% 73.1% Coverage
0.0% 0.0% Duplication

@metacosm metacosm merged commit 21c8dac into main May 11, 2023
19 checks passed
@metacosm metacosm deleted the handle-namespaces branch May 11, 2023 16:02
@metacosm
Copy link
Collaborator Author

I think we should add also an integration test for namespace before release. But if you want I can do that in a separate PR.

I think the current tests cover what needs to be covered and I'm not sure what an additional integration test would bring.

@coltmcnealy-lh
Copy link
Contributor

coltmcnealy-lh commented May 11, 2023

When is the next release? Perhaps I could write a test since I was the one who reported the bug. But I wouldn't have time until this weekend.

Also, thank you for fixing this so promptly!

@metacosm
Copy link
Collaborator Author

It's been released already but a test is always welcomed…

@coltmcnealy-lh
Copy link
Contributor

 - io.littlehorse.operator.lhcluster.dependents.LHServerServiceAccountDR -> java.lang.IllegalStateException: No spec found on resource io.fabric8.kubernetes.api.model.ServiceAccount
	at io.javaoperatorsdk.operator.ReconcilerUtils.noSpecException(ReconcilerUtils.java:146)

It appears there is a similar issue for ServiceAccounts; I assume that it will extend to Role Binding and Roles as well.

@metacosm
Copy link
Collaborator Author

@coltmcnealy-lh see #1909

@coltmcnealy-lh
Copy link
Contributor

Thanks! I pulled the latest commit on main (#1909 was the last commit), built it, and used 4.3.4-SNAPSHOT, but I still get the same error. I'll look again, I must be doing something wrong.

YIKES: 2023-05-22 13:28:57 ERROR ReconciliationDispatcher - Error during event processing ExecutionScope{ resource id: ResourceID{name='insecure-example', namespace='littlehorse'}, version: 15551} failed.
io.javaoperatorsdk.operator.AggregatedOperatorException: Exception(s) during workflow execution. Details:
 - io.littlehorse.operator.lhcluster.dependents.LHServerServiceAccountDR -> java.lang.IllegalStateException: No spec found on resource io.fabric8.kubernetes.api.model.ServiceAccount
	at io.javaoperatorsdk.operator.ReconcilerUtils.noSpecException(ReconcilerUtils.java:146)
	at io.javaoperatorsdk.operator.ReconcilerUtils.getSpec(ReconcilerUtils.java:111)
	at io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher.match(GenericKubernetesResourceMatcher.java:90)
	at io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher.match(GenericKubernetesResourceMatcher.java:32)
	at io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher.match(GenericKubernetesResourceMatcher.java:14)
	at io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource.match(KubernetesDependentResource.java:144)
	at io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource.match(KubernetesDependentResource.java:33)
	at io.javaoperatorsdk.operator.processing.dependent.AbstractDependentResource.reconcile(AbstractDependentResource.java:67)
	at io.javaoperatorsdk.operator.processing.dependent.SingleDependentResourceReconciler.reconcile(SingleDependentResourceReconciler.java:19)
	at io.javaoperatorsdk.operator.processing.dependent.AbstractDependentResource.reconcile(AbstractDependentResource.java:52)
	at io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowReconcileExecutor$NodeReconcileExecutor.doRun(WorkflowReconcileExecutor.java:124)
	at io.javaoperatorsdk.operator.processing.dependent.workflow.NodeExecutor.run(NodeExecutor.java:22)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)

@csviri
Copy link
Collaborator

csviri commented May 23, 2023

@metacosm I can check and add an integration test.

@metacosm
Copy link
Collaborator Author

Ah, I didn't pay enough attention, the PR only dealt with the update part, not the matching part 🤦🏼

@csviri
Copy link
Collaborator

csviri commented May 23, 2023

Yeah, IMO best to add integration tests for these well definable features / aspects, so we check it running.

@csviri
Copy link
Collaborator

csviri commented May 23, 2023

added just IT here: #1910
since @metacosm already working on fix

@metacosm
Copy link
Collaborator Author

#1911 should fix this now, hopefully!

@coltmcnealy-lh
Copy link
Contributor

Thank you! I still see the IllegalStateException...

To verify via the test @csviri wrote, you can do

git checkout matchers
cd operator-framework
mvn test -Dtest=SpecialResourcesDependentIT#specialCRUDReconciler

Let me know if you'd like me to take a gander at the matchers branch and try to help contribute a fix.

@metacosm
Copy link
Collaborator Author

metacosm commented May 24, 2023

The #1911 PR includes the tests and CI is passing. I've also run the test locally without issue. I did initially see the issue but this turned out to be a problem with the test using an outdated snapshot version. Rebuilding the whole project solved the issue. Please let us know if you're still seeing an issue.

@coltmcnealy-lh
Copy link
Contributor

I am currently having some issues; but it's likely due to me being a Maven newbie. Thank you for your patient help.

If it turns out to not be a "me" problem, I'll push a PR with a test and a fix. I really appreciate all of your help!

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.

NullPointerException when reconciling Namespace
3 participants