bilcus
commented
1 year ago We are having the same issue when upgrading Ambassador from 3.3.1 to 3.4.0.
Open Andrei-Predoiu opened 1 year ago
bilcus
commented
1 year ago We are having the same issue when upgrading Ambassador from 3.3.1 to 3.4.0.
bluehawk27
commented
1 year ago I updated my service annotations to ambassador/v2 and that got it working for me. Ideally this doesn't even cause a shutdown. It would be a shame for a single misconfigured application to bring down all of the services that depend on ambassador as an ingress controller.
lorenzo-biava
commented
1 year ago Given all the issues the v1 to v2 upgrade brought, it's a bit of a shame there are no regression tests in the codebase for misconfigured resources that could still use v1 APIs. Especially annotations, it's very hard to reject those faulty objects when they are deployed to the cluster. And if you run a multi-tenant environment, you're absolutely sure to incur in this issue.
pie-r
commented
1 year ago If you need to identify the problematic resources:
k get svc -A -o yaml > svc-list.yaml && \
yq '.items[].metadata | select(.annotations."getambassador.io/config" | test(".*(getambassador.io|ambassador.io|ambassador)/v1.*") ) | [{"name": .name, "creationTimestamp": .creationTimestamp, "namespace": .namespace, "annotationsWithBadContent": .annotations."getambassador.io/config" }]' svc-list.yaml > bad_service.yaml \
| tee bad_service.yaml 
cindymullins-dw
commented
1 year ago We try to highlight the need to identify and upgrade v1 resources beforehand in the Migration Steps but I see your point about more graceful handling in the event that they still exist.
akundrock
commented
1 year ago I had this issue today. The docs for upgrading via helm charts have no link to the precautions available in the yaml upgrade path
dgaffuri
commented
1 year ago Hi, as stated before the same happens with 3.4.0, but in change log for 3.4.0 I read "Support for the getambassador.io/v1 apiVersion has been re-introduced". This means that custom resources in that namespace are accepted (I've not tried to be honest) but annotations with "ambassador/v1" cause emissary-ingress to panic?
dgaffuri
commented
1 year ago The problem is due to the following private field, present in all resources defined in pkg/api/getambassador.io/v1 package, that breaks conversion of annotations by reflection in pkg/snapshot/v1.convertAnnotationObject (annotations.go). There's a test for conversion (annotations_test.go) but no test case involve a v1 (or v2) namespace.
// dumbWorkaround is a dumb workaround for a bug in conversion-gen that it doesn't pay
// attention to +k8s:conversion-fn=drop or +k8s:conversion-gen=false when checking if it can
// do the direct-assignment or direct-conversion optimizations, and therefore might disobey
// the +k8s:conversion-fn=drop on metav1.TypeMeta.
//
// +k8s:conversion-gen=false
dumbWorkaround byte `json:"-"` //nolint:unused // dumb workaroundSince I've not well understood the purpose of the workaround I'm not able to propose a patch.
dgaffuri
commented
1 year ago @LukeShu Hi, I've seen you're the author of the commit that reintroduced v1 resources (b8c6a24), do you think there's some different solution that don't break v1 annotations? Our problem is that that we cannot update a non single namespaced instance because of old annotations in the cluster used by 1.x deployments not under our control. Thank you very much for your attention
Describe the bug Emissary 2.5.1 crashes if there are deprecated mappings in the cluster
To Reproduce Steps to reproduce the behavior:
getambassador.io/v1) fx:Expected behavior A clear error is displayed on the ambassador diag page or the deprecated mappings are ignored.
Versions (please complete the following information):
Additional context Example mapping: