-
Notifications
You must be signed in to change notification settings - Fork 802
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
Bootstrap the new clusters in k8s-infra-prow project. #7127
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the initial review, it looks great overall, but I have some questions
kubernetes/apps/argocd.yaml
Outdated
project: default | ||
source: | ||
path: kubernetes/gke-utility/argocd | ||
repoURL: https://github.com/borg-land/k8s.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we have to get this PR merged, then update this to k/k8s.io, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will the final commit on this PR.
namespace: argocd | ||
|
||
resources: | ||
- github.com/argoproj/argo-cd/manifests/ha/cluster-install?ref=v2.11.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should keep the source manifests committed to this repo. That way we're sure we're always going to have access to the manifest we deployed. Also, we can easier track what's changed in the manifest and if there's anything that we need to pay attention to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another comment: v2.11.2 is not too old, but v2.12.0 has just been released.
labels: | ||
argocd.argoproj.io/secret-type: repository | ||
stringData: | ||
url: https://github.com/kubernetes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the ArgoCD docs, it appears that this should be the full repository URL, not the organization URL. How do we exactly use this Secret?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at the third example called private-repo-creds and the paragraph below it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I fully understand why do we need this, because the document is describing this in the context of private repositories, but all repositories in k-org that we use are public. Does this repository configure Argo to connect to k-org without authentication or there's something else to it?
targetRevision: utility-dev | ||
syncPolicy: | ||
automated: | ||
prune: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see prune: false
used in a couple of places, but we have to be careful with it because it can leak resources and folks might not expect that. I think we should have a note in the README file describing that some resources are not pruned and that it must be done manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArgoCD will be the one service where pruning is disabled. All other deployments will be automated with pruning enabled.
data: | ||
policy.default: role:readonly | ||
policy.csv: | | ||
g, kubernetes:sig-k8s-infra-leads, role:admin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think about having a way to grant other folks access to Argo, e.g. @koksay and I will need it once we extend it to the EKS cluster.
spec: | ||
goTemplate: true | ||
generators: | ||
# targets all clusters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are clusters defined? I see that we have the gke-prow
cluster defined, but I don't see the gke-utility
cluster defined and labeled. Is it done manually/via click-ops?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gke-utility is the default cluster. it was missing labels so I renamed it in the Argo Console and added the labels you see on gke-prow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks for the clarification! I think we should document this as part of the README.
- name: serviceAccount.name | ||
value: external-secrets | ||
valueFiles: | ||
- $values/kubernetes/{{ .name }}/helm/external-secrets.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this $values
resolve? Is it resolving to the source defined below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's resolved from the values defined below.
kind: ConfigMap | ||
metadata: | ||
name: argocd-cm | ||
data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include some comments what are different options doing and why did we enable/disable these options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: upodroid, xmudrii The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I have been working on this for a while but I will have this shipped this week.
This has already been deployed but I'll be adding a few things incrementally.