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

♻️ add loading of self signed certs to director-v2 and webserver entrypoint.sh #3678

Conversation

mrnicegyu11
Copy link
Member

adds loading of self signed certificates to director-v2 and webserver

@mrnicegyu11 mrnicegyu11 added a:webserver issue related to the webserver service a:director-v2 issue related with the director-v2 service enhancement labels Dec 14, 2022
@mrnicegyu11 mrnicegyu11 added this to the Zefram Cochrane milestone Dec 14, 2022
@mrnicegyu11 mrnicegyu11 self-assigned this Dec 14, 2022
# In case the service must access a docker registry in a secure way using
# non-standard certificates (e.g. such as self-signed certificates), this call is needed.
# It needs to be executed as root.
update-ca-certificates
Copy link
Member

Choose a reason for hiding this comment

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

Q: why the web-server needs this? AFAIK it does not access the docker registry directly

Copy link
Member Author

Choose a reason for hiding this comment

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

the comment was copy pasted sorry, let me adjust it

# In case the service must access a docker registry in a secure way using
# non-standard certificates (e.g. such as self-signed certificates), this call is needed.
# It needs to be executed as root.
update-ca-certificates
Copy link
Member

Choose a reason for hiding this comment

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

same question here, the docker registry is accessed via the director right?

Copy link
Member Author

Choose a reason for hiding this comment

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

adjusted the comment, for the director this was added anticipatory, I at this point dont know if or which outside services the direcot-v2 might need.

But for sure, the director-v2 will eventually have to pass the self-signed cert onwards to the dynamic-sidecars and dy-proxys, so I guess he needs it and it is ok if the director-v2 trusts this self-signed certificate

Copy link
Contributor

Choose a reason for hiding this comment

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

What services is this accessing via https? usually it access stuff inside the docker network where there is no https. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it is not acessing https services, but why would the director-v2 not trust a certificate it will instruct the dy-sidecars spawned by it to trust? :D maybe I am missing something here

Copy link
Contributor

Choose a reason for hiding this comment

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

My indirect question was something on the lines of: what issues does this fix?

@mrnicegyu11
Copy link
Member Author

Thanks please re-review @pcrespov

@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #3678 (78b6129) into master (2fae099) will increase coverage by 5.6%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3678      +/-   ##
=========================================
+ Coverage    83.4%   89.0%    +5.6%     
=========================================
  Files         883     338     -545     
  Lines       37398   17206   -20192     
  Branches      786       0     -786     
=========================================
- Hits        31197   15325   -15872     
+ Misses       5992    1881    -4111     
+ Partials      209       0     -209     
Flag Coverage Δ
integrationtests 67.1% <ø> (+14.3%) ⬆️
unittests 84.1% <ø> (+2.5%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ore_service_director_v2/utils/dask_client_utils.py 80.8% <0.0%> (-6.1%) ⬇️
..._postgres_database/models/direct_acyclic_graphs.py
...irector/rest/generated_code/models/simcore_node.py
...ings-library/src/settings_library/utils_service.py
...catalog/src/simcore_service_catalog/core/events.py
...imcore_service_datcore_adapter/api/module_setup.py
...ervice_dynamic_sidecar/api/containers_extension.py
...ervice_autoscaling/api/dependencies/application.py
...vice_dynamic_sidecar/modules/long_running_tasks.py
...dels-library/src/models_library/rest_pagination.py
... and 559 more

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

At this point I'm ok with it as well.

@sanderegg sanderegg merged commit 1ca5611 into ITISFoundation:master Dec 15, 2022
@sonarcloud
Copy link

sonarcloud bot commented Dec 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:director-v2 issue related with the director-v2 service a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants