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.

Sign up for GitHub

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

Jump to bottom

fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitConfig. #1082

Merged
merged 3 commits into from Aug 22, 2023
Merged

fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitConfig. #1082

merged 3 commits into from Aug 22, 2023

Conversation

DanielaS12
Copy link
Contributor

@DanielaS12 DanielaS12 commented Aug 10, 2023 •
edited

This change removes unused deprecated class. Its instantiation caused fiat to fail when configuring SSL. Other then fixing an issue, the class was never used.

Tested on local environment on fiat 1.29.x, master and 1.30.x.

Before this change:

  • fiat was throwing FileNotFound because OkHttpClientConfigurationProperties were not decrypted
    • the properties were not decrypted and the key-store could not be downloaded into a file
  • this happened because okHttpClientConfig instantiated OkHttpClientConfigurationProperties before the SecretBeanPostProcessor was instantiated

After the change:

  • SecretBeanPostProcessor is instantiated first
  • then it is used when OkHttpClientConfigurationProperties is instantiated to decrypt each property

@DanielaS12 DanielaS12 changed the title fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitC… fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitConfig. Aug 10, 2023
@DanielaS12
Copy link
Contributor Author

@jonsie @cfieber Could you please review this changes? Thank you!

@dbyron-sf
Copy link
Contributor

What config flags made fiat fail?

@DanielaS12
Copy link
Contributor Author

DanielaS12 commented Aug 11, 2023 •
edited

@dbyron-sf there were no config changes, but simply a forgotten autowired class, that was not used. That messed up the bean instantiation order and it was not visible before.

The failure appeared after bumping up the kork version b655872, specifically this commit in kork spinnaker/kork@a68c7f5

This change is paired with multiple commits in fiat, that omitted to clean the resource from RetrofitConfig.

kkotula
kkotula approved these changes Aug 11, 2023
View reviewed changes
Copy link

@kkotula kkotula left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@dbyron-sf
Copy link
Contributor

@DanielaS12 I don't see how spinnaker/kork@a68c7f5 is relevant -- it doesn't seem to mention OkHttpClientConfiguration.

@DanielaS12
Copy link
Contributor Author

DanielaS12 commented Aug 16, 2023 •
edited

@dbyron-sf you are correct. I mentioned this commit because before the commit everything worked fine, after it the error started.

My proposed changes, only remove an unused, deprecated autowire and fix the issue mentioned above.

@DanielaS12
Copy link
Contributor Author

@dbyron-sf Can this be merged?

@ovidiupopa07
Copy link
Contributor

It looks good to me. Considering that with this PR we do some cleanup (delete unused field) that is messing with the SSL configuration.
I am going ahead and will merge this considering that all tests are passing.

@ovidiupopa07 ovidiupopa07 added the ready to merge Approved and ready for merge label Aug 22, 2023
@mergify mergify bot added the auto merged label Aug 22, 2023
@mergify mergify bot merged commit 75e3660 into spinnaker:master Aug 22, 2023
4 checks passed
@jasonmcintosh
Copy link
Member

@Mergifyio backport release-1.29.x release-1.30.x release-1.31.x

@mergify
Copy link
Contributor

mergify bot commented Aug 22, 2023 •
edited

backport release-1.29.x release-1.30.x release-1.31.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 22, 2023
@DanielaS12 @mergify
fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitC…
e54ddc0 
…onfig. (#1082)

(cherry picked from commit 75e3660)
mergify bot pushed a commit that referenced this pull request Aug 22, 2023
@DanielaS12 @mergify
fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitC…
ed3d399 
…onfig. (#1082)

(cherry picked from commit 75e3660)
This was referenced Aug 22, 2023
Merged
Merged
mergify bot pushed a commit that referenced this pull request Aug 22, 2023
@DanielaS12 @mergify
fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitC…
1f87ba2 
…onfig. (#1082)

(cherry picked from commit 75e3660)
@mergify mergify bot mentioned this pull request Aug 22, 2023
Merged
@jasonmcintosh
Copy link
Member

The kork PR broke this b/c it changed the ordering of bean loading/dependency handling :( The lazy loading IN THEORY might have fixed this, but ... that's iffy.

@jasonmcintosh
Copy link
Member

Last comment: MOST of the stuff in this class seems iffy config/usage wise. Most of the fields aren't used at this point (and most of this should be in kork anyways). It looks like the RetryingInterceptor bean generation is the main thing

mergify bot added a commit that referenced this pull request Aug 22, 2023
@mergify @DanielaS12
fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitC…
a74ce83 
…onfig. (#1082) (#1092)

(cherry picked from commit 75e3660)

Co-authored-by: DanielaS12 <111055962+DanielaS12@users.noreply.github.com>
mergify bot added a commit that referenced this pull request Aug 22, 2023
@mergify @DanielaS12
fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitC…
6f40a11 
…onfig. (#1082) (#1091)

(cherry picked from commit 75e3660)

Co-authored-by: DanielaS12 <111055962+DanielaS12@users.noreply.github.com>
mergify bot added a commit that referenced this pull request Aug 22, 2023
@mergify @DanielaS12
fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitC…
a8924d9 
…onfig. (#1082) (#1090)

(cherry picked from commit 75e3660)

Co-authored-by: DanielaS12 <111055962+DanielaS12@users.noreply.github.com>
aman-agrawal pushed a commit to OpsMx/fiat-oes that referenced this pull request May 7, 2024
@DanielaS12 @aman-agrawal
fix(ssl): Removed unused deprecated okHttpClientConfig from retrofitC…
7d8b302 
…onfig. (spinnaker#1082)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkotula kkotula kkotula approved these changes

@ovidiupopa07 ovidiupopa07 ovidiupopa07 approved these changes

@cfieber cfieber Awaiting requested review from cfieber cfieber is a code owner

@jonsie jonsie Awaiting requested review from jonsie jonsie is a code owner

Assignees
No one assigned
Labels
auto merged ready to merge Approved and ready for merge target-release/1.32
Projects
None yet
Milestone
No milestone
6 participants
@DanielaS12 @dbyron-sf @ovidiupopa07 @jasonmcintosh @kkotula @spinnakerbot