Fix when context variable isn't used for metadata #321
Conversation
| // If the name doesn't match the original, return an error | ||
| if needsPerNameTemplating && desiredObj.GetName() != name { | ||
| // Error if the name doesn't match the parsed name from the objectSelector | ||
| if objectSelector != nil && desiredObj.GetName() != name { |
There was a problem hiding this comment.
I reconsidered this block a lot and even considered removing it, not wanting to unnecessarily throttle the capabilities of policy users. I think the logic here makes sense now. My initial concern was the ability to set the name or namespace to something other than what was determined by the namespaceSelector or objectSelector, which seemed like somewhat of an antipattern. But I could be swayed if the logic seems off or there are cases I'm not considering where setting the name or namespace to something else could be beneficial somehow.
There was a problem hiding this comment.
I'm concerned that if I create a policy that uses object selector, and at first it only selects one object, I can use that name in the policy. But if later another object is created that matches the selector, suddenly by policy will have this error? Is that correct?
If that's the current behavior (I could've misunderstood), I think it might be better if we always report an error if the policy defines both the name and the object selector. Does that make sense?
I was also worried a little bit about the error message, since it doesn't give the details of what the "selected name" is - if the error was just something like name may not be set when object selector is used, it might be more clear to the user how to fix it.
There was a problem hiding this comment.
@JustinKuli GOOD Point!!
There was a problem hiding this comment.
@JustinKuli Hmmm. The differing error behavior is a good point. We could forbid users from using templated strings for the name or namespace if the objectSelector or namespaceSelector are in use, since that's really the complexity here. (Providing a non-templated string overrides the selector as it does with the namespaceSelector.) Does that align with your point? It'd seem to align with your point about the error message also.
There was a problem hiding this comment.
Providing a non-templated string overrides the selector as it does with the namespaceSelector.
I hadn't remembered that detail. It makes sense for namespace, since I think I've been guilty of just copying a default namespaceSelector into new policies, and still setting the namespace in a template to whatever I want. And then for consistency, it makes sense to do the same thing for object selector and names... I have a feeling if we were building this from scratch I might try to make slightly different decisions, but it's too late now.
I don't think I'd want us to forbid templated strings entirely here, since name: {{.ObjectName}} "feels right". But name: {{.ObjectName}}-new definitely feels wrong. So the conditional statement that is currently there seems right. And now, after talking about it for a while, I finally understand what the error message is trying to say. But I don't have a great way to reword it to be more clear immediately.
There was a problem hiding this comment.
I've updated the message and also added an additional conditional in the namespace selector block since I feel like there are so many edge cases that have come up during development I wanted to try to make the conditional clearer/more explicit just in case.
There was a problem hiding this comment.
I was kind of referring to the fact that setting the namespace can override the namespace selector. Something like
namespaceSelector:
exclude: "prod-*"
...
objectDefinition:
metadata:
namespace: prod-stuffcould/should be a user-error that gets reported. But it would be a breaking change to do that at this point.
There was a problem hiding this comment.
good point!! @JustinKuli finding edge cases!!!
There was a problem hiding this comment.
@JustinKuli The objectSelector is new, so we could update its behavior to disallow it specifying both... 😅
There was a problem hiding this comment.
We could, but I'd rather not, personally.
dhaiducek
force-pushed
the
fix-name-setting
branch
from
41eeef8 to
bb77d82
Compare
|
LGTM but hold for Justin |
dhaiducek
force-pushed
the
fix-name-setting
branch
from
bb77d82 to
ffd8e2c
Compare
| // If the name doesn't match the original, return an error | ||
| if needsPerNameTemplating && desiredObj.GetName() != name { | ||
| // Error if the name doesn't match the parsed name from the objectSelector | ||
| if objectSelector != nil && desiredObj.GetName() != name { |
There was a problem hiding this comment.
I was kind of referring to the fact that setting the namespace can override the namespace selector. Something like
namespaceSelector:
exclude: "prod-*"
...
objectDefinition:
metadata:
namespace: prod-stuffcould/should be a user-error that gets reported. But it would be a breaking change to do that at this point.
When the `.ObjectName` template variable was used outside `.metadata.name`, the controller complained that the name didn't match. This also removes an errant E2E test. Signed-off-by: Dale Haiducek <[email protected]>
dhaiducek
force-pushed
the
fix-name-setting
branch
from
ffd8e2c to
ff428bd
Compare
|
Unrelated E2E failure: |
|
/unhold CI is passing--ready for another review! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhaiducek, JustinKuli 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 |
openshift-merge-bot
bot
merged commit dcd8d0e
into
open-cluster-management-io:main
When the
.ObjectNametemplate variable was used outside.metadata.name, the controller complained that the name didn't match.This also removes an errant E2E test that was testing that a template that resolved to an empty string when the
objectSelectorwas in play was NonCompliant, but really an empty string would be expected there regardless so that the controller could fill in the value.