Introduce cloud.google.com/app-protocols to eventually replace existing annotation #417
Conversation
k8s-ci-robot
added
cncf-cla: yes
pkg/annotations/service.go
Outdated
| val, ok := svc.v[ServiceApplicationProtocolKey] | ||
| var val string | ||
| var ok bool | ||
| // First check the new annotation, then fall back to the old one. |
There was a problem hiding this comment.
Actually, you should check old first and if it exists, use it. That way an old client can "win" in the face of a rollback.
There was a problem hiding this comment.
Maybe I'm missing something, but in a rollback situation, wouldn't the old annotation win always? My thinking is that if we rollback, the controller no longer "knows" about the new annotation and therefore proceeds as it did before, which is to look for the "old" annotation only.
|
/assign @MrHohn |
|
I meant a client rollback.
…On Tue, Jul 31, 2018, 5:40 PM Rohit Ramkumar ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/annotations/service.go
<#417 (comment)>
:
> @@ -105,9 +108,15 @@ func FromService(obj *v1.Service) *Service {
// ApplicationProtocols returns a map of port (name or number) to the protocol
// on the port.
func (svc *Service) ApplicationProtocols() (map[string]AppProtocol, error) {
- val, ok := svc.v[ServiceApplicationProtocolKey]
+ var val string
+ var ok bool
+ // First check the new annotation, then fall back to the old one.
Maybe I'm missing something, but in a rollback situation, wouldn't the old
annotation win always? My thinking is that if we rollback, the controller
no longer "knows" about the new annotation and therefore proceeds as it did
before, which is to look for the "old" annotation only.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#417 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVAtaPF-NiVOcZf5uI1bEB9xXJ6mmks5uMPj3gaJpZM4Vpnay>
.
|
|
@thockin Even if there was a client rollback, I think this code still holds. For example, if a user replaces the alpha annotation w/ the Google-specific one and then rolled back their service, the old annotation would win given that the new one is not present anymore. I think in any case, if both annotations exists and the controller knows that both of them exist, we want the new one to win. |
|
Look at it this way. User has YAML that uses the old annotation. They User changes their YAML and also upgrades to the new annotation, removing the old one, and patch it. If any sort of merge patch can add the new but leave the old, we now have both. This is true for 2 fields (annotations -> real field), but maybe not for annotations. User changes the value of the new annotation in their YAML and applies it. Now we have old and new disagreeing. It doesn't matter how we end up in this situation - you have old and new values that disagree... If we troll back the controller, we have changed the behavior underneath the user. If they roll back their YAML and edit the old annotation, it becomes a silent no-op (again, not sure that can happen for 2 annotations). The general rule is that in such cases, the older has to take precedence. If the user wants the newer, they have to remove the older explicitly. I am not sure we can get into this corner with 2 annotations, but consistent philosophy should apply. |
rramkumar1
force-pushed
the
app-protocols
branch
from
f65018c to
8dc8fda
Compare
|
@thockin Ack. Modified the PR, PTAL. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
For posterity, 2 annotations CAN get into trouble:
```
$ k get svc hostnames -o go-template='{{.metadata.annotations}}{{"\n"}}'
map[old:value1]
$ k patch svc hostnames -p '{"metadata":{"annotations":{"new":"value"}}}'
service/hostnames patched
$ k get svc hostnames -o go-template='{{.metadata.annotations}}{{"\n"}}'
map[new:value old:value1]
$ k patch svc hostnames -p '{"metadata":{"annotations":{"old":"value2"}}}'
service/hostnames patched
$ k get svc hostnames -o go-template='{{.metadata.annotations}}{{"\n"}}'
map[new:value old:value2]
```
Through a series of unfortunate patches, the controller would see both
values, the user can update old and it would never work.
…On Mon, Aug 13, 2018 at 11:00 AM Zihong Zheng ***@***.***> wrote:
Merged #417 into master.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
/assign @bowei