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(trait): nil pointer dereference when applying traits during kit building #3471

Merged
merged 1 commit into from
Jul 22, 2022

Conversation

tadayosi
Copy link
Member

@tadayosi tadayosi commented Jul 21, 2022

Fix #3205

Release Note

fix(trait): nil pointer dereference when some traits are enabled at IntegrationPlatform

@tadayosi tadayosi added the kind/bug Something isn't working label Jul 21, 2022
@tadayosi tadayosi added this to the 1.10.0 milestone Jul 21, 2022
@tadayosi
Copy link
Member Author

I found that the impact of the bug is much broader than Tracing trait. In essence, every trait that is naively implemented assuming e.Integration is always present can throw the nil pointer and kill the operator when it is enabled at IntegrationPlatform...

I'm trying to create a better fix that solves the issue altogether.

@tadayosi tadayosi force-pushed the Issue-3205-tracing-nil-pointer branch from 6fad820 to ca87efc Compare July 21, 2022 07:27
@tadayosi
Copy link
Member Author

@squakez @oscerd @astefanutti

The last commit might be a bit controversial but I find it necessary for the fix. I think Camel trait has actually been influential to kits but hasn't been explicitly specified so.

I'd like to make sure I'm not doing something completely wrong, so please review this again.

@squakez
Copy link
Contributor

squakez commented Jul 21, 2022

Thanks @tadayosi . I think it partially influences the kits. Ie, if the runtime version is changed, then, it makes sense to build a new kit. However, if a property change (a runtime property, as build time property are specified in the builder trait), we probably don't want to rebuild the kit. I'm not sure how to tackle this, maybe we should split the trait?

@tadayosi tadayosi changed the title fix(trait): nil pointer dereference at tracing trait fix(trait): nil pointer dereference when applying traits during kit building Jul 21, 2022
@tadayosi
Copy link
Member Author

tadayosi commented Jul 22, 2022

However, if a property change (a runtime property, as build time property are specified in the builder trait), we probably don't want to rebuild the kit. I'm not sure how to tackle this, maybe we should split the trait?

Thanks. That's very valid point. It also looks not so easy to split the trait and at the same time keep backward compatibility. And some common E2E tests still failing may suggest there are a few more traits that would implicitly influence kits...

I think there's a bit of mess in traits about what actually influence kits and what do not and their relationship with the return value of the InfluencesKit() methods that should be sorted out. But I'm a bit intimidated to try to address it within v1. We can rather tackle it in Camel K v2.

I'll explore another approach that checks possibility of nil integration in every trait, instead of making Camel trait explicitly influencing kits and applying traits that influence kits during kit building.

@tadayosi tadayosi force-pushed the Issue-3205-tracing-nil-pointer branch from 4a16907 to 0621da9 Compare July 22, 2022 05:10
@tadayosi
Copy link
Member Author

Applied the 2nd approach of thorough nil checking.

Good news is that most of the traits are bug free, as they are already guarded with the Integration Phase checking methods which means if Integration is absent they simply return false instead of crashing.

Nonetheless I made it explicit by checking e.Integration == nil at the beginning of every trait's Configure() method so that we can easily distinguish if it's a trait targeted for Integration or not.

This issue happened when an trait was enabled at IntegrationPlatform
which is implemented not considering possible absence of Integration.

Fix apache#3205
@tadayosi tadayosi force-pushed the Issue-3205-tracing-nil-pointer branch from 0621da9 to dee83e4 Compare July 22, 2022 05:42
@tadayosi tadayosi merged commit 3cedfa0 into apache:main Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nil pointer dereference tracing trait
3 participants