Skip to content
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

Remove org field and make req ctx fields optional #1831

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

teodor-yanev
Copy link
Contributor

@teodor-yanev teodor-yanev commented Dec 6, 2023

Fixes: #1743
Fixes: #1744

@teodor-yanev teodor-yanev self-assigned this Dec 6, 2023
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! There are a few more places where the organization appears, I can think of UserPermissions and RoleInfo in jwtauth.go

evankanderson
evankanderson previously approved these changes Dec 6, 2023
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple proto compatibility notes. Note that optional string and string are just different ways of handling the value in Go code, and don't (much) affect the actual on-wire representation.

The only difference is that with optional string, it's possible to request explicitly sending "" as a value, whereas that value with string will cause the field not to be sent at all.

@@ -1004,8 +996,7 @@ message Provider {
// Context defines the context in which a rule is evaluated.
// this normally refers to a combination of the provider, organization and project.
message Context {
string provider = 1;
optional string organization = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename this to retired_organization and put at the bottom of the message, so we don't re-use id 2?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. So the practice of deprecating a field is to prefix it?
  2. Is it the same if it's not optional?
  3. And last one :D Is it accepted to remove the prefix if at some point we decide to re-introduce that label?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @evankanderson, addressed in 907044c.

@@ -1004,8 +996,7 @@ message Provider {
// Context defines the context in which a rule is evaluated.
// this normally refers to a combination of the provider, organization and project.
message Context {
string provider = 1;
optional string organization = 2;
optional string provider = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://protobuf.dev/programming-guides/proto3/#field-labels:

If no explicit field label is applied, the default field label, called “implicit field presence,” is assumed. (You cannot explicitly set a field to this state.) A well-formed message can have zero or one of this field (but not more than one). You also cannot determine whether a field of this type was parsed from the wire. An implicit presence field will be serialized to the wire unless it is the default value. For more on this subject, see Field Presence.

This means that if a client built with optional string provider sends an empty message to a server built with string provider, the server will read the string as "". Similarly, if the server sends "" as a string provider, the client will read null, because the default value of the field won't be serialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming this.

@teodor-yanev teodor-yanev merged commit 96adac7 into main Dec 7, 2023
14 checks passed
@teodor-yanev teodor-yanev deleted the remove-org-field-req-ctx-opt branch December 7, 2023 10:56
Comment on lines -972 to -976
message Context {
string organization = 1;
string project = 2;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally glossed over this, thinking it was unused (despite the fact that it's in the next unchanged diff line).

This is an unsafe change, since proto's wire format depends only on tag numbers. (It will compile, but it's not compatible with old client / new server). The Context below was referencing minder.v1.Provider.Context, which was defined as:

message X {
  string organization = 1;
  string project = 2;
}

It was replaced with the minder.v1.Context message, which is defined as:

message Y {
    optional string provider = 1;
    optional string project = 3;
}

While the types of the two don't conflict, message X believes that tag 1 is an organization, while Y believes that tag 1 is a provider. This means that a client might send a message that says "create a Provider in organization f31eag... and project 0a31fd...", but the server interprets this as "create a Provider with provider name f31eag... and retired organization 0a31fd... (and project "")".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants