Skip to content

Add detection of Azure App Service to CloudPlatform #25829

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

Closed

Conversation

jdubois
Copy link
Contributor

@jdubois jdubois commented Mar 30, 2021

Add Azure platform support to Spring Boot.

More specifically, this adds "Azure App Service" support to Spring Boot - as this is the Azure standard PaaS service, it makes sense to give it the "AZURE" enum name. If it provides any additional value, we can add later Azure Functions and Azure Spring Cloud, but those shouldn't be the default and would have their own enumeration name, as they work very differently.

This discovers Azure using the "WEBSITE_SITE_NAME" environment properties, which is similar to what is done with Heroku (using the "DYNO" environment property).

I put Azure near the top of the file, trying to have some alphabetical sorting after "NONE", as I believe this is how people will read that class.

As a consequence, setting this value should put server.forward-headers-strategy=native by default, which will allow applications to receive sensitive security headers from the reverse proxy. More specifically, this will solve this error, when using Active Directory to secure App Service, which looks pretty common: https://stackoverflow.com/questions/60365017/redirect-url-for-spring-oauth2-app-on-azure-with-active-directory-invalid-redir

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 30, 2021
@jdubois
Copy link
Contributor Author

jdubois commented Mar 31, 2021

After internal discussion at Microsoft, I'm going to modify this PR to use "AZURE_APP_SERVICE" instead of "AZURE", in order to be clear on the service that is being used.

@cuspymd
Copy link

cuspymd commented Mar 31, 2021

There seems to be no problem with the operation, but I'm afraid I can't understand it. It's a very common variable name, could you be sure that only azure uses this variable name?

Is it difficult to add an environment variable representing azure? It may sound rude, but I ask because I'm really curious.

@jdubois
Copy link
Contributor Author

jdubois commented Mar 31, 2021

You are correct @cuspymd but unfortunately there isn't a good specific environment variable for Azure. Let me try to find a second one, and then I'll use the combinaison of them to reduce the risk.

@jdubois
Copy link
Contributor Author

jdubois commented Mar 31, 2021

It looks like "WEBSITES_ENABLE_APP_SERVICE_STORAGE" is used everywhere (at least for Java and Docker, and they are the only ones interesting for Spring), so I'm going to use both variables in order to reduce the risk. Thanks @cuspymd !

@cuspymd
Copy link

cuspymd commented Apr 1, 2021

@jdubois Thank you for checking.

Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @jdubois. I've left a couple of comments for your consideration.

Copy link
Contributor Author

@jdubois jdubois left a comment

Choose a reason for hiding this comment

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

I fixed both issues, @wilkinsona can you check and approve the review?

@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 6, 2021
@wilkinsona wilkinsona added this to the 2.5.x milestone Apr 6, 2021
@wilkinsona wilkinsona self-requested a review April 6, 2021 09:01
@wilkinsona wilkinsona changed the title Add Azure platform support Add detection of Azure App Service to CloudPlatform Apr 7, 2021
@wilkinsona wilkinsona closed this in 4d510d3 Apr 7, 2021
@wilkinsona wilkinsona modified the milestones: 2.5.x, 2.5.0-RC1 Apr 7, 2021
@wilkinsona
Copy link
Member

Thanks very much, @jdubois. The proposed changes have now been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants