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

ClusterExternalSecret Ready Condition Should Only be Set to False if There is an Error #3029

Closed
iNoahNothing opened this issue Jan 16, 2024 · 5 comments · Fixed by #3077
Closed
Assignees
Labels
good first issue Good for newcomers kind/feature Categorizes issue or PR as related to a new feature.

Comments

@iNoahNothing
Copy link

Is your feature request related to a problem? Please describe.
Currently, since #2582 has been merged. ClusterExternalSecrets never report ready until the controller has created an ExternalSecret in a targeted namespace. I get the rationale behind wanting to make it easier to indicate that the ClusterExternalSecret is not managing any ExternalSecrets since this could be an indication of a misconfiguration of some kind but the implementation of not setting the Ready condition to True until a namespace matches properly fails to recognize there are valid usecases for the ClusterExternalSecret to be sitting idle and waiting for a namespace to request an ExternalSecret be created.

An example of such use case is to have a cluster operator provision a ClusterExternalSecret that can then be used by developer teams to create ExternalSecrets in namespaces that they need access to the secret data. Until a developer provisions a namespace that has this requirement, this ClusterExternalSecret Ready condition should be True and waiting for a namespace to request an ExternalSecret be provisioned. This causes issues with Argo CD Since the ClusterExternalSecret is stuck with Ready = False, Argo never progresses the application to be ready and it is stuck perpetually pending until a namespace is created that matches the selector. This can have large downstream effects if you have automation in place that checks the status of argo applications to take some action.

Screenshot 2024-01-16 at 11 10 37

Describe the solution you'd like
The status should report that the ClusterExternalSecret Ready condition is True even if there are no namespaces that match the selector. This is in line with what a controller would indicate Ready to mean which is "This resource is ready to be used". The Provisioned Namespaces element of the status seems to be a more appropriate place to indicate that no namespaces have provisioned the external secret and another condition should be added if the ClusterExternalSecret controller tried to create an ExternalSecret in the namespace but was unable to. Using the Ready condition for this is causes issues when there are none present.

Describe alternatives you've considered
The best workaround right now is the label the namespace that ESO is deployed in so that it matches the ClusterExternalSecret condition. This is fine but isn't really how this controller should be used as this namespace has no need for the data being grabbed by the ExternalSecret that get provisioned.

@iNoahNothing iNoahNothing added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 16, 2024
@moolen moolen added the good first issue Good for newcomers label Jan 16, 2024
@moolen
Copy link
Member

moolen commented Jan 16, 2024

Thank you @iNoahNothing for writing up this detailed issue and to provide all the necessary context!

The request sounds reasonable to me, happy if someone from the community can take this :)

@ppatel1604
Copy link
Contributor

@moolen I am happy to work on this. Can you please assign it to me if it is available to take?

@ppatel1604
Copy link
Contributor

@moolen Can you please review the Draft PR #3077? I am not 100% sure if I am on correct track. Can you please provide some guidance on it?
I have moved the initialisation of the condition at bit early stage. I am setting the condition just after the check if we have any failed namespace and if we don't have any failed namespace then set the condition as Ready then continue the process again and set the condition one more time after the external secret is created. I have also update the util function to check only if there is any failed condition instead of checking the item of the namespace and failed namespace.

@shuheiktgw
Copy link
Contributor

Thank you for reporting the issue, @iNoahNothing! Hmm, I see. At that time, I didn't notice the use case but this makes sense to me. Users can set a monitor for the size of provisioned namespaces.

@shuheiktgw
Copy link
Contributor

@ppatel1604 I've left some comments on the PR so PTAL 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants