MAISTRA-1682 add support for v2 profiles #519
Conversation
|
I needed to regenerate the CRDs, so I rolled them back to v1beta1 in this PR. |
Signed-off-by: rcernich <rcernich@redhat.com>
rcernich
force-pushed
the
MAISTRA-1682-1575
branch
from
dfd0674
to
6548fee
Compare
|
I've verified that |
rcernich
force-pushed
the
MAISTRA-1682-1575
branch
from
6548fee
to
f6e9854
Compare
rcernich
changed the title
rcernich
force-pushed
the
MAISTRA-1682-1575
branch
from
f6e9854
to
92711fb
Compare
Signed-off-by: rcernich <rcernich@redhat.com>
Signed-off-by: rcernich <rcernich@redhat.com>
rcernich
force-pushed
the
MAISTRA-1682-1575
branch
from
92711fb
to
3eb5715
Compare
| # note, mixer runtime is configured as part of policy.mixer or telemetry.mixer | ||
| pilot: # same as gateway.runtime structure illustrated above | ||
| deployment: | ||
| replicas: 2 |
There was a problem hiding this comment.
is this better to have in the yaml files?
There was a problem hiding this comment.
this is just an example file embedded with the struct definition, as it's difficult to get an idea of the structure by looking at the go code (since it's all spread around between files).
also, i think we've decided that we want the profiles to be the default, so we're not relying on values.yaml. that said, i don't think we'll be there for a while. also, it means the user will have to have a full config if they're not using the default profile, which means we may want to always apply a default.
There was a problem hiding this comment.
the default profile should be used as the base for everything shouldn't it? If you don't specify a profile, use default. If you specify a profile, have it inherit at some level from default? I don't think that should be avoidable.
| var template v1.ServiceMeshControlPlane | ||
| if err = yaml.Unmarshal(templateContent, &template); err != nil { | ||
| return v1.ControlPlaneSpec{}, fmt.Errorf("failed to parse template %s contents: %s", name, err) | ||
| obj, gvk, err := decoder.Decode(profileContent, nil, nil) |
There was a problem hiding this comment.
Is there any advantage to using this over the ghodds? Ghodss is simpler, and doesn't require the additional weight of using reflection.
Looking at kubernetes/kubernetes#74378, it looks like Kubernetes forked ghodss, but it's unclear why.
I'd be curious if making this change slows us down/what the cost/benefit of the additional complexity is.
There was a problem hiding this comment.
It looks like they may have switched to a fork to try to support YAML 1.2, which it looks like they may be falling back on. YAML 1.2 introduces breaking changes in handling of booleans, and some other changes. Not sure how that would affect those writing templates. kubernetes/kubernetes#34146
There was a problem hiding this comment.
No, because we don't know which version of SMCP is being loaded. We need to pass an object to the marshaller and we don't know which one to pass (v1 or v2).
There was a problem hiding this comment.
Ah ok. Other option here would be:
v2 := &v1.SMCP
if Unmarshal(v2) != error ...
else unmarshal(v1) != error ...
else fail
Probably faster just to take the hit for reflection at that point though.
|
|
||
| if visited.Has(smcp.Template) { | ||
| return smcp, fmt.Errorf("SMCP templates form cyclic dependency. Cannot proceed") |
There was a problem hiding this comment.
I don't think this should be removed. It's really low hanging validation and the presence of circular dependencies means the user configured something improperly. Ignoring it it means that the user will not be achieving the behavior that they specified. We should find a way to surface this as a warning to the user before they encounter unrelated issues.
There was a problem hiding this comment.
Yeah, I guess I disagree. They are getting the behavior they specified. They just specified something that could break the operator (because we'd continually recurse). The other problem is that because profiles is now a list, it's possible that a profile could be specified multiple times, without actually being circular. All this does is break the chain, returning a result. If the user doesn't get the result they wanted, then the configuration error is their problem, which isnot really any different than if they had specified bogus stuff in a single profile, that wasn't part of a circular chain.
I think the ultimate solution is to incorporate the profiles during the validation phase, so that we reject any invalid smcp before it gets to reconcile. We should also add the resulting resource to the status, so users can inspect what they've got. Adding a history would be nice too, but a better option might be to have some sort of tool that users can test with, or have some sort of dry-run capabilities. Feel free to open a jira.
There was a problem hiding this comment.
yeah. Adding the resulting resource to the status does allow our users to go back when they start seeing problems and see what the result after parsing is. And adding the SMCP templates to validation would also help.
|
Yeah, the default stuff is not generated/supported for v1beta1. I left it commented out for when we move back to v1. That said, I think @brian-avery, was having problems building the OLM index, so I think there's some additional cleanup required wrt the generated crds. (I went from using operator-sdk to using controller-tools directly. operator-sdk does not expose all the flags available for crd generation, specifically, MaxDescLen.) |
Also includes
MAISTRA-1575 refine SMCP v2 structurewhich includes changes to make working with the new structure easier (based on updating the profiles to v2).