fix(server): appset list uses argocd's namespace instead of all (#15429) #15432

JorTurFer · 2023-09-08T19:51:53Z

As it's explained on #15429, current appset listing implementation is using all namespaces instead of default namespace when the namespace isn't explicitly provided. This PR adds a check following other functions with in the same file to ensure that in case of not explicitly setting the namespace, argocd's namespace is used and not a global query is done (if a global query is the expected behaviour, the RBAC must be update to allow it accordingly).

Checklist:

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.
The title of the PR states what changed and the related issues number (used for the release note).
The title of the PR conforms to the Toolchain Guide
I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
I have signed off all my commits as required by DCO
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My build is green (troubleshooting builds).
My new feature complies with the feature status guidelines.
I have added a brief description of why this PR is necessary and/or what this PR solves.

Fixes #15429

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

jannfis

PTAL at #15429 (comment)

… monitors for apps Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

JorTurFer · 2023-09-13T21:15:11Z

Hi @jannfis
PTAL 🙏
I have replaced the current client with a lister scoped to the same namespaces that the applications. I have reused the same factory indeed, if you think that it's better to create its own factory to allow different namespaces for appsets than the applications, I can update the PR.

codecov · 2023-09-13T21:30:52Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (f8f9ae9) 49.69% compared to head (f8d054d) 49.69%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15432      +/-   ##
==========================================
- Coverage   49.69%   49.69%   -0.01%     
==========================================
  Files         267      267              
  Lines       46362    46363       +1     
==========================================
  Hits        23039    23039              
- Misses      21063    21064       +1     
  Partials     2260     2260

Files	Coverage Δ
server/server.go	`56.18% <100.00%> (-0.05%)`	⬇️
server/applicationset/applicationset.go	`59.91% <90.90%> (+0.34%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jsoref · 2023-09-26T17:33:10Z

This needs documentation and a release notes item.

JorTurFer · 2023-09-26T18:23:04Z

This needs documentation and a release notes item

About the documentation, there isn't any new feature to document, this is a fix for the RBAC issue. I mean, we can add it as a feature, but the idea was to fix the bug in the RBAC for appsets. Could you point me to the correct place to update it? I'll try to do it (if you'd give me an example, it would be nice)

I have followed as well the PR template and I haven't seen anything about the release notes, could you point me to there too?
I have seen that the changelog hasn't been updated for a year, is it the correct place?

jsoref · 2023-09-26T19:26:35Z

Could you point me to the correct place to update it? I'll try to do it (if you'd give me an example, it would be nice)

So, here's the upgrade docs for 2.7 to 2.8: https://argo-cd.readthedocs.io/en/stable/operator-manual/upgrading/2.7-2.8/

As mentioned, this would be a breaking change (as is your fix), so it needs some thought.

A breaking change should be listed in upgrading as a call out to anyone upgrading.

Fwiw, Support dropped for argocd-cm plugins is a breaking change, it doesn't have to use the word breaking.

that content was originally introduced in #12707, although it was since moved...

JorTurFer · 2023-09-26T19:41:27Z

A breaking change should be listed in upgrading as a call out to anyone upgrading.

No no, this isn't a breaking change at all, let me summarize the things:

v2.8 introduced changes in the appsets which require RBAC changes
I opened a PR reverting some changes to avoid the RBAC changes requirement
@jannfis told me that it'd be a breaking change, and we agreed with adding listers instead of kubernetes client
I refactored my PR with those changes, not introducing any breaking change but fixing the RBAC issue

My question about the docs was because as this isn't a breaking change and the UX hasn't changed (because I have just fixed a bug), I don't know where I should document it. The links sent are for migration path, but this isn't something for a migration, it's just a bugfix

jsoref · 2023-09-26T19:45:54Z

If the RBAC behavior is different before/after, surely that's a breaking change -- someone with a certain set of configurations will see different behavior before/after, no? (I'm on slack, and I suspect this'd be easier to discuss there... -- I might need a picture -- sorry)

JorTurFer · 2023-09-26T19:55:44Z

If the RBAC behavior is different before/after, surely that's a breaking change -- someone with a certain set of configurations will see different behavior before/after, no? (I'm on slack, and I suspect this'd be easier to discuss there... -- I might need a picture -- sorry)

Sure, write me (Jorge Turrado on CNCF/Kubernetes workspaces) or give me your handle and I'll do it 😄

server/applicationset/applicationset_test.go

server/applicationset/applicationset.go

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

ishitasequeira

LGTM!!

ishitasequeira

@JorTurFer I approved the PR but we have a failure to the generated code. Can you fix that?

The code LGTM!!

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

JorTurFer · 2023-10-05T12:57:31Z

I approved the PR but we have a failure to the generated code. Can you fix that?

I have just updated the generated code, I didn't notice that it was autogenerated code and I changed it manually 🤦

JorTurFer · 2023-10-05T15:12:41Z

I don't know how to solve the current issue. I updated the manifest with the pending changes, but now it shows other error and I don't know how to solve it. Locally I execute make manifests-local and make codegen-local but there isn't any change in my local

JorTurFer · 2023-10-12T23:31:54Z

The issue has gone automagically just rebasing the branch xd

blakepettersson

LGTM 🚀

ishitasequeira

Thanks @JorTurFer! LGTM!

times are a changing

crenshaw-dev · 2023-11-01T14:56:42Z

/cherry-pick release-2.9

…) (#15432) * fix(server): appset list uses argocd's namespace instead of all Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * use lister to scope the observed namespaces based on which namespaces monitors for apps Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * apply feedback Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * add missing change 🤦 Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * update generated manifests Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> --------- Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

crenshaw-dev · 2023-11-01T14:57:02Z

Let's start by introducing this fix just in 2.9. If it behaves, and if people ask for it, we can pick it further back.

…) (#15432) (#16203) * fix(server): appset list uses argocd's namespace instead of all * use lister to scope the observed namespaces based on which namespaces monitors for apps * apply feedback * add missing change 🤦 * update generated manifests --------- Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> Co-authored-by: Jorge Turrado Ferrero <jorge.turrado@scrm.lidl>

JorTurFer · 2023-11-01T20:42:54Z

Let's start by introducing this fix just in 2.9. If it behaves, and if people ask for it, we can pick it further back.

We are using v2.8 😢
I'll check to upgrade to 2.9 when the fix is released

JorTurFer · 2023-11-02T06:56:00Z

v2.9 hasn't been released yet :(
Could you include the fix in next v2.8?

…proj#15429) (argoproj#15432) * fix(server): appset list uses argocd's namespace instead of all Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * use lister to scope the observed namespaces based on which namespaces monitors for apps Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * apply feedback Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * add missing change 🤦 Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * update generated manifests Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> --------- Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> Signed-off-by: jmilic1 <70441727+jmilic1@users.noreply.github.com>

…proj#15429) (argoproj#15432) * fix(server): appset list uses argocd's namespace instead of all Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * use lister to scope the observed namespaces based on which namespaces monitors for apps Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * apply feedback Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * add missing change 🤦 Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * update generated manifests Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> --------- Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

…proj#15429) (argoproj#15432) * fix(server): appset list uses argocd's namespace instead of all Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * use lister to scope the observed namespaces based on which namespaces monitors for apps Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * apply feedback Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * add missing change 🤦 Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * update generated manifests Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> --------- Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> Signed-off-by: Kevin Lyda <kevin@lyda.ie>

ebuildy · 2024-05-07T19:46:31Z

I suspect this PR for new warning logs:

W0507 19:44:33.469804 7 reflector.go:424] pkg/mod/k8s.io/client-go@v0.26.11/tools/cache/reflector.go:169: failed to list *v1alpha1.ApplicationSet: the server could not find the requested resource (get applicationsets.argoproj.io)
E0507 19:44:33.469828 7 reflector.go:140] pkg/mod/k8s.io/client-go@v0.26.11/tools/cache/reflector.go:169: Failed to watch *v1alpha1.ApplicationSet: failed to list *v1alpha1.ApplicationSet: the server could not find the requested resource (get applicationsets.argoproj.io)

We disable applicationSet, and seeing every minute these messages since we upgrade from 2.8 to 2.10.

JorTurFer · 2024-05-07T19:48:29Z

We disable applicationSet, and seeing every minute these messages since we upgrade from 2.8 to 2.10.

What do you mean with We disable applicationSet? Did you remove the CRD?

ebuildy · 2024-05-07T20:16:17Z

We disable applicationSet, and seeing every minute these messages since we upgrade from 2.8 to 2.10.

What do you mean with We disable applicationSet? Did you remove the CRD?

ApplicationSet is optional, we use official argocd helm chart from:

https://github.com/argoproj/argo-helm/blob/main/charts/argo-cd/templates/crds/crd-applicationset.yaml#L1

Maybe the CRD should be always installed !

JorTurFer · 2024-05-07T20:23:13Z

oh, interesting. I don't think that this PR is the root cause, as this PR changed the global listing with namespace listing, so the problem is previous to this PR. Before this PR the applicationsets were already readed from the cluster

…proj#15429) (argoproj#15432) * fix(server): appset list uses argocd's namespace instead of all Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * use lister to scope the observed namespaces based on which namespaces monitors for apps Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * apply feedback Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * add missing change 🤦 Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * update generated manifests Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> --------- Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

fix(server): appset list uses argocd's namespace instead of all

8d26b64

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

blakepettersson added the ready-for-review label Sep 10, 2023

jannfis previously requested changes Sep 11, 2023

View reviewed changes

blakepettersson removed the ready-for-review label Sep 13, 2023

use lister to scope the observed namespaces based on which namespaces…

d1663b5

… monitors for apps Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

Merge branch 'master' into fix-appset-list

acb43ec

JorTurFer requested a review from a team as a code owner September 26, 2023 06:51

blakepettersson added the ready-for-review label Sep 26, 2023

crenshaw-dev previously requested changes Oct 2, 2023

View reviewed changes

server/applicationset/applicationset_test.go Outdated Show resolved Hide resolved

server/applicationset/applicationset.go Outdated Show resolved Hide resolved

JorTurFer added 2 commits October 2, 2023 17:09

apply feedback

af2c211

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

add missing change 🤦

dd17dec

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

ishitasequeira approved these changes Oct 5, 2023

View reviewed changes

ishitasequeira requested changes Oct 5, 2023

View reviewed changes

update generated manifests

b653a23

Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>

Merge branch 'master' into fix-appset-list

56c3354

Merge branch 'master' into fix-appset-list

f8d054d

blakepettersson approved these changes Oct 28, 2023

View reviewed changes

ishitasequeira approved these changes Nov 1, 2023

View reviewed changes

ishitasequeira enabled auto-merge (squash) November 1, 2023 02:11

ishitasequeira requested review from jannfis and crenshaw-dev November 1, 2023 02:12

ishitasequeira merged commit c70e1b7 into argoproj:master Nov 1, 2023
22 checks passed

gcp-cherry-pick-bot bot mentioned this pull request Nov 1, 2023

fix(server): appset list uses argocd's namespace instead of all (#15429) (cherry-pick #15432) #16203

Merged

JorTurFer deleted the fix-appset-list branch November 1, 2023 20:31

ebuildy mentioned this pull request May 7, 2024

feat(argo-cd): Always install applicationset crd argoproj/argo-helm#2682

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(server): appset list uses argocd's namespace instead of all (#15429) #15432

fix(server): appset list uses argocd's namespace instead of all (#15429) #15432

JorTurFer commented Sep 8, 2023

jannfis left a comment

JorTurFer commented Sep 13, 2023

codecov bot commented Sep 13, 2023 •

edited

Loading

jsoref commented Sep 26, 2023

JorTurFer commented Sep 26, 2023 •

edited

Loading

jsoref commented Sep 26, 2023

JorTurFer commented Sep 26, 2023 •

edited

Loading

jsoref commented Sep 26, 2023

JorTurFer commented Sep 26, 2023

ishitasequeira left a comment

ishitasequeira left a comment •

edited

Loading

JorTurFer commented Oct 5, 2023 •

edited

Loading

JorTurFer commented Oct 5, 2023

JorTurFer commented Oct 12, 2023

blakepettersson left a comment

ishitasequeira left a comment

crenshaw-dev commented Nov 1, 2023

crenshaw-dev commented Nov 1, 2023

JorTurFer commented Nov 1, 2023

JorTurFer commented Nov 2, 2023

ebuildy commented May 7, 2024

JorTurFer commented May 7, 2024

ebuildy commented May 7, 2024

JorTurFer commented May 7, 2024

fix(server): appset list uses argocd's namespace instead of all (#15429) #15432

fix(server): appset list uses argocd's namespace instead of all (#15429) #15432

Conversation

JorTurFer commented Sep 8, 2023

jannfis left a comment

Choose a reason for hiding this comment

JorTurFer commented Sep 13, 2023

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

jsoref commented Sep 26, 2023

JorTurFer commented Sep 26, 2023 • edited Loading

jsoref commented Sep 26, 2023

JorTurFer commented Sep 26, 2023 • edited Loading

jsoref commented Sep 26, 2023

JorTurFer commented Sep 26, 2023

ishitasequeira left a comment

Choose a reason for hiding this comment

ishitasequeira left a comment • edited Loading

Choose a reason for hiding this comment

JorTurFer commented Oct 5, 2023 • edited Loading

JorTurFer commented Oct 5, 2023

JorTurFer commented Oct 12, 2023

blakepettersson left a comment

Choose a reason for hiding this comment

ishitasequeira left a comment

Choose a reason for hiding this comment

crenshaw-dev commented Nov 1, 2023

crenshaw-dev commented Nov 1, 2023

JorTurFer commented Nov 1, 2023

JorTurFer commented Nov 2, 2023

ebuildy commented May 7, 2024

JorTurFer commented May 7, 2024

ebuildy commented May 7, 2024

JorTurFer commented May 7, 2024

codecov bot commented Sep 13, 2023 •

edited

Loading

JorTurFer commented Sep 26, 2023 •

edited

Loading

JorTurFer commented Sep 26, 2023 •

edited

Loading

ishitasequeira left a comment •

edited

Loading

JorTurFer commented Oct 5, 2023 •

edited

Loading