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

feat(HMS-1674): Verify Azure source availability #750

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

ezr-ondrej
Copy link
Member

@ezr-ondrej ezr-ondrej commented Nov 10, 2023

Implement Azure availability check.
We are checking for subscription metadata.
If we are able to fetch that, we consider the source available. It's not bullet proof, but it covers basics ;)

"github.com/RHEnVision/provisioning-backend/internal/telemetry"
)

const AZURE_SERVICE_ACCOUNT_OBJECT_ID = "bf9c3e0e-4bbd-425b-adb1-f005d3dfe9f9"
Copy link
Member Author

Choose a reason for hiding this comment

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

Ugly, unfortunatelly this is the attribute we do not have in config, we do have application ID.

We could:
a) resolve applicationID to objectID through azure sdk,
b) store the applicationID in config as new value used just for this check.

Copy link
Member

Choose a reason for hiding this comment

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

Storing it in the config makes sense, assuming it does not change too often.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oki, moved to config

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

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

Okay thanks.

"github.com/RHEnVision/provisioning-backend/internal/telemetry"
)

const AZURE_SERVICE_ACCOUNT_OBJECT_ID = "bf9c3e0e-4bbd-425b-adb1-f005d3dfe9f9"
Copy link
Member

Choose a reason for hiding this comment

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

Storing it in the config makes sense, assuming it does not change too often.

@ezr-ondrej ezr-ondrej force-pushed the verify_azure_availability branch 2 times, most recently from 75761a6 to 31e08d6 Compare November 13, 2023 11:25
@lzap
Copy link
Member

lzap commented Nov 14, 2023

Relevant test error I believe.

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

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

Code looks good. Failure on CI tho.

@ezr-ondrej
Copy link
Member Author

/retest

Right, I've updated the account in ephemeral, lets 🙏

@lzap
Copy link
Member

lzap commented Nov 16, 2023

Bad luck.

@lzap
Copy link
Member

lzap commented Nov 16, 2023

Relevant failure error.

@ezr-ondrej ezr-ondrej force-pushed the verify_azure_availability branch from bd2e559 to 0564821 Compare November 21, 2023 11:53
@ezr-ondrej
Copy link
Member Author

I've completely changed the approach here. I've tried to verify the correct role assignment, but it's bit too ambitious as Azure lighthouse works bit differently.
I've decided to simplify this and only fetch the subscription metadata and get TenantID from there.
It does not verify the permissions completely, but it does improve the situation quite a bit.

Implement Azure availability check.
We are only checking that we can login to the given subscritpion and fetch metadata.
We currently fetch the tenant ID.
@ezr-ondrej ezr-ondrej force-pushed the verify_azure_availability branch from 0564821 to dabf313 Compare November 21, 2023 11:57
@ezr-ondrej
Copy link
Member Author

🍏

@lzap lzap merged commit 110af0c into RHEnVision:main Nov 22, 2023
6 checks passed
@lzap
Copy link
Member

lzap commented Nov 22, 2023

Thanks.

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.

2 participants