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 PodIdentityProvider config in azure pipeline trigger #4867

Merged
merged 7 commits into from
Jan 16, 2024

Conversation

toniiiik
Copy link
Contributor

@toniiiik toniiiik commented Aug 8, 2023

Support of PodIdentityProvider for azure pipelines scaler.

Checklist

(https://github.com/kedacore/governance/blob/main/SCALERS.md)

Fixes #

Relates to #

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

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

🏖️ Over the summer, the response time will be longer than usual due to maintainers taking time off so please bear with us.

While you are waiting, make sure to:

Learn more about:

@toniiiik toniiiik changed the title Support PodIdentity Provider config in azure pipeline trigger ADD support for PodIdentityProvider config in azure pipeline trigger Aug 8, 2023
@toniiiik toniiiik marked this pull request as ready for review August 10, 2023 13:14
@toniiiik toniiiik requested a review from a team as a code owner August 10, 2023 13:14
@JorTurFer
Copy link
Member

Hi @toniiiik,
Thanks for this awesome feature! ❤️
We are adding all the required stuff in background for the e2e tests because our DevOps organization is currently attached to other tenant different from the infrastructure tenant. I hope to fix this soon but I wanted to share it with you

@toniiiik
Copy link
Contributor Author

toniiiik commented Sep 1, 2023

Hi @toniiiik,
Thanks for this awesome feature! ❤️
We are adding all the required stuff in background for the e2e tests because our DevOps organization is currently attached to other tenant different from the infrastructure tenant. I hope to fix this soon but I wanted to share it with you

Hi @JorTurFer thank you sharing this info. Do you have some estimation when it will be ready?

@JorTurFer
Copy link
Member

I plan to prepare all the pending stuff this week, sorry for the terrible huge delay, we have had faced with some blockers in the process but I think that we are ready to it

@luck2bhanu
Copy link

Hi
Just to be sure this feature enables us to workload identity or pod identity because pod identity is not supported anymore

@toniiiik
Copy link
Contributor Author

toniiiik commented Nov 1, 2023

Hi Just to be sure this feature enables us to workload identity or pod identity because pod identity is not supported anymore

hi @luck2bhanu, yes azure-workload is only one supprted pod identity provider. Please see linked doc PR.

@JonasCordsen
Copy link

This is a very nice feature, is there any ETA?

@JorTurFer
Copy link
Member

Kindly reminder @tomkerkhove : Have you linked the AAD with AzDO?

@tomkerkhove
Copy link
Member

No, would you mind dropping me an email please?

@norbinto
Copy link

norbinto commented Nov 7, 2023

It would be an awesome feature for the next release

@JorTurFer
Copy link
Member

No, would you mind dropping me an email please?

I'll send you an email

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

@tomkerkhove @JorTurFer do you have any update here please?

@ezYakaEagle442
Copy link

@tomkerkhove Hi !
Could you please share an ETA for this PR ?

@tomkerkhove
Copy link
Member

We're working on it, had to set up new ADO org and project which is in place now and @JorTurFer is actively working on the rest of it

@JorTurFer
Copy link
Member

I'm facing some issues adding the identity to the AzDO directly, I'll try using an AAD group tomorrow

@JorTurFer
Copy link
Member

JorTurFer commented Jan 2, 2024

/run-e2e azure_pipelines_aad_wi
Update: You can check the progress here

@JorTurFer
Copy link
Member

It seems that the token isn't working properly:

2024-01-03T00:07:40Z	ERROR	scale_handler	error resolving auth params	{"type": "ScaledObject", "namespace": "azure-pipelines-test-ns", "name": "azure-pipelines-test-so", "scalerIndex": 0, "error": "error parsing azure Pipelines metadata: agent pool with id `11` not found: the Azure DevOps REST API returned error. urlString: https://dev.azure.com/kedaoss/_apis/distributedtask/pools?poolID=11 status: 401 response: {\"$id\":\"1\",\"innerException\":null,\"message\":\"TF401444: Please sign-in at least once as e0372f7f-a362-47fb-9631-74a5c4ba8bbf\\\\e0372f7f-a362-47fb-9631-74a5c4ba8bbf\\\\48a9fa1b-211e-436f-ae08-0c9651555054 in a web browser to enable access to the service.\",\"typeName\":\"Microsoft.TeamFoundation.Framework.Server.UnauthorizedRequestException, Microsoft.TeamFoundation.Framework.Server\",\"typeKey\":\"UnauthorizedRequestException\",\"errorCode\":0,\"eventId\":3000}"}
github.com/kedacore/keda/v2/pkg/scaling.(*scaleHandler).buildScalers
	/workspace/pkg/scaling/scalers_builder.go:83
github.com/kedacore/keda/v2/pkg/scaling.(*scaleHandler).performGetScalersCache
	/workspace/pkg/scaling/scale_handler.go:355
github.com/kedacore/keda/v2/pkg/scaling.(*scaleHandler).GetScalersCache
	/workspace/pkg/scaling/scale_handler.go:280
github.com/kedacore/keda/v2/controllers/keda.(*ScaledObjectReconciler).getScaledObjectMetricSpecs
	/workspace/controllers/keda/hpa.go:211
github.com/kedacore/keda/v2/controllers/keda.(*ScaledObjectReconciler).newHPAForScaledObject
	/workspace/controllers/keda/hpa.go:77
github.com/kedacore/keda/v2/controllers/keda.(*ScaledObjectReconciler).createAndDeployNewHPA
	/workspace/controllers/keda/hpa.go:50
github.com/kedacore/keda/v2/controllers/keda.(*ScaledObjectReconciler).ensureHPAForScaledObjectExists
	/workspace/controllers/keda/scaledobject_controller.go:442
github.com/kedacore/keda/v2/controllers/keda.(*ScaledObjectReconciler).reconcileScaledObject
	/workspace/controllers/keda/scaledobject_controller.go:271
github.com/kedacore/keda/v2/controllers/keda.(*ScaledObjectReconciler).Reconcile
	/workspace/controllers/keda/scaledobject_controller.go:182
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile
	/workspace/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:119
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
	/workspace/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:316
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
	/workspace/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:266
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
	/workspace/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:227

@JorTurFer
Copy link
Member

In theory, the managed identity is admin of the project (it's memeber of Project Collection Administrators)

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Accept the suggestions please, they update the info to use new resources

@JorTurFer
Copy link
Member

JorTurFer commented Jan 3, 2024

/run-e2e azure_pipelines_aad_wi
Update: You can check the progress here

@toniiiik
Copy link
Contributor Author

toniiiik commented Jan 9, 2024

/run-e2e azure_pipelines_aad_wi Update: You can check the progress here

it looks like tests are failing.

I faced same issue when managed identity had no permission to the pool on organisation scope. In fact that requests to obtain ID from name is

 https://dev.azure.com/{organization}/_apis/distributedtask/pools?poolName={poolName}

and there is no project name included in url. When I compared agent pool permissions on organisation level and project level the permissions were different. After configuring permissions for managed identity on organisation level e2e tests worked without error.
I am not sure if this is same case. If there is something I should update in tests let me know..... I still need to fix DCO

@JorTurFer
Copy link
Member

I've explicitly set the group where the MSI is as pool admin, lets see if it works 🤞
image

@JorTurFer
Copy link
Member

JorTurFer commented Jan 11, 2024

/run-e2e azure_pipelines_aad_wi

Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented Jan 11, 2024

/run-e2e azure_pipelines_aad_wi
Update: You can check the progress here

@JorTurFer
Copy link
Member

What permissions does the msi need? Could you guide me to assign it?

@toniiiik
Copy link
Contributor Author

toniiiik commented Jan 12, 2024

What permissions does the msi need? Could you guide me to assign it?

hmm it looks good I've assign same admin permission for agentpool. I've send you invitation to my ADO org you can check config here https://dev.azure.com/wedocloudsolutions/_settings/agentpools?poolId=12&view=security. There is also tf for AKS deployment https://dev.azure.com/wedocloudsolutions/_git/Keda

@JorTurFer
Copy link
Member

I'v not received any invitation :(
nvm, we can talk about this by Slack (probably it'll be faster). I'm available Kubernetes and CNCF Slack as Jorge Turrado, are you there?

@JorTurFer
Copy link
Member

JorTurFer commented Jan 12, 2024

/run-e2e azure_pipelines_aad_wi
Update: You can check the progress here

@JorTurFer
Copy link
Member

It has passed! 🥳
Could you fix DCO?

Signed-off-by: anton.lysina <alysina@gmail.com>
Signed-off-by: anton.lysina <alysina@gmail.com>
@zroubalik
Copy link
Member

zroubalik commented Jan 15, 2024

/run-e2e azure_pipelines_aad_wi
Update: You can check the progress here

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/scalers/azure_pipelines_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/azure_pipelines_scaler.go Outdated Show resolved Hide resolved
toniiiik and others added 2 commits January 15, 2024 18:08
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: toniiiik <32296236+toniiiik@users.noreply.github.com>
Signed-off-by: anton.lysina <alysina@gmail.com>
@JorTurFer
Copy link
Member

JorTurFer commented Jan 15, 2024

/run-e2e azure_pipelines_aad_wi
Update: You can check the progress here

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Have we lost the token refreshing process? Checking the PR it seems that we don't cache the token anymore and it was a nice feature because otherwise we will request 4-6 tokens per minute

pkg/scalers/azure_pipelines_scaler.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
toniiiik and others added 3 commits January 15, 2024 23:42
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: toniiiik <32296236+toniiiik@users.noreply.github.com>
Signed-off-by: anton.lysina <alysina@gmail.com>
Signed-off-by: anton.lysina <alysina@gmail.com>
@toniiiik
Copy link
Contributor Author

Have we lost the token refreshing process? Checking the PR it seems that we don't cache the token anymore and it was a nice feature because otherwise we will request 4-6 tokens per minute

token cache added. Last time I implemented cache for credentials but not for token (at this moment we lost "token refreshing"). Now both credentials and token are reused.

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

LGTM!

@JorTurFer
Copy link
Member

JorTurFer commented Jan 16, 2024

/run-e2e azure_pipelines_aad_wi
Update: You can check the progress here

@tomkerkhove tomkerkhove merged commit 6d66962 into kedacore:main Jan 16, 2024
19 checks passed
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

8 participants