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

Remove netflix zuul library #4549

Closed
lpalashevski opened this issue Jan 29, 2021 · 36 comments
Closed

Remove netflix zuul library #4549

lpalashevski opened this issue Jan 29, 2021 · 36 comments
Assignees
Milestone

Comments

@lpalashevski
Copy link
Contributor

lpalashevski commented Jan 29, 2021

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:

  • Detaching this extra dependency will help improving the security lifecycle and updates we regularly provide (See Update Spring dependencies once netflix zuul block addressed #4441 for more information)
  • Removing zuul will increase flexibility and allow users to decide for themselves on most appropriate hosting configuration with API gateway component i.e. still go with zull setup or choose different technology such as Nginx for the same purpose.

cc: @sarbull @bogdan-sava @planetf1

@lpalashevski lpalashevski changed the title Remove netflix zull library Remove netflix zuul library Jan 29, 2021
@sarbull
Copy link
Member

sarbull commented Jan 29, 2021

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 #4441 (comment)):

  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
Copy link
Contributor Author

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 #4263 (comment)

@sarbull
Copy link
Member

sarbull commented Jan 29, 2021

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
Copy link
Contributor Author

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
Copy link
Member

sarbull commented Jan 29, 2021

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
Copy link
Member

planetf1 commented Feb 1, 2021

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

  • on docs for setting up the UI
  • the sample helm charts we have for labs/vdc

On the latter the change should be simple - please raise an issue or @mention me when needed. It's likely minor, again maybe just docs ( port/URL changes) or minor changes to container config.

@planetf1
Copy link
Member

planetf1 commented Feb 1, 2021

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
Copy link
Member

sarbull commented Feb 1, 2021

@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
Copy link
Member

planetf1 commented Feb 1, 2021

@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
Copy link
Member

sarbull commented Feb 1, 2021

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

@lpalashevski
Copy link
Contributor Author

Next steps following zull removal #4723

@planetf1
Copy link
Member

planetf1 commented Mar 4, 2021

#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

  • Run the ui server chassis on host1:8443 (this is effectively the api server)
  • Run the egeria-ui container on host2:80 with a proxy passthrough for /api to host1:8443/api
  • Users get to the ui via host2:80

A few observations from this

  • The egeria-ui container should really support https as well as http (maybe it does already) --then change the above to host2:8443 ? Just config I think?
    - an nginx config needs to be added for docker-compose and k8s 'lab' environments with the proxy passthrough

Meaning the tasks we have are @lpalashevski :

  • Add nginx / proxy passthrough definition for docker compose
  • Add nginx / proxy passthrough definition for the odpi-egeria-lab chart
  • ( I think the above changes do not prevent the UI working as it is today)
  • update egeria ui notebook (which has little beyond the URL to launch) & READMEs for how to open the UI in these envs
  • Commit Build of UI without zuul @bogdan-sava
  • Docs update on how to run the UI 'manually'
  • Update spring dependencies

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 )

  • Migrate egeria-ui build to github actions
  • Migrate build of egeria-ui docker container to egeria-ui
  • consider whether ui chassis should move out of egeria repo
  • Agreeing release 'process' - ie with dependencies between the repos

@sarbull
Copy link
Member

sarbull commented Mar 4, 2021

@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
Copy link
Member

planetf1 commented Mar 4, 2021

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
Copy link
Member

planetf1 commented Mar 4, 2021

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

@sarbull
Copy link
Member

sarbull commented Mar 4, 2021

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
Copy link
Member

planetf1 commented Mar 4, 2021

@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
Copy link
Member

planetf1 commented Mar 4, 2021

@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
Copy link
Member

sarbull commented Mar 4, 2021

@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)

@planetf1
Copy link
Member

planetf1 commented Mar 4, 2021

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
Copy link
Contributor Author

lpalashevski commented Mar 4, 2021

@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
Copy link
Member

planetf1 commented Mar 4, 2021

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.

sarbull added a commit that referenced this issue Mar 18, 2021
sarbull added a commit that referenced this issue Mar 18, 2021
Signed-off-by: Sirbu Nicolae-Cezar <sirbunicolaecezar@gmail.com>
sarbull added a commit that referenced this issue Mar 18, 2021
@planetf1
Copy link
Member

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

planetf1 added a commit to planetf1/egeria that referenced this issue Mar 19, 2021
Signed-off-by: Nigel Jones <nigel.l.jones+git@gmail.com>
planetf1 added a commit to planetf1/egeria that referenced this issue Mar 19, 2021
Signed-off-by: Nigel Jones <nigel.l.jones+git@gmail.com>
planetf1 added a commit to planetf1/egeria that referenced this issue Mar 19, 2021
Signed-off-by: Nigel Jones <nigel.l.jones+git@gmail.com>
planetf1 added a commit to planetf1/egeria that referenced this issue Mar 19, 2021
Signed-off-by: Nigel Jones <nigel.l.jones+git@gmail.com>

Signed-off-by: Nigel Jones <nigel.l.jones+git@gmail.com>
planetf1 added a commit to planetf1/egeria that referenced this issue Mar 19, 2021
Signed-off-by: Nigel Jones <nigel.l.jones+git@gmail.com>
planetf1 added a commit to planetf1/egeria that referenced this issue Mar 22, 2021
Signed-off-by: Nigel Jones <nigel.l.jones+git@gmail.com>
@planetf1
Copy link
Member

We now have a configuration that works either

  • going to the ui chassis 8443 -> redirects using zuul
  • going to the nginx proxy config 443 -> redirects with nginx.

This now seems to prove the mechanism

Note - docker-compose config still needs updating

@lpalashevski
Copy link
Contributor Author

lpalashevski commented Mar 22, 2021

@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
Copy link
Contributor Author

lpalashevski commented Mar 22, 2021

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
Copy link
Member

planetf1 commented Mar 22, 2021

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

Difinately agree!

sarbull added a commit that referenced this issue Mar 22, 2021
Signed-off-by: Sirbu Nicolae-Cezar <sirbunicolaecezar@gmail.com>
planetf1 added a commit to planetf1/egeria that referenced this issue Mar 24, 2021
Signed-off-by: Nigel Jones <nigel.l.jones+git@gmail.com>
planetf1 added a commit to planetf1/egeria that referenced this issue Mar 24, 2021
Signed-off-by: Nigel Jones <nigel.l.jones+git@gmail.com>
@planetf1
Copy link
Member

see #4936 for the docker-compose update to support nginx - please review.

@planetf1
Copy link
Member

Note:

  • Will do doc updates prior to shipping next release & will add to readme (for the new URL)
  • Once the compose update is in, we can remove zuul (and then retest asap) 0 either before or after release as you wish
  • I would leave spring updates until the next cycle

planetf1 added a commit that referenced this issue Mar 25, 2021
…l-module

Revert "Revert "Remove Zuul Module #4549 #3795""
@planetf1 planetf1 modified the milestones: 2020.03 (2.8), 2020.04 (2.9) Mar 29, 2021
@planetf1
Copy link
Member

planetf1 commented Apr 7, 2021

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

@planetf1 planetf1 closed this as completed Apr 7, 2021
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

No branches or pull requests

3 participants