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

Fixing Delete acl functionality #984

Merged
merged 4 commits into from
Jun 9, 2023

Conversation

shubhamcoc
Copy link
Contributor

@shubhamcoc shubhamcoc commented May 25, 2023

Description

Fixing Delete ACL functionality as it was not passing correct arguments. Just passing the dn in sarama.AclFilter struct leading to take default values (i.e. Unknown) for other arguments as a result it was failing in the validation at kafka and resulting in connection close with EOF error.

private void normalizeAndValidate() {
        if (version() == 0) {
            for (DeleteAclsRequestData.DeleteAclsFilter filter : data.filters()) {
                PatternType patternType = PatternType.fromCode(filter.patternTypeFilter());

                // On older brokers, no pattern types existed except LITERAL (effectively). So even though ANY is not
                // directly supported on those brokers, we can get the same effect as ANY by setting the pattern type
                // to LITERAL. Note that the wildcard `*` is considered `LITERAL` for compatibility reasons.
                if (patternType == PatternType.ANY)
                    filter.setPatternTypeFilter(PatternType.LITERAL.code());
                else if (patternType != PatternType.LITERAL)
                    throw new UnsupportedVersionException("Version 0 does not support pattern type " +
                            patternType + " (only LITERAL and ANY are supported)");
            }
        }

        final boolean unknown = data.filters().stream().anyMatch(filter ->
                filter.patternTypeFilter() == PatternType.UNKNOWN.code()
                        || filter.resourceTypeFilter() == ResourceType.UNKNOWN.code()
                        || filter.operation() == AclOperation.UNKNOWN.code()
                        || filter.permissionType() == AclPermissionType.UNKNOWN.code()
        );

        if (unknown) {
            throw new IllegalArgumentException("Filters contain UNKNOWN elements, filters: " + data.filters());
        }
    }

linked issue #983

Type of Change

  • Bug Fix

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests

@shubhamcoc shubhamcoc requested a review from a team as a code owner May 25, 2023 10:29
@shubhamcoc
Copy link
Contributor Author

Hi @panyuenlau, @bartam1, Kindly review the patch.

Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

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

The direction of this is correct, but we are missing some details here

@shubhamcoc shubhamcoc force-pushed the user-deletion-with-acl branch from 46ac523 to 9a319bb Compare June 9, 2023 05:17
Copy link
Member

@pregnor pregnor left a comment

Choose a reason for hiding this comment

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

LGTM

@pregnor pregnor merged commit 115f380 into banzaicloud:master Jun 9, 2023
@pregnor pregnor mentioned this pull request Jun 9, 2023
1 task
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.

3 participants