-
Notifications
You must be signed in to change notification settings - Fork 0
feat: update namespace validation logic #281
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
base: main
Are you sure you want to change the base?
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.
Auto-approved because this is so small 🙈
| namespace := obj.GetNamespace() | ||
| if namespace == "" { | ||
| namespace = v.namespace | ||
| obj.SetNamespace(namespace) | ||
| } |
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.
this would allow eso-server to manage resources on different namespaces, right? Do we want that? 🤔
If we do, we also need to change the sync controllers - they are all focused on a single namespace.
As is, this means we are not ever going to update these resources if there is a change on the cluster side.
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 don't think so. ValidateManifest still only writes to the validator namespace, so in main.go that's KUBERNETES_NAMESPCACE, which defaults to default only if the env is empty.
I had to change this because any manifest that skipped metadata.namespace got forced into default regardless of the controller namespace. the sync controllers already watch and reconcile only their configured namespace so they'd never see cluster-side updates.
So this basically fix the missing namespaces to the controller's namespace.
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.
if namespace != nil wouldn't this return obj.GetNamespace() instead?
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 mean, I thought we were already doing it with obj.SetNamespace(v.namespace) for any objects (with namespaces defined or not/) 🤔 was the controller set up with a valid KUBERNETES_NAMESPACE env var? 🤔
I literally use this approach on demos and it works (manifests are forced to the correct namespace the controller is set up with) 🤔
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.
Hmm... I'm probably confused as I had to go through a lot of stuff to make it work. Let me check.
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.
Is it possible your demos always ran the controller in the default namespace? Here’s what I see when we run anywhere else:
• the scan job produces a Consumer manifest with no metadata.namespace
• without the fix, the validator hard-codes that to default, so the CR lands in the wrong namespace
• the sync controller only watches the namespace ESO is deployed in (eso-server in test setup), so it never sees those CRs and the test timeouts
No description provided.