Skip to content

Add some props to the log4j kafka appender to control security configs#6216

Closed
rodesai wants to merge 3 commits intoapache:trunkfrom
rodesai:add-kafka-appender-security-props
Closed

Add some props to the log4j kafka appender to control security configs#6216
rodesai wants to merge 3 commits intoapache:trunkfrom
rodesai:add-kafka-appender-security-props

Conversation

@rodesai
Copy link
Copy Markdown
Contributor

@rodesai rodesai commented Feb 1, 2019

This patch adds 2 props to the log4j kafka appender that get put directly
into the sasl properties passed to the producer:
- ClientJaasConf: This property sets sasl.jaas.config
- SaslMechanim: This property sets sasl.mechanism

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

This patch adds 2 props to the log4j kafka appender that get put directly
into the sasl properties passed to the broker:
    - ClientJaasConf: This property sets sasl.jaas.config
    - SaslMechanim: This property sets sasl.mechanism
@dongjinleekr
Copy link
Copy Markdown
Contributor

Hi Rohan.

To link your PR to the Jira issue, you should make the PR title to like: 'KAFKA-7896: Add some Log4J Kafka Properties for Producing to Secured Brokers' Please update the PR title. For this time, I linked the PR by manual. Also, I left a message on your discussion thread about how to open a KIP and make some discussions.

Thanks again for your proposal! Let's continue on the discussion on mailing thread! 😃

System.setProperty("java.security.krb5.conf", kerb5ConfPath);
}
}
if (saslMechanism != null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks like we need to consolidate with existing above code. What if user wants to set kerb5ConfPath and clientJaasConf?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Then the user could set both. The block above isn't touching the sasl.jaas.config property. It lets you configure kerberos through system properties. I'm not sure why its so defensive but that seems orthogonal to this change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clientJaasConf can be used to configure Kerberos. In that case, users may want to configure kerb5ConfPath. We may want to pull the below code to outside the if block..

            if (kerb5ConfPath != null) {
                System.setProperty("java.security.krb5.conf", kerb5ConfPath);
            }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok done

Copy link
Copy Markdown
Contributor

@omkreddy omkreddy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. LGTM.

@omkreddy
Copy link
Copy Markdown
Contributor

omkreddy commented Feb 12, 2019

Test failures are not related. merging to trunk.

@omkreddy omkreddy closed this in 08036fa Feb 12, 2019
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…fka appender

This patch adds 2 props to the log4j kafka appender that get put directly
into the sasl properties passed to the producer:
    - ClientJaasConf: This property sets sasl.jaas.config
    - SaslMechanim: This property sets sasl.mechanism

Author: Rohan Desai <desai.p.rohan@gmail.com>

Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>

Closes apache#6216 from rodesai/add-kafka-appender-security-props
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