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

temp fix for synapse firewall resource #13511

Closed
wants to merge 2 commits into from

Conversation

ms-henglu
Copy link
Contributor

@ms-henglu ms-henglu commented Sep 26, 2021

To address issue: #13510

image

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @ms-henglu

Thanks for this PR.

Unfortunately this isn't guaranteed to be available within a minute (even if it responds that way at the moment) - as such we can't ship this with a Sleep here - can we poll on something else?

Thanks!

@owenfarrell
Copy link
Contributor

owenfarrell commented Sep 27, 2021

@tombuildsstuff - Sadly, I think the answer is "no". The existing implementation runs a Get immediately following the call to CreateOrUpdate.

But I think that means the 5-minute delay cited in the Azure documentation is on Microsoft's side of the PaaS. 😔

Short of injecting a Sleep in the resource (which, I agree, doesn't feel right), I think chaining a time_sleep resource is the only safe bet here.

EDIT: Disregard the above... no sooner did I write this that I see ProvisioningState on the firewall rules response.

Of note, I ran in to this same issue when running some recent acceptance tests. So I think "mocking" up the delay as part of a PreConfig might be a need in some of the Synapse tests.

@ms-henglu
Copy link
Contributor Author

ms-henglu commented Sep 28, 2021

Hi @tombuildsstuff , @owenfarrell , thanks for checking this PR.

Unfortunately this isn't guaranteed to be available within a minute (even if it responds that way at the moment)

Actually I've checked with service team, the delay is 1 minute. But I agree that this is not a good way to fix it.

So here's another solution. When IP is in the firewall ip range, we call GetWorkspace API to check whether firewall changes are taking effect. This is the recommended solution by service team.

@tombuildsstuff tombuildsstuff self-assigned this Sep 28, 2021
@tombuildsstuff
Copy link
Contributor

hi @ms-henglu

Thanks for pushing those changes and checking with the service team - however this approach introduces a dependency on a third-party web service which we also can't ship here - instead whilst I'd like to thank you for this contribution I'm going to close this in favour of #13525 which fixes this in a different manner.

Thanks!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants