System - Improve versioning of system to allow backward compatibility

[dvstans]

Instead of using the protobuf version to lock all components to the same version, there should be a way to allow newer releases to permit older deployed components to work, if that is desired. Currently, a bump in the protobuf version causes all older components (Python CLI/API, Globus, Repos) to simply stop working. One simple solution is to simple have two version numbers - one for the protocol, and one for the system. A substantial difference in the protocol version would be blocked, but a system version difference would simply generate a warning to upgrade when possible.

[dvstans]

Also need to add version checks to repo server and python CLI/API. (web server already has check.)

[dvstans]

And the gridFTP auth worker.

[dvstans]

On second thought, it would be reasonable to say that the gridFTP authz API shall never change, so no need to do a version check.

[dvstans]

This has become more complex - the current system of using alphabetical order of messages had to be changed to file order to allow protobuf additions to not break backward compatibility. This required writing a python script to generate name/index mapping for python protobuf output files (and modifying cmake files as well). This now works, but the proto version needs to also be split into a major/minor component to work correctly (not done yet)

[dvstans]

Fixed. Added version check to repo server. Changed how proto files are loaded in all system services.

[JoshuaSBrown]

This actually, still breaks things. It looks like the file order is non deterministic.

  {
    auto a_enum_desc = Anon::Protocol_descriptor();
    if (a_enum_desc->name() != "Protocol")
      EXCEPT(EC_PROTO_INIT, "Must register with Protocol EnumDescriptor.");

    const proto::FileDescriptor *file = a_enum_desc->file();
    if (!file)
      EXCEPT(EC_PROTO_INIT,
             "Failed to acquire protocol buffer file descriptor.");

    const proto::EnumValueDescriptor *val_desc =
        a_enum_desc->FindValueByName("ID");
    if (!val_desc)
      EXCEPT(EC_PROTO_INIT, "Protocol enum missing required ID field.");

    uint16_t id = val_desc->number();
    // std::cout << __FILE__ << ":" << __LINE__ << " PROTOCOL id is " << id <<
    // std::endl;
    m_file_descriptor_map[id] = file;

    int count = file->message_type_count();
    uint16_t msg_type = id << 8;

    for (int i = 0; i < count; i++, msg_type++) {
      const proto::Descriptor *desc = file->message_type(i);
      m_descriptor_map[msg_type] = desc;
      // Register Message types from  Anon
      m_msg_type_map[desc] = msg_type;
    }
    m_protocol_ids[MessageProtocol::GOOGLE_ANONONYMOUS] = id;
  }

The order of messages returned by file->message_type(i) is NOT guaranteed to remain stable across code changes.

Something like the below would be a much more robust solution.

message Envelope {
  oneof payload {
    Login login = 1;
    Logout logout = 2;
    UpdateProfile update_profile = 3;
  }
}

1. Deserialize Envelope first (You always know you're receiving an Envelope, so this is safe.)

Envelope env;
env.ParseFromString(data);

2. Check which message type is active In C++, oneof generates enum accessors and has_ methods:

switch (env.payload_case()) {
    case Envelope::kLogin:
        // handle login
        break;
    case Envelope::kLogout:
        // handle logout
        break;
    case Envelope::kUpdateProfile:
        // handle update profile
        break;
    case Envelope::PAYLOAD_NOT_SET:
        // handle empty envelope
        break;
}

Or using HasField():

if (env.has_login()) {
    // handle login
} else if (env.has_logout()) {
    // handle logout
}

[JoshuaSBrown]

This might very well be related to to https://github.com/ORNL/DataFed/issues/840

[JoshuaSBrown]

Here is a list of best practices to follow, https://protobuf.dev/best-practices/dos-donts/

https://protobuf.dev/best-practices/1-1-1/

Rationale
The 1-1-1 best practice is to keep every proto_library and .proto file as small as is reasonable, with the ideal being:

One proto_library build rule
One source .proto file
One top-level entity (message, enum, or extension)
Having the fewest number of message, enum, extension, and services as you reasonably can makes refactoring easier. Moving files when they’re separated is much easier than extracting messages from a file with other messages.

Following this practice can help build times and binary size by reducing the size of your transitive dependencies in practice: when some code only needs to use one enum, under a 1-1-1 design it can depend just on the .proto file that defines that enum and avoid incidentally pulling in a large set of transitive dependencies that may only be used by another message defined in the same file.