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

Add support for Azure EventHub provider #191

Merged
merged 9 commits into from
May 6, 2021
Merged

Add support for Azure EventHub provider #191

merged 9 commits into from
May 6, 2021

Conversation

NissesSenap
Copy link
Contributor

Solves #190

Copy link

@simongottschlag simongottschlag left a comment

Choose a reason for hiding this comment

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

Left a few comments 👍😊

internal/notifier/eventhub.go Outdated Show resolved Hide resolved
internal/notifier/eventhub.go Outdated Show resolved Hide resolved
internal/notifier/eventhub.go Outdated Show resolved Hide resolved
internal/notifier/eventhub.go Outdated Show resolved Hide resolved
internal/notifier/eventhub.go Outdated Show resolved Hide resolved
internal/notifier/eventhub.go Outdated Show resolved Hide resolved
internal/notifier/eventhub.go Outdated Show resolved Hide resolved
Copy link

@simongottschlag simongottschlag left a comment

Choose a reason for hiding this comment

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

Minor tweaks

docs/spec/v1beta1/provider.md Outdated Show resolved Hide resolved
}

// Post all notification-controller messages to EventHub
func (e *AzureEventHub) Post(event events.Event) error {

Choose a reason for hiding this comment

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

Doesn't really matter, but maybe change the variable from e to a

NissesSenap and others added 5 commits May 5, 2021 11:01
Solves #190

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>
* Update go.sum
* Use ctx with timeout
* minor linting & clean-up

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>
Depending if endpointURL starts with Endpoint or not we assume that it's a JWT token
or not.
Making sure that the JWT token is up to date is NOT the notifcation-controllers work

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>
For JWT
* channel = eventhub namespace
* address = eventhub name
* token   = jwt token

For SAS
* address = connectionString, including endpoint tokens etc

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>
Signed-off-by: Edvin Norling <edvin.norling@xenit.se>
@stefanprodan
Copy link
Member

@NissesSenap please run make test before pushing, this will fix go mod, code formatting, etc.

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>
phillebaba
phillebaba previously approved these changes May 5, 2021
docs/spec/v1beta1/provider.md Outdated Show resolved Hide resolved
docs/spec/v1beta1/provider.md Outdated Show resolved Hide resolved
docs/spec/v1beta1/provider.md Outdated Show resolved Hide resolved
docs/spec/v1beta1/provider.md Show resolved Hide resolved
docs/spec/v1beta1/provider.md Show resolved Hide resolved
docs/spec/v1beta1/provider.md Outdated Show resolved Hide resolved
docs/spec/v1beta1/provider.md Outdated Show resolved Hide resolved
internal/notifier/azure_eventhub.go Outdated Show resolved Hide resolved
internal/notifier/azure_eventhub.go Show resolved Hide resolved
Edvin Norling added 2 commits May 5, 2021 16:39
* Add secret example for SAS and JWT

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>
* Change name of var eventhubNamespace to eventhubNamespace

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>
@NissesSenap NissesSenap marked this pull request as ready for review May 5, 2021 14:43
Copy link

@simongottschlag simongottschlag left a comment

Choose a reason for hiding this comment

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

nit

export AZURE_SECRET='secret-client-secret-generated-at-creation'
export AZURE_TENANT=$(az account show -o tsv --query tenantId)

curl -X GET --data 'grant_type=client_credentials' --data "client_id=$AZURE_CLIENT" --data "client_secret=$AZURE_SECRET" --data 'resource=https://eventhubs.azure.net' -H 'Content-Type: application/x-www-form-urlencoded' https://login.microsoftonline.com/$AZURE_TENANT/oauth2/token |jq .access_token

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will use your account instead of the application account. Risk of giving to much access, I would recomend to use this way.

@phillebaba phillebaba self-requested a review May 6, 2021 05:46
@phillebaba phillebaba dismissed their stale review May 6, 2021 05:46

Wrong

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>
Copy link
Member

@phillebaba phillebaba left a comment

Choose a reason for hiding this comment

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

LGTM 🌹

@phillebaba phillebaba merged commit 585fed3 into fluxcd:main May 6, 2021
@phillebaba phillebaba linked an issue May 6, 2021 that may be closed by this pull request
@NissesSenap NissesSenap deleted the azureeventhub branch May 6, 2021 08:39
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.

Azure EventHub integration
4 participants