feat: make argocd installation manifests Istio compatible #2784 #3893

Cajga · 2020-07-02T18:34:00Z

Checklist:

[ X ] Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
[ X ] The title of the PR states what changed and the related issues number (used for the release note).
[ X ] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
Optional. My organization is added to USERS.md.
[ Partly ] I've signed the CLA and my build is green (troubleshooting builds).

I've signed the CLA but apart from making some simple tests, did not run the tests locally.

This PR makes argocd installation manifests compatible with Istio. It implements this request: #2784

In case this is accepted I will make a PR to have 2 examples with Istio ingress:

TLS termination at Istio Ingress gateway
TLS termination at ArgoCD server (Istio ingress gateway is configured to TLS PASSTHROUGH)

…stio compatibility

Cajga · 2020-07-02T18:38:51Z

Please note that Istio is about to switch to use the labels recommended by kubernetes.
Based on the latest comment it is already working with those so the PR is making sure that argocd is using the new labels for deployments (by adding app.kubernetes.io/version to it). The PR uses argocd version for each deployments for the label.

codecov · 2020-07-02T18:43:56Z

Codecov Report

Merging #3893 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3893   +/-   ##
=======================================
  Coverage   42.44%   42.44%           
=======================================
  Files         120      120           
  Lines       17567    17567           
=======================================
  Hits         7456     7456           
  Misses       9161     9161           
  Partials      950      950

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21c93d9...8237f66. Read the comment docs.

Cajga · 2020-07-02T18:49:52Z

Hmm... seems I got kustomize3... I will downgrade to 2 and will commit the fixes required.

Cajga · 2020-07-02T19:55:18Z

manifests/base/server/argocd-server-service.yaml

-  - name: http
+  - name: tcp-argocdserver
    protocol: TCP
    port: 80
    targetPort: 8080
-  - name: https
+  - name: tcp-argocdservertls
    protocol: TCP
    port: 443
    targetPort: 8080


Please note the ideally we would remove port 80 and keep only 443. In that case we could use https as protocol (istio does not support multiple protocols for a single service ports and as such we have to go down to TCP which works fine with two ports but we will lack http level information in Istio telemetry).
I did not want to put that into the PR as that would be probably too big change... or?

We can't remove port 80 from this service, because many people actually run ArgoCD using HTTP mode and doing TLS termination elsewhere.

Yes, that is my plan as well (doing TLS termination at the Istio gateway level) but to be honest, I believe we should have one way of doing it as default (like keep 443 as that is a simple, "secure" setup) and document all the other possibilities (like how to do TLS termination elsewhere).
I have Istio config for both (Istio passthrough with TLS termination at argocd-server and Istio TLS termination with Istio mTLS and argocd-server using http) and planning to send a PR to your Ingress docu page.

Cajga · 2020-07-02T20:01:48Z

hack/update-manifests.sh

+# update label patches with current version
+for onedir in base/application-controller/overlays base/dex/overlays base/repo-server/overlays base/redis/overlays base/server/overlays ha/base/redis-ha/overlays; do
+  cd ${SRCROOT}/manifests/${onedir}
+  for onetemplate in `ls -1 *.tmpl`;do
+    sed "s/ARGOCD_VERSION_TO_BE_REPLACED/${ARGOCD_VERSION_LABEL}/g" ${onetemplate} > ${onetemplate%.*}.yaml
+  done
+done
+


As the version is a variable and kustomize does not support such external variables (more info on this here) I had to introduce "simple templating" to properly set the version labels on deployments with kustomize

Cajga · 2020-07-15T10:26:13Z

@jannfis would you need any more information to get this reviewed/merged?

jannfis · 2020-07-21T19:15:10Z

@Cajga Awesome work! I was first under the impression that this change would be somewhat intrusive to the manifests, but I think the pros outweigh the cons. I think that especially the version tag is very useful, also for troubleshooting ("Does someone use old versions of manifests with a newer version of the application").

@alexmt @jessesuen are there any dependencies on service names that I don't know of?

jannfis

LGTM. Waiting for others to raise their hands, if necessary.

jannfis

I just noticed that changing the service names would be a breaking change for many that refer to service names in their ingress.

Maybe it would make sense to provide a new set of services compatible with Istio instead of modifying the existing ones?

Cajga · 2020-07-24T11:58:55Z

I just noticed that changing the service names would be a breaking change for many that refer to service names in their ingress.

Maybe it would make sense to provide a new set of services compatible with Istio instead of modifying the existing ones?

We do not change the service names but we do the name of the service ports. I think you were referring to that and yes, seems you are right, this would break the normal ingress config if it follows the examples from the website.

So, should we have new install files compatible with istio? If yes, which way would you prefer:

under its own directory (like manifests/istio-compatible/install.yaml)?
just append -istio to the name of the existing yaml files (like manifests/install-istio.yaml etc.)

yogeek · 2021-02-03T15:24:47Z

Hello everyone :)

@Cajga I am currently struggling to expose ArgoCD UI with Istio Gateway/VirtualService and I would like to know if this PR is still ongoing or otherwise, where can I find intel about the good argocd-server options (deployment options + service config) and the corresponding Istio manifests.
Thank you

sbose78 · 2021-04-01T14:11:36Z

Queued for discussion at the contributor meeting

omidb · 2022-03-25T16:32:01Z

Hi, thanks for working on this. what's the status of this PR? @Cajga @sbose78
Is there any other activity on Istio + argocd ?

roee-landesman · 2022-04-08T18:56:27Z

I'm also in support of this PR as we use Istio for ingress and would like a native integration with ArgoCD. Thanks!

tafaust · 2022-07-27T07:16:28Z

@jannfis @Cajga Any chance this PR can make its way into upstream? We are currently on the brink of whether to opt for argocd or fluxcd in our company.

crenshaw-dev · 2022-08-08T16:24:48Z

@jannfis as far as I can tell, your concern about this PR (changing service names) has been addressed. Are we okay to move forward?

@Cajga are you still available to resolve conflicts? If not, would someone like to open a new PR based on this one?

Hronom · 2023-04-01T14:41:55Z

This PR relates to this issue #2784 please can you merge it?

pre · 2023-04-12T11:29:28Z

Would be superb to get this merged OR create list of steps required to get this merged.

Istio requires the application protocol be specified in the Service resources so that the port's name is protocol-suffix (eg http-metrics).

It'd be a straightforward change for ArgoCD to name the ports as eg.http-metrics instead of metrics in the default manifests. Now these need to be overridden eg. with kustomize when deploying argocd with istio mtls:

metrics -> http-metrics
webhook -> http-webhook
server -> https-server (argocd-repo-server)

https://istio.io/latest/docs/ops/configuration/traffic-management/protocol-selection/

howardjohn · 2023-04-12T20:00:49Z

I just noticed that changing the service names would be a breaking change for many that refer to service names in their ingress.

Maybe it would make sense to provide a new set of services compatible with Istio instead of modifying the existing ones?

You could set appProtocol instead. That should be backwards compatible, is supported by istio, and is likely supported by other things as it is a k8s-defined standard.

pre · 2023-04-13T04:16:38Z

I just noticed that changing the service names would be a breaking change for many that refer to service names in their ingress.
Maybe it would make sense to provide a new set of services compatible with Istio instead of modifying the existing ones?

You could set appProtocol instead. That should be backwards compatible, is supported by istio, and is likely supported by other things as it is a k8s-defined standard.

Service name does not need to be changed, only the port name is.

Probably setting the appProtocol would work too, however, it might perhaps break on a non-istio setup? Currently it takes a lot of trial and error to get Istio mTLS working with ArgoCD due to the missing protocol definitions in argocd manifests (unless one finds eg this Github issue which defines the correct values).

Especially the argocd-repo-server https-server was tricky to find out because all other services need a http-prefix.

Also the argocd-server https port needs to be removed because argocd-server already has http. Argocd can’t correctly detect the connection type with istio mtls hence the https port mixes up the setup.

Full set of required changes is here: #2784 (comment)

EDIT: Also the dex service port needs to be renamed from http to either https or tcp: #6183 (comment)

Cajga · 2023-04-23T18:27:42Z

Hi All, sorry but we are not using ArgoCD nor Istio anymore. I am afraid someone will have to take this PR over as I have to focus on projects that we use.

eightseventhreethree · 2023-05-08T18:40:48Z

This is an issue for us. I manually renamed the port, but this means that upgrades to the chart will break SSO.

todaywasawesome

The author of this PR is no longer working on it, so I'm going to close it but I strongly encourage someone else to pick it up and get it over the finish line.

add version labels to deployments and rename ports for services for I…

4a06cea

…stio compatibility

commit with manifests generated with kustomize2

8237f66

Cajga commented Jul 2, 2020

View reviewed changes

jannfis approved these changes Jul 21, 2020

View reviewed changes

jannfis self-requested a review July 24, 2020 06:20

jannfis requested changes Jul 24, 2020

View reviewed changes

jessesuen assigned mayzhang2000 Sep 2, 2020

crenshaw-dev unassigned mayzhang2000 Aug 8, 2022

Hronom mentioned this pull request Apr 12, 2023

Argo CD Istio deploy compatibility #2784

Open

todaywasawesome requested changes Jun 28, 2023

View reviewed changes

todaywasawesome closed this Jun 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make argocd installation manifests Istio compatible #2784 #3893

feat: make argocd installation manifests Istio compatible #2784 #3893

Cajga commented Jul 2, 2020

Cajga commented Jul 2, 2020

codecov bot commented Jul 2, 2020 •

edited

Loading

Cajga commented Jul 2, 2020

Cajga Jul 2, 2020

jannfis Jul 21, 2020

Cajga Jul 22, 2020

Cajga Jul 2, 2020 •

edited

Loading

Cajga commented Jul 15, 2020

jannfis commented Jul 21, 2020

jannfis left a comment

jannfis left a comment

Cajga commented Jul 24, 2020

yogeek commented Feb 3, 2021 •

edited

Loading

sbose78 commented Apr 1, 2021

omidb commented Mar 25, 2022

roee-landesman commented Apr 8, 2022

tafaust commented Jul 27, 2022 •

edited

Loading

crenshaw-dev commented Aug 8, 2022

Hronom commented Apr 1, 2023

pre commented Apr 12, 2023 •

edited

Loading

howardjohn commented Apr 12, 2023

pre commented Apr 13, 2023 •

edited

Loading

Cajga commented Apr 23, 2023

eightseventhreethree commented May 8, 2023

todaywasawesome left a comment

feat: make argocd installation manifests Istio compatible #2784 #3893

feat: make argocd installation manifests Istio compatible #2784 #3893

Conversation

Cajga commented Jul 2, 2020

Cajga commented Jul 2, 2020

codecov bot commented Jul 2, 2020 • edited Loading

Codecov Report

Cajga commented Jul 2, 2020

Cajga Jul 2, 2020

Choose a reason for hiding this comment

jannfis Jul 21, 2020

Choose a reason for hiding this comment

Cajga Jul 22, 2020

Choose a reason for hiding this comment

Cajga Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Cajga commented Jul 15, 2020

jannfis commented Jul 21, 2020

jannfis left a comment

Choose a reason for hiding this comment

jannfis left a comment

Choose a reason for hiding this comment

Cajga commented Jul 24, 2020

yogeek commented Feb 3, 2021 • edited Loading

sbose78 commented Apr 1, 2021

omidb commented Mar 25, 2022

roee-landesman commented Apr 8, 2022

tafaust commented Jul 27, 2022 • edited Loading

crenshaw-dev commented Aug 8, 2022

Hronom commented Apr 1, 2023

pre commented Apr 12, 2023 • edited Loading

howardjohn commented Apr 12, 2023

pre commented Apr 13, 2023 • edited Loading

Cajga commented Apr 23, 2023

eightseventhreethree commented May 8, 2023

todaywasawesome left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 2, 2020 •

edited

Loading

Cajga Jul 2, 2020 •

edited

Loading

yogeek commented Feb 3, 2021 •

edited

Loading

tafaust commented Jul 27, 2022 •

edited

Loading

pre commented Apr 12, 2023 •

edited

Loading

pre commented Apr 13, 2023 •

edited

Loading