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

Fix that new host name isn't applied to ingress #699

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mosuke5
Copy link

@mosuke5 mosuke5 commented Jun 28, 2022

What type of PR is this?
/kind bug

What does this PR do / why we need it:
Fixed a problem where changes to .spec.server.host in the ArgoCD CR were not reflected in the Ingress resource by changing the order of processing in reconcileArgoServerIngress().

Have you updated the necessary documentation?

  • Documentation update is required by this PR.
  • Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #558

How to test changes / Special notes to the reviewer:
I have added a test scenario in ingress_test.go. You can be verified by a test run.
If you want to check manually, you can do so as follows.

$ cat <<EOF | kubectl apply -f - 
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
  name: argocd-sample
spec:
  server:
    host: before.example.com
    ingress:
      enabled: true
EOF
argocd.argoproj.io/argocd-sample configured

$ kubectl get ingress argocd-sample-server -o yaml
...
spec:
  rules:
  - host: before.example.com
    http:
      paths:
      - backend:
          service:
            name: argocd-sample-server
            port:
              name: http
        path: /
        pathType: ImplementationSpecific
  tls:
  - hosts:
    - before.example.com
    secretName: argocd-secret

$ cat <<EOF | kubectl apply -f - 
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
  name: argocd-sample
spec:
  server:
    host: after.example.com
    ingress:
      enabled: true
EOF
argocd.argoproj.io/argocd-sample configured

$ kubectl get ingress argocd-sample-server -o yaml
...
spec:
  rules:
  - host: after.example.com
    http:
      paths:
      - backend:
          service:
            name: argocd-sample-server
            port:
              name: http
        path: /
        pathType: ImplementationSpecific
  tls:
  - hosts:
    - after.example.com
    secretName: argocd-secret

@CLAassistant
Copy link

CLAassistant commented Jun 28, 2022

CLA assistant check
All committers have signed the CLA.

if !found {
return r.Client.Create(context.TODO(), ingress)
}
return r.Client.Update(context.TODO(), ingress)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @mosuke5 Thanks for your PR!
Can we add a check here to see if anything has changed before calling Update? Otherwise we might have a lot of unnecessary api calls being made

Copy link
Author

Choose a reason for hiding this comment

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

@jaideepr97 Thank you for your review. I fixed to update only when annotations and spec of ingress resource are changed.

@simonfelding
Copy link

bump. this also applies to HTTPRoutes from the Kubernetes Gateway API

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.

Changing host does not update ingress
4 participants