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

chore: Update docs to reflect changes in eventhub after the SDK migration #1313

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JorTurFer
Copy link
Member

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)

Fixes #

…tion

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer JorTurFer requested a review from a team as a code owner February 13, 2024 21:07
Copy link

netlify bot commented Feb 13, 2024

Deploy Preview for keda ready!

Name Link
🔨 Latest commit 070b86c
🔍 Latest deploy log https://app.netlify.com/sites/keda/deploys/660f0be41b085f0008059442
😎 Deploy Preview https://deploy-preview-1313--keda.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

  • Add your contribution to all applicable KEDA versions
  • GitHub checks are passing
  • Is the DCO check failing? Here is how you can fix DCO issues

Learn more about:

Comment on lines -29 to -32
# Required when cloud = Private.
activeDirectoryEndpoint: https://login.airgap.example/
# Required when cloud = Private.
eventHubResourceURL: https://eventhubs.airgap.example/
Copy link
Member

Choose a reason for hiding this comment

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

So how do they configure this then?

Copy link
Member Author

@JorTurFer JorTurFer Feb 14, 2024

Choose a reason for hiding this comment

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

New SDK gets the ENV var AZURE_AUTHORITY_HOST in the configuration chain (although we can provide it explicitly) for activeDirectoryEndpoint and that var is provided by workload identity webhook during the mutating process. Maybe I'm wrong at any point, but that's what I've understood reading the code

For the eventHubResourceURL there are 2 ways:

  • It's explicitly provided by connection string
  • it's generated by eventHubNamespace + endpointSuffix

Copy link
Member

Choose a reason for hiding this comment

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

Should we document it that way then?

Copy link
Member Author

Choose a reason for hiding this comment

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

IDK, I'd say that it's something internal, I mean, you have to set them if you are using private cloud with msi, so I think that it's implicit

Copy link
Member

Choose a reason for hiding this comment

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

Do they have it documented? If so, let's mention it and point to their docs.

Our end-users don't know our underlying tech (and should not) but should at least know where to configure it

Copy link
Member Author

Choose a reason for hiding this comment

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

This picture if from their docs:
image

Is it enough?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Where would you like to include the reference? I don't get where it can match, as it's part of pod identity itself 🤷

@@ -58,8 +54,6 @@ triggers:
- `cloud` - Name of the cloud environment that the Event Hub belongs to. (Values: `AzurePublicCloud`, `AzureUSGovernmentCloud`, `AzureChinaCloud`, `AzureGermanCloud`, `Private`, Default: `AzurePublicCloud`, Optional)
- `endpointSuffix` - Service Bus endpoint suffix of the cloud environment. (Required when `cloud` is set to `Private`, e.g. `servicebus.cloudapi.de` for `AzureGermanCloud`).
- `storageEndpointSuffix` - Blob Storage endpoint of the cloud environment. (Required when `cloud` is set to `Private`, e.g. `airgap.example`. Do not include the `blob` part of the endpoint.)
- `activeDirectoryEndpoint` - Active Directory endpoint of the cloud environment. (Required when `cloud` is set to `Private`, e.g. `https://login.microsoftonline.de/` for `AzureGermanCloud`).
- `eventHubResourceURL` - Event Hub resource URL of the cloud environment. (Required when `cloud` is set to `Private`, e.g. `https://eventhubs.azure.net/` for known Azure Clouds).

> 💡 Learn more about the checkpointing behaviour in this [section](#checkpointing-behaviour).
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a pointer here to look at the workload identity trigger auth docs maybe?

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
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.

None yet

2 participants