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

[operator] Add degraded condition when gateway is enabled and tenants spec is nil #5383

Merged
merged 3 commits into from
Feb 15, 2022

Conversation

sasagarw
Copy link

@sasagarw sasagarw commented Feb 14, 2022

What this PR does / why we need it:
Fixes the issue of lokistack-gateway deployment when tenant's spec is missing from lokistack CR

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

@xperimental
Copy link
Collaborator

The configuration "enable gateway but have no tenant configuration" seems like a misconfiguration that will produce no working solution. For me, this would be equivalent to defining authentication but then having no allowed users, so I think it should still produce a gateway configuration.

I think it would be better to update the templates so that they can cope with having a nil Tenant instead of not generating a gateway deployment at all. Maybe have the operator emit a warning that it has detected a potentially invalid configuration.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 14, 2022
@sasagarw
Copy link
Author

Thanks @xperimental for the clarity. I updated the PR with the same. This looks more relatable.

@sasagarw sasagarw changed the title [operator] Deploy gateway when both flag + tenants spec provided [operator] Deploy gateway in degraded condition when tenants spec is nil Feb 14, 2022
@xperimental
Copy link
Collaborator

The added condition provides good feedback for the error condition but it still stops all processing of the operator. I still think it would be better to have the operator continue working for the non-gateway parts even if the error is there.
Is it possible to emit the condition without erroring out of the processing? What do you think would provide the better usability?

@sasagarw
Copy link
Author

@xperimental that would be possible to do but we will be deviating from our focused goals. If a customer wants to move forward then they should disable the flag and then continue. If we just give warning about gateway then we are not giving justice to the implementation of feature flags. I think we should run the operator in degraded condition till the customer takes necessary steps.

@xperimental
Copy link
Collaborator

I have tried it out on a test-cluster now. This approach is consistent with the other error conditions in that area, so let's error out and let the user sort out his configuration. 👍

@periklis periklis self-assigned this Feb 15, 2022
@periklis periklis removed the request for review from a team February 15, 2022 09:36
Copy link
Collaborator

@xperimental xperimental left a comment

Choose a reason for hiding this comment

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

Sorry, had not given this enough thought yesterday.

operator/internal/handlers/lokistack_create_or_update.go Outdated Show resolved Hide resolved
@sasagarw
Copy link
Author

@xperimental I guess now it works as expected? Have put the degraded condition as well as it will not return any error. Also, as soon as the user put the correct configuration in the CR, it will retrigger the reconcilation immediately.

@xperimental xperimental changed the title [operator] Deploy gateway in degraded condition when tenants spec is nil [operator] Add degraded condition when gateway is enabled and tenants spec is nil Feb 15, 2022
@periklis periklis merged commit b8e9973 into grafana:main Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants