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

Make ClusterNetworkAddressConfigProvider and co internal #1394

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

robobario
Copy link
Contributor

Type of change

  • Refactoring

Description

Closes #1381

These interfaces may become a supported pluggable interface one day, but for now we want to reduce the API surface area we need to support. They have been deprecated long enough that we can make the breaking change required as per our deprecation policy.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Review performance test results. Ensure that any degradations to performance numbers are understood and justified.
  • Make sure all Sonarcloud warnings are addressed or are justifiably ignored.
  • Update documentation
  • Reference relevant issue(s) and close them after merging
  • For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).

@robobario robobario force-pushed the move-cnacp-and-friends branch from b8ef2c3 to 3099fd0 Compare August 2, 2024 05:04
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sonar doesn't like the ipv6 regex

Make sure the regex used here, which is vulnerable to polynomial runtime due to backtracking, cannot lead to denial of service.

Maybe we should merge this with a suppress and make an issue to follow up.

Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need a follow up issue? We are sure that we want an IPv6 address and that its not exposed to arbitrary input only that from the config file so I'm happy to merge with a suppression and nothing else.

Copy link
Contributor

@k-wall k-wall left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

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

LGTM thanks @robobario

Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need a follow up issue? We are sure that we want an IPv6 address and that its not exposed to arbitrary input only that from the config file so I'm happy to merge with a suppression and nothing else.

These interfaces may become a supported pluggable interface one day, but
for now we want to reduce the API surface area we need to support. They
have been deprecated long enough that we can make the breaking change
required as per our deprecation policy.

Signed-off-by: Robert Young <[email protected]>
@robobario robobario force-pushed the move-cnacp-and-friends branch from 3099fd0 to 0dd4261 Compare August 6, 2024 02:19
Copy link

sonarqubecloud bot commented Aug 6, 2024

@robobario robobario merged commit 47ea71a into kroxylicious:main Aug 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Move ClusterNetworkAddressConfigProvider and associated deprecated classes to an internal module
3 participants