Skip to content

Derive dedicated Dex deployment#564

Merged
merenbach merged 22 commits intoargoproj:masterfrom
merenbach:555-dedicated-dex-deployment
Sep 15, 2018
Merged

Derive dedicated Dex deployment#564
merenbach merged 22 commits intoargoproj:masterfrom
merenbach:555-dedicated-dex-deployment

Conversation

@merenbach
Copy link
Contributor

@merenbach merenbach commented Sep 7, 2018

Dutifully closes #555.

Help wanted: how to test properly? I can see that the local Dex container gets created.

Testing in progress.

Also updates pseudo-random token generation for SSO to be more cryptographically secure.

@merenbach merenbach requested a review from jessesuen September 7, 2018 22:18
@merenbach merenbach added the help-wanted Extra attention is needed label Sep 7, 2018
@jessesuen
Copy link
Member

Help wanted: how to test properly? I can see that the local Dex container gets created.

Go through the steps of configure SSO for your minikube environment:
https://github.com/argoproj/argo-cd/blob/master/docs/sso.md

alexmt
alexmt previously requested changes Sep 10, 2018
Copy link
Collaborator

Choose a reason for hiding this comment

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

dex don't talk to kubernetes, so we can remove serviceAccountName.

Copy link
Collaborator

Choose a reason for hiding this comment

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

please disregard this comment. we need to launch /shared/argocd-util which reads configmaps, so we need separate service account, role and binding.

Copy link
Collaborator

Choose a reason for hiding this comment

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

copyutil init container is not needed for dex

Copy link
Collaborator

Choose a reason for hiding this comment

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

please disregard this comment. we need both volume and init container

Copy link
Collaborator

Choose a reason for hiding this comment

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

volume is not required for dex

Copy link
Collaborator

Choose a reason for hiding this comment

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

please disregard this comment. we need both volume and init container

@jessesuen
Copy link
Member

A code change is needed to update the reverse proxy to point to the new dex hostname. Which means you need a service object for dex (in addition to the deployment).

@merenbach merenbach removed the help-wanted Extra attention is needed label Sep 13, 2018
@merenbach merenbach changed the title [WIP] Derive dedicated Dex deployment Derive dedicated Dex deployment Sep 13, 2018
@merenbach merenbach changed the title Derive dedicated Dex deployment [WIP] Derive dedicated Dex deployment Sep 13, 2018
@merenbach merenbach changed the title [WIP] Derive dedicated Dex deployment Derive dedicated Dex deployment Sep 14, 2018
@merenbach
Copy link
Contributor Author

@jessesuen decided it would be over-engineering to bring in my other code. :p Just rewrote the token generation instead.

@merenbach merenbach dismissed alexmt’s stale review September 14, 2018 00:31

Clarified concerns

@alexmt
Copy link
Collaborator

alexmt commented Sep 14, 2018

LGTM

Copy link
Member

Choose a reason for hiding this comment

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

We do not want this role to read all secrets and configmaps in the namespace just the settings one. We should add:

resourceNames:
- argocd-cm
- argocd-secret

@merenbach
Copy link
Contributor Author

merenbach commented Sep 14, 2018

For posterity, here's testing instructions:

  1. Modify the Argo CD server service (manifests/components/04e_argocd-server-service.yaml) to set spec.type: LoadBalancer.
  2. Modify the Argo CD config map (manifests/components/02a_argocd-cm.yaml) to include the desired SSO config.
  3. Update the callback address on your GitHub app (if using GitHub) to be https://<your IP address>/api/dex/callback.
  4. Modify the following script (has a DESTRUCTIVE effect on your existing running local services/deployments, even if they're not related, so those lines are commented out) and run it. Please read through carefully before use.
#! /bin/sh -x

build_containers() {
	make all
}

push_containers() {
	USERNAME=$1
	for CONTAINER in argocd-server argocd-application-controller argocd-repo-server
	do
		docker tag "${CONTAINER}" "${USERNAME}/${CONTAINER}"
		docker push "${USERNAME}/${CONTAINER}"
	done
}

apply_local_manifest() {
	cat manifests/components/*.yaml | sed "s/argoproj\/\([^:]*\):.*$/andrewdm\/\1:latest/g" | tee manifests/local-install.yaml | kubectl -n argocd apply -f -
}

# comment the following two lines out to save lots of time if you're just tinkering with manifest changes
build_containers
push_containers <your DockerHub username>

# Required to run script properly, but DESTRUCTIVE, so commented out so you don't run verbatim
#kubectl -n argocd delete services --all
#kubectl -n argocd delete deployments --all

sleep 10

apply_local_manifest

# adapt as needed to `en1` or any other relevant network device name
PUB_URL=https://`ipconfig getifaddr en0`
kubectl -n argocd get configmap/argocd-cm -o json | jq --arg puburl "${PUB_URL}" '.data.url=$puburl' | kubectl apply -f -

sleep 20

kubectl -n argocd get pods,deployments,services

@merenbach merenbach merged commit 4699946 into argoproj:master Sep 15, 2018
@merenbach merenbach deleted the 555-dedicated-dex-deployment branch September 15, 2018 00:08
leoluz added a commit to leoluz/argo-cd that referenced this pull request Mar 13, 2025
…idation even with server side apply (argoproj#564)

* Revert "feat: retry with client side dry run if server one was failed (argoproj#548)"

This reverts commit c0c2dd1.

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Revert "fix(server): use server side dry run in case if it is server side apply (argoproj#546)"

This reverts commit 4a5648e.

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Fixed the logic to disable server side apply if it is a dry run

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Added more values in the log message for better debugging

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Fixed compilation error

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Written an inline fn to get string value of dry-run strategy

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Added comment as requested with reference to the issue number

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

---------

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>
Co-authored-by: Leonardo Luz Almeida <leoluz@users.noreply.github.com>
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.

Split out dex into it's own deployment (instead of sidecar)

3 participants