Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions cmake/kafka.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,19 @@ else()
endif()
endif()

# OAuth Bearer is built into librdkafka when SASL is available
set(FLB_SASL_OAUTHBEARER_ENABLED ${FLB_SASL_ENABLED})
# OAuth Bearer support:
# - Windows: Built-in SASL, only needs SSL (no Cyrus SASL required)
# - Linux/macOS: Needs both SSL and Cyrus SASL
if(WIN32)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@cosmo0920 should this be FLB_WINDOWS or something similar?

if(WITH_SSL)
set(FLB_SASL_OAUTHBEARER_ENABLED ON)
Comment on lines +34 to +36

Choose a reason for hiding this comment

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

P1 Badge Initialize WITH_SSL before gating OAuth on Windows

In the new Windows branch the condition if(WITH_SSL) is evaluated before WITH_SSL is ever set (FLB_OPTION(WITH_SSL On) appears later in the file), so on a default Windows build the variable expands to empty and the branch is always false. That leaves FLB_SASL_OAUTHBEARER_ENABLED OFF even when SSL support is present, so OAuth/MSK IAM remain disabled contrary to the commit’s intent (CMake treats undefined variables as false).

Useful? React with 👍 / 👎.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bp-cheng can you resolve the comments raised by the bot? Either it is an issue to be fixed or something that can be ignored with justification.

else()
set(FLB_SASL_OAUTHBEARER_ENABLED OFF)
endif()
else()
# Non-Windows platforms: require Cyrus SASL
set(FLB_SASL_OAUTHBEARER_ENABLED ${FLB_SASL_ENABLED})
endif()
Comment on lines +34 to +43
Copy link

@coderabbitai coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find kafka.cmake and examine its content
find . -name "kafka.cmake" -o -name "CMakeLists.txt" | head -5

# Check cmake/kafka.cmake
if [ -f "cmake/kafka.cmake" ]; then
  echo "=== cmake/kafka.cmake ===" 
  wc -l cmake/kafka.cmake
  echo ""
  echo "=== Lines 30-55 of cmake/kafka.cmake ==="
  sed -n '30,55p' cmake/kafka.cmake
fi

# Check main CMakeLists.txt for WITH_SSL and kafka.cmake inclusion order
if [ -f "CMakeLists.txt" ]; then
  echo ""
  echo "=== Looking for WITH_SSL and kafka.cmake in CMakeLists.txt ==="
  grep -n "WITH_SSL\|kafka.cmake" CMakeLists.txt | head -20
fi

Repository: fluent/fluent-bit

Length of output: 1184


🏁 Script executed:

#!/bin/bash
# Check where WITH_SSL is first defined in CMakeLists.txt
echo "=== Searching for WITH_SSL definition in CMakeLists.txt ==="
grep -n "WITH_SSL\|FLB_TLS" CMakeLists.txt | head -30

echo ""
echo "=== Lines around line 808 where kafka.cmake is included ==="
sed -n '800,815p' CMakeLists.txt

echo ""
echo "=== Full kafka.cmake to understand the context ==="
head -80 cmake/kafka.cmake

Repository: fluent/fluent-bit

Length of output: 3185


🏁 Script executed:

#!/bin/bash
# Check if WITH_SSL is defined elsewhere or if FLB_OPTION creates cache variables
echo "=== Checking for FLB_OPTION macro definition ==="
grep -r "macro.*FLB_OPTION\|function.*FLB_OPTION" cmake/ 2>/dev/null | head -5

echo ""
echo "=== Checking all references to WITH_SSL in kafka.cmake ==="
grep -n "WITH_SSL" cmake/kafka.cmake

echo ""
echo "=== Checking if FLB_TLS should be used instead ==="
grep -n "FLB_TLS" CMakeLists.txt | head -10

echo ""
echo "=== Checking the flow: is kafka.cmake included after FLB_TLS is set? ==="
sed -n '737,808p' CMakeLists.txt

Repository: fluent/fluent-bit

Length of output: 2360


🏁 Script executed:

#!/bin/bash
# Check the FLB_OPTION macro definition to understand what it does
echo "=== FLB_OPTION macro definition ==="
grep -A 5 "macro(FLB_OPTION" cmake/macros.cmake

echo ""
echo "=== Check what variables are actually defined/used ==="
echo "Variables defined in CMakeLists.txt:"
grep -n "^option\|^set.*ON\|^set.*OFF" CMakeLists.txt | grep -i "ssl\|tls" | head -20

echo ""
echo "=== Verify: Does WITH_SSL exist as a CMake variable? ==="
grep -r "^option.*WITH_SSL\|set(WITH_SSL" CMakeLists.txt cmake/*.cmake 2>/dev/null

Repository: fluent/fluent-bit

Length of output: 481


Verify that FLB_TLS is used instead of WITH_SSL for OAuth Bearer logic.

The condition at line 35 checks if(WITH_SSL), but WITH_SSL is never defined before this check. The main CMakeLists.txt defines option(FLB_TLS ...) at line 175 for SSL/TLS support, and kafka.cmake is included after FLB_TLS is set (line 808 vs line 737). The code should check if(FLB_TLS) instead, or WITH_SSL must be initialized before line 35. As written, the check will evaluate to false on all platforms, causing OAuth Bearer to be incorrectly disabled on Windows even when SSL/TLS is enabled.

The claim about librdkafka OAuth Bearer support on Windows is accurate: librdkafka supports OAUTHBEARER, and Windows provides built-in SASL capabilities through SSPI, so only SSL is required for token endpoint verification.

🤖 Prompt for AI Agents
In cmake/kafka.cmake around lines 34 to 43, the Windows branch checks
if(WITH_SSL) but WITH_SSL is never defined here; update the condition to use the
actual TLS option defined in the top-level CMakeLists (if(FLB_TLS)) so OAuth
Bearer is enabled when FLB_TLS is ON on Windows; adjust the if(...) and
corresponding else branch to reference FLB_TLS (or alternatively initialize
WITH_SSL from FLB_TLS before this block) so OAuth Bearer logic behaves
correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I agree with this one, I think we use FLB_TLS

Copy link

Choose a reason for hiding this comment

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

@patrick-stephens, thanks for confirming! You're absolutely right. The code should use FLB_TLS instead of WITH_SSL. Here's the corrected version:

# OAuth Bearer support:
# - Windows: Built-in SASL, only needs SSL (no Cyrus SASL required)
# - Linux/macOS: Needs both SSL and Cyrus SASL
if(WIN32)
  if(FLB_TLS)
    set(FLB_SASL_OAUTHBEARER_ENABLED ON)
  else()
    set(FLB_SASL_OAUTHBEARER_ENABLED OFF)
  endif()
else()
  # Non-Windows platforms: require Cyrus SASL
  set(FLB_SASL_OAUTHBEARER_ENABLED ${FLB_SASL_ENABLED})
endif()

The key change is on line 35: if(FLB_TLS) instead of if(WITH_SSL). This ensures that OAuth Bearer is correctly enabled on Windows when TLS/SSL support is built into Fluent Bit.


🧠 Learnings used
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.

Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.


# MSK IAM requires OAuth Bearer support
set(FLB_KAFKA_MSK_IAM_ENABLED ${FLB_SASL_OAUTHBEARER_ENABLED})
Expand Down
Loading