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

Don't use CDN as default and enable it from appsettings #7599

Closed
wants to merge 2 commits into from

Conversation

hishamco
Copy link
Member

CDN

@hishamco hishamco requested a review from agriffard November 12, 2020 15:57
@jptissot
Copy link
Member

Maybe add a way to disable displaying this from the setup page if the value is configured from an environment variable or appsettings.json

@hishamco
Copy link
Member Author

@jptissot if we need to add the ability to control it from the appsettings.json many options should have the settings

@sebastienros
Copy link
Member

Isn't the issue that this setting should be there, as soon as in the setup, only if the user who does the setup doesn't have access to the CDNs? Since this can be changed afterwards.

@hishamco
Copy link
Member Author

Please let me check the original issue again

@hishamco
Copy link
Member Author

This is the original PR #5969, but I can easily add a settings as @jptissot mentioned above

@deanmarcussen
Copy link
Member

I am with @sebastienros on this one @jptissot

I think it's too much to be having appsettings to hide a cdn options dropdown.

Especially when setup hasn't run so you don't even know the box is there yet.

@hishamco
Copy link
Member Author

So should we make it visible by default or remove the setting at all?

@Skrypt
Copy link
Contributor

Skrypt commented Nov 13, 2020

Guys, make it automatic by adding some CDN download tester. If it fails then it should simply fallback to the local resources.

@jptissot
Copy link
Member

I am with @sebastienros on this one @jptissot

I think it's too much to be having appsettings to hide a cdn options dropdown.

Especially when setup hasn't run so you don't even know the box is there yet.

I just did not want to see that option on my sites as I disable the cdn's by default.

@sebastienros
Copy link
Member

make it automatic by adding some CDN download tester

That was my reaction at first too. But then I started thinking about what this settings is trying to accomplish, and it's not easy to find a good solution: We have a site settings to support no cdns such that the admin will work even for users where cdns are blocked. It means that if the settings is not correctly set during setup, and the admin has cdns blocked, we might not be able to change it.

Now two cases: the user who sets up the site has access to cdns or does not.

1- Super user has access to cdns: they can change it later on in the admin. It can also be part of the recipe. Having an option in the Setup screen is not necessary.
2- Super user doesn't have access to cdns: they might not be able to change it later as the admin requires some scripts. And the recipes might not have it, so it would require to have custom ones. Having an option in the Setup screen is necessary.

But then assume that the user doesn't have access to cdns, how does the Setup work in this case? What is our default Setup experience? I assume it loads bootstrap and more from CDNs, so how would the Setup look like in this case. Is Setup even too late to choose this option?

Detecting CDNs availability dynamically is flaky. What CDNs do we test? Do we have to change it as soon as we use a resource from a CDN that could be blocked somewhere?

To me it looks like if a setting needs to be set, it's in the app settings. Such that when a tenant is created (or for transient tenants like during setup), it's taking the site settings into account. And the Setup would also take that into account for free, so the Setup would also server local assets if the app settings define it.

@sebastienros
Copy link
Member

On the other PR, the contributor mentioned that for new users it might not be obvious to know how to change in the recipe. That's a valid point too. Even for the first Setup screen experience.

So even with this options in app settings, and even with a drop down, would we not want the Setup to use local assets at all times? Which would then also work if you don't have internet and just want to install locally?
Then extrapolating it, would we not want it to be local by default, and let users enable CDNs as a perf optimization?

I think we could organize a conference and keep busy for days just talking about it ;) And not even be bored!

@jtkech
Copy link
Member

jtkech commented Nov 13, 2020

@sebastienros

But then assume that the user doesn't have access to cdns, how does the Setup work in this case? What is our default Setup experience? I assume it loads bootstrap and more from CDNs, so how would the Setup look like in this case. Is Setup even too late to choose this option?

After a quick look to the specific _layout.cshtml of the setup module, not sure at 100% but it seems that he setup view already only uses local resources

@jptissot
Copy link
Member

I agree, setup should only use local assets !

@deanmarcussen
Copy link
Member

Yeah the specific issue is that setup bundles jquery but the admin does not.

So the menu doesn’t work and you can’t navigate to settings to turn off the cdn.

Maybe...
Cdn of by default in dev (but on in prod by default... like resource debugging)
Or maybe bundle jquery in the admin too.

The cdn option is not so much (I think) about the admin area but front end delivery

@sebastienros
Copy link
Member

The cdn option is not so much (I think) about the admin area but front end delivery

I disagree a little. We don't manage what themes will do, though it will apply to any resource, even custom ones for the front-end.
So it looks like we should assume it applies for both. So should the setting, as I don't think there should be two settings.

@Skrypt
Copy link
Contributor

Skrypt commented Nov 14, 2020

Ok so if I summarize.

  • Using local asset by default won't cover all use cases (recipes) and won't fix it 100%.
  • Using a CDN download tester could be flaky, could be any network issue making download failing.
  • It should not be a different setting for frontend and backend.
  • Setup already uses local assets.
  • Admin UI fails to render properly and it doesn't allow to get to the settings to change them.

Now, I'm thinking about websites on production that could experience network issues with CDN's and we don't provide any fallback mechanism if that ever happens on a website. So, if anything wrong happens with the CDN service, the production website will also be down ; this could be prevented. If you want to ensure that your website works properly it should maybe have some fallback mechanism. This is mainly why I was thinking about the option of testing CDN's?

  • Test CDN DNS on first request (Show a warning)
  • Test CDN DNS periodically every X time
  • Failsafe mechanism as soon as a CDN request fails

Even if this seems like the only obvious path that would cover all use cases. At the same time, do we want to add this kind of processing overhead? Maybe not ...

An other option I can see is that we could provide a safe mode admin theme which would use none dependencies for when the current admin theme returns too much network issues coming from the CDN's. I don't like this idea at all because we should not need to do this if we have the assets locally.

I'd need to analyze more the appsettings.json solution. Maybe a setting that forces CDN's on/off globally could fix the issue with recipes?

This was referenced Nov 23, 2020
@hishamco
Copy link
Member Author

@skypt it has been awhile coz my hard disk accidently have a bad sectors, I was disconnected for 5 days, now I'm trying to recover everything. Regarding this PR shall we go with appsettings.json option or could we add a global setting per tenant, so whenever the tenants is running it could be toggled on / off depending on the use case

/cc @deanmarcussen @sebastienros @jptissot

@hishamco
Copy link
Member Author

hishamco commented Dec 3, 2020

Any updates on this?

@deanmarcussen
Copy link
Member

So continuing the lengthy discussion on this one, because I think we need to look for another way to resolve the issue.

I think what @jptissot has identified is that we shouldn't ever add settings to the Setup Screen, when they can be set by a recipe.

What we could have, if this was to be a setup choice, is parameterized recipes, discussed a few times, and still open for grabs, for whowever wants to work on it. Related issues #495 #396

In this way our recipes could include a parameter definition for Use Cdn, but other recipes would have that parameter, so not draw the ui for it.

There are some big pluses in doing that, in terms of being able to also have options (set via the recipe), for selecting a set of cultures, and/or whatever features you want to help preconfigure.

@hishamco
Copy link
Member Author

hishamco commented Dec 4, 2020

May it's better as Dean mentioned to add such preconfigured settings in recipe files

@sebastienros
Copy link
Member

And about using local resources by default (as in the options object)? You can still force a value in the recipes, but our default recipes would not have any values. And some performance optimization section in the docs would explain to turn it on.

We can use dev/prod because you might setup a site in prod and have the same issue.

@agriffard
Copy link
Member

appsettings.json conflict to solve.

@Skrypt
Copy link
Contributor

Skrypt commented Dec 20, 2020

@hishamco Please, if you can, prioritize this PR. A lot of people asked for this to work. We should not have a settings to select from the setup UI.

@Skrypt Skrypt changed the title Add Use CDN option to setup screen Don't Use CDN as default and enable it from appsettings Dec 20, 2020
@Skrypt Skrypt changed the title Don't Use CDN as default and enable it from appsettings Don't use CDN as default and enable it from appsettings Dec 20, 2020
@hishamco
Copy link
Member Author

In this way our recipes could include a parameter definition for Use Cdn, but other recipes would have that parameter, so not draw the ui for it.

@deanmarcussen if I added a variable in recipes for that, how can I access its value from within the SetupController?

@deanmarcussen
Copy link
Member

And about using local resources by default (as in the options object)? You can still force a value in the recipes, but our default recipes would not have any values. And some performance optimization section in the docs would explain to turn it on.

We can use dev/prod because you might setup a site in prod and have the same issue.

@hishamco I think the above is the better option to consider for now, i.e. change the site settings to be Cdn = false by default.
And add some docs about it.

In this way our recipes could include a parameter definition for Use Cdn, but other recipes would have that parameter, so not draw the ui for it.

@deanmarcussen if I added a variable in recipes for that, how can I access its value from within the SetupController?

You can't right now @hishamco . Parameters are the nicest idea, but that is a large pr to add support for that. My point was that's the feature we need, not adding extra settings to the setup screen.

@hishamco
Copy link
Member Author

We can use dev/prod because you might setup a site in prod and have the same issue.

That means a settings value from appsettings.json should be used in SetupController

@hyzx86
Copy link
Contributor

hyzx86 commented Jan 15, 2021

Maybe we can refer to the practice of "Flash player", which uses CDN, by default, such as

var cdnAvaliable=checkCdn() // some code to check cdn is avaliable

if(!cdnAvaliable && confirm("The CDN server is not avaliable, do you want to disable CDN settings?, after you can re-enable the CDN option from XXX")){
 disbaleCDN()
}

@hyzx86
Copy link
Contributor

hyzx86 commented Jan 15, 2021

insert the codes to the admin dashboard's _Layourt file, and login page

@sebastienros
Copy link
Member

I created this issue: #8324

@agriffard agriffard deleted the hishamco/use-cdn branch October 5, 2021 19:30
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.

8 participants