Skip to content

Conversation

@hangc0276
Copy link
Contributor

This reverts commit c7eabc9.

Motivation

PR #19455 introduced break change and has been cherry-picked to release branch-2.11. It will lead to users upgrading to the new release version authenticate failed.

Modifications

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@hangc0276 hangc0276 self-assigned this Mar 15, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 15, 2023
Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM

@hangc0276
Copy link
Contributor Author

We need to revert more commits related to this PR.

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

We need to revert more commits related to this PR.

Even if the PMC decides that the right decision is to "revert this behavior change", we should not revert this whole commit. Only a few lines of code are actually under dispute.

Comment on lines -332 to -335
} else if (StringUtils.isNotBlank(originalPrincipal)
&& !(allowNonProxyPrincipalsToBeEqual && originalPrincipal.equals(authenticatedPrincipal))) {
errorMsg = "cannot specify originalPrincipal when connecting without valid proxy role.";
}
Copy link
Member

Choose a reason for hiding this comment

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

These are the lines that are under question.

@codelipenghui
Copy link
Contributor

We have an acceptable solution for now. So that we don't need to revert this change, just push a fix to the release branches.
https://lists.apache.org/thread/q1k1krfjr8njdf8t3sy1ht4vg67s4x71 the discussion thread.

And @michaeljmarshall message me in Slack, he will open PR to fix it.

Thanks @hangc0276 and @michaeljmarshall

@michaeljmarshall
Copy link
Member

#19830 should supersede this PR, so I am going to close this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants