search

odpi / egeria

Egeria core
https://egeria-project.org
Apache License 2.0
805 stars 260 forks source link

Remove netflix zuul library #4549

Closed lpalashevski closed 3 years ago

lpalashevski commented 3 years ago

We need to check the impact of removing netflix zuul library form the ui-spring-chassis spring boot UI application. As discussed on the community call, there are several reasons why we need to move this way:

cc: @sarbull @bogdan-sava @planetf1

sarbull commented 3 years ago

removing zuul doesn't increase any flexibility, we can deploy the UI and API separately with the zuul right now, we just have to tell the API where the UI is deployed(by URL). i don't think it can get simpler than this

if we want to remove zuul, we should remove everything that has to do with the UI that it is stored in the ui-chassis-spring and linked to the egeria-ui

the main idea of zuul was to allow the API to deliver the UI contents through it's context increasing the security level of what we send to the client, the session and all requests that are at Spring level

i don't think of removing zuul as being a step forward, moving this kind of config at server level(nginx) will actually minimize the flexibility of how users deploy..

i would rather go for the two complete separate deployments(cc: @planetf1 https://github.com/odpi/egeria/issues/4441#issuecomment-769214092):

  1. API (with CORS activated) (as a facade of the backend / ui-chassis-spring)
  2. UI deployed statically basically anywhere with a config file / json where you add the URL to the API

we didn't even check to see why we can't update Spring version and we already think of zuul as leftover

lpalashevski commented 3 years ago

The driving pricliple here was separation of concerns.

I am not in favour of emmbedding "reverse proxy" capability inside the backend app (server-cahssis-ui) just because it comes handy from development perspective. When you think from operational and maintainance it is restrictive because of the two reasons mentioned above. Additionally...

About the flexibility: We have to also consider different options and deployment architectures whrere clients can choose for different reasons: security, availability etc aspect that might be already established as practices and standards in an organization. I do not see need for us to prefer any deployment option.

About the maintainance: We are trying to minimize the effort and automate as much as possible to have regular updates on dependencies and spend less time identifying cross dependencies when some library contains vulnerability. In this case we need to update spring but we cannot because of compatibility issues with zuul (we depend on external release cycle).

For pointers why we cannot update see following discussion https://github.com/odpi/egeria/issues/4263#issuecomment-738044829

sarbull commented 3 years ago

it doesn't come in hand just from development perspective, the zuul approach is actually designed production first [0]

Zuul is an edge service that proxies requests to multiple backing services. It provides a unified “front door” to your system, which allows a browser, mobile app, or other user interface to consume services from multiple hosts without managing cross-origin resource sharing (CORS) and authentication for each one.

now this is a show stopper, [1] for reference, that's why they are not compatible anymore

The following Spring Cloud Netflix modules and corresponding starters will be placed into maintenance mode:

spring-cloud-netflix-archaius spring-cloud-netflix-hystrix-contract spring-cloud-netflix-hystrix-dashboard spring-cloud-netflix-hystrix-stream spring-cloud-netflix-hystrix spring-cloud-netflix-ribbon spring-cloud-netflix-turbine-stream spring-cloud-netflix-turbine spring-cloud-netflix-zuul

[0] - https://blog.heroku.com/using_netflix_zuul_to_proxy_your_microservices [1] - https://spring.io/blog/2018/12/12/spring-cloud-greenwich-rc1-available-now

lpalashevski commented 3 years ago

I did not mean just as dev tool, on contrary looks like can very poverfull services but it is just one of the options. My argument is more about the choice of embedding it as part of the application

Users can always install it as middleware gateway component to support all the cross cutting ascpect we mentioned before if they want to.

sarbull commented 3 years ago

Agree on that, the initial intentions of adding zuul was to minimize the pipeline changes and the time needed to integrate in the infrastructures at that moment.

planetf1 commented 3 years ago

If/when removed I think the main impact outside the UI itself is

planetf1 commented 3 years ago

We're now pushing out a few levels of spring versions, and have issues to resolve. would be good to address this for our Feb release (ie code we work on in feb)

sarbull commented 3 years ago

@planetf1 a major thing will be that we will access the UI directly not via the backend anymore. there will be a default setup for this, the public setup, and other solutions per case or by choice depending on the environment on where it will be deployed

cc: @lpalashevski

planetf1 commented 3 years ago

@sarbull yes, understood. It is a change from what we've had before - but it is very typical for UI deployments, so I think it's what we should do.

We should add some information to the readme for the release to point out the change, and update the supporting docs

For our samples we already include the front and backend, so it's just minor tweaks to config/docs

Overall we'll have a better end result :-)

sarbull commented 3 years ago

@planetf1 yes, on tomorrow's discussion me and @lpalashevski will present some technical aspects on the solution and output some action points

lpalashevski commented 3 years ago

Next steps following zull removal #4723

planetf1 commented 3 years ago

4710 appears to remove the build dependency on zuul - so we will have no re-routing in the spring UI. Is this the last step or the first, or do we need to do all together. If together it's going to be best to do in a single PR (though we can cherry-pick from the existing PRs)

4723 is proposing a change to nginx.conf -- but this needs to be configured specifically in each environment. Firstly the nature of the change required to get the ui working alongside the static content needs to be clear in the UI docs (be it in base or the egeria-ui repo). The fragment provided is likely useful for a developer, but will need to use the coco platform hostnames - in docker-compose these are fixed (so we could just have a static file), whilst in k8s these are variables and so we would need to insert a variable, or more likely use a k8s configmap for the nginx config - needs a bit further work

So can I clarify this is what I think is needed for containers

Meaning the tasks we have are @lpalashevski :

Does that make sense? Is the order correct to allow things to continue working at each step? (though we may consolidate)

For future thought: (cc: @sarbull )

sarbull commented 3 years ago

@planetf1 the final solution should also include updating the UI so that it contains a configuration file where we specify the API_URL from where it gets all its data and with the CORS feature enabler that @bogdan-sava implemented this should be the default mechanism for how we deploy the UI in production, one UI server and one API server(two different URLs).

The nginx proxy should be just another way of deploying the UI in production, as it works in some of the contexts we have, not the default public one which everyone should start with.

planetf1 commented 3 years ago

Ok @sarbull do you see that as a change you will make very soon - in which case the changes to the charts/compose will need to set that config rather than create a nginx config. If so, the work to setup that nginx config is then no longer needed so I would prefer to go straight to that if it's viable. Whilst nginx is relevant in production for more complex cases, the primary purpose of that chart is to get a lab environment working - nginx just being one way without being dependent on the UI change

That being said the egeria-ui container is currently based on nginx in any case, just to serve the static content

planetf1 commented 3 years ago

We will also need to test with decent certs (see #4722) - as these will need to be deployed to the egeria-ui container

sarbull commented 3 years ago

Ok @sarbull do you see that as a change you will make very soon - in which case the changes to the charts/compose will need to set that config rather than create a nginx config. If so, the work to setup that nginx config is then no longer needed so I would prefer to go straight to that if it's viable. Whilst nginx is relevant in production for more complex cases, the primary purpose of that chart is to get a lab environment working - nginx just being one way without being dependent on the UI change

That being said the egeria-ui container is currently based on nginx in any case, just to serve the static content

the solution i proposed would keep the deployment problem just at UI level, so that the egeria-ui container that is based on nginx would just serve the static content and configured with a given API_URL for the backend, pretty encapsulated i think.

unfortunately this isn't tracked by any issue tracker and it should be aligned with the Zuul removal PR.

i will raise a PR for the JS config so that it provides the given API_URL and raise an issue in Github odpi/egeria-ui.

planetf1 commented 3 years ago

@sarbull thanks - with that feature in place it would mean either solution would work. I do agree that having a specific backend api server configuration to me makes the most sense (even if that then points to an nginx deployment, or of course a k8s service -- plenty of flexibility)

I think you could do that change first, and could default to localhost:8443/api ? In that case it would work as today? then the other changes I listed above could be made in followup PRs (I think the ordering makes them compatible at each stage)

planetf1 commented 3 years ago

@sarbull What about timing? Do you think that change could be made soon (within a week) or more longer term ( a later release) ? Just so I can decide what to do about the remainder. There isn't a whole lot of difference in the approaches. Both mean a config setting needs adding to the containers

sarbull commented 3 years ago

@planetf1 i think it should not have a given default

as a technical implementation i see the following:

planetf1 commented 3 years ago

You could do a quick update to the container to set that variable to local host first (Egeria). Or I can. Then the variable can be used in the ui and it can be committed there without breakage.

With that done I can do more fix up of the charts and container and remove the default if needed?

Just trying to do the changes incrementally?

lpalashevski commented 3 years ago

@planetf1 i think it should not have a given default

as a technical implementation i see the following:

* default would mean just to prepend `''` as `API_URL` which will be used for any endpoint usage in the application via JS default import and sampled like this `${ API_URL }/api/users`, which will also work for the NGINX proxy approach

* something else would mean to prepend with `'http(s)://locahost:8443'` (`API_URL`)

@sarbull are you already working on this change? I cannot find issue open about the change.. I think we all agree that we are following this approach as default but we need to test it - has to work with https and self signed certs like it is configured in current docker image. Can you test it locally first?

planetf1 commented 3 years ago

It seems the first step is this UI change. Will keep an eye out for that distinct discussion and return here once done. Meanwhile I've ensured any other PRs relating to these changes are in draft status so they don't get merged until we're ready as we have some linked dependencies.

lpalashevski commented 3 years ago

As discussed on the latest security call, we are going to proceed in parallel adjusting the docker image that is already used to create k8s containers and progress also with testing new hosting including ssl configuration. This step is needed anyway because we are moving away from ui-server-chassis as front end entry point.

Note that this work is not conflicting egeria-ui work in progress (adding API_URL) and we will discuss the documentation updates and guides as separate topcis once it is done.

Sample for updating nginx configuration:

server {

    listen                443 ssl;
    server_name           ##ENV_VAR_FE_SERVER_NAME##;
    ssl_certificate       ##ENV_VAR_PATH_TO_CRT_FILE##;
    ssl_certificate_key   ##ENV_VAR_PATH_TO_KEY_FILE##;

    root /var/www/;
    index index.html;

    # Force all paths to load either itself (js files) or go through index.html.
    location / {
        try_files $uri /index.html;
    }

    location /api {
        proxy_pass ##ENV_VAR_UI_SERVER_CHASSIS_ADDRESS##;

        proxy_set_header Host $http_host; # useful because we still have some redirects
        proxy_ssl_verify       off; # useful  if we use self signed certs in ui-server-chassis
        proxy_ssl_session_reuse on;
        proxy_ssl_server_name on;
    }
}

More details about ssl module https://nginx.org/en/docs/http/ngx_http_ssl_module.html

planetf1 commented 3 years ago

**nginx is commonly used with Kubernetes.

Perhaps the best production-ready approach is to use the nginx ingress controller, which can be configured to manage the inbound requests in your k8s cluster. There's more on this at https://kubernetes.github.io/ingress-nginx/ . Configuration is dynamic and flexible and scalable

However there are intricacies of specific environments such as the relationship with Load Balancers that may mean slightly different setups for a local microk8s environment vs Azure or say IBMCloud. If we were properly using it we'd likely want to put all our web requests behind nginx.

I think longer term we should look more at this kind of configuration - in particular when going beyond setting up simple demos, and more into documenting typical deployment topologies. This needs to sit alongside work for example on our operator

However the current need is tactical. It's to aid in refactoring and is localized to the egeria UI interactions.

At the opposite extreme we could add nginx into the egeria-ui container to perform the local mapping - but this adds clutter and doesn't cleanly seperate the concerns.

I think a more suitable, tactical, approach is to make use of the nginx docker container https://hub.docker.com/_/nginx , to redirect requests accordingly either to the UI spring app, or static content. This could be deployed as a sidecar container (same lifecycle) but it's probably cleaner/simpler to have as a new pod.

This keeps concerns well separated, and the environment easy to debug. It also may make it easier to replicate in compose (leaving that until later). We can then proceed with zuul removal and further UI refactoring.

In parallel I can work on the egeria operator .. and at some point after that revisit a more appropriate example of using nginx for deployment

sarbull commented 3 years ago

I think we can move forward with the zuul removal, the API_URL feature is in place and it works like this:

You start in odpi/egeria with the ui-chassis-spring with the CORS feature ON.

And then in odpi/egeria-ui you start or build using this command:

npm run start --api-url=https://ui-chassis-spring.local.

planetf1 commented 3 years ago

i still need to complete some chart updates before we can remove, to avoid breaking charts. work in progress

planetf1 commented 3 years ago

We now have a configuration that works either

Note - docker-compose config still needs updating

lpalashevski commented 3 years ago

@planetf1 can we not point to the same docker file in egeria-ui repo for the docker compose ui sample in labs?

Instead using open-metadata-resources/open-metadata-deployment/docker/egeria-uistatic/Dockerfilechange references to https://github.com/odpi/egeria-ui/blob/master/Dockerfile (?)

lpalashevski commented 3 years ago

ah I see, disregard my previous comment. we still need the npm build -- then we still need this dockerfile to have npm build + hosting sample in the labs

planetf1 commented 3 years ago

The work in docker compose is only around the yml Configuration and proxy setup. Should be No change needed to either docker images or build process

planetf1 commented 3 years ago

Also the docker file you mentioned is being removed. The Egeria ui repo will be responsible for building with npm then publishing a docker image based on that output

planetf1 commented 3 years ago

There are so many ways to manage this of course that will technically work, but my approach has been decomposition. Try and keep each image s simple as possible delivering just one capability that can be consumed in different ways. Then where we need orchestration and aggregation we do that with tools like compose and helm/k8s

lpalashevski commented 3 years ago

Difinately agree!

planetf1 commented 3 years ago

see https://github.com/odpi/egeria/pull/4936 for the docker-compose update to support nginx - please review.

planetf1 commented 3 years ago

Note:

planetf1 commented 3 years ago

This has been done so we can close - we already have a followon issue to track updating of spring dependencies