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

mirrord policy for enforcing header pattern #3048

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

DmitryDodzin
Copy link
Member

Update to MirrordPolicy and MirrordClusterPolicy to allow enforcing header pattern for steal-with-filter

@DmitryDodzin DmitryDodzin force-pushed the dimad/mbe-673-mirrord-policy-for-enforcing-header-pattern branch from c3d5957 to 18bc696 Compare January 29, 2025 15:34
mirrord/operator/src/crd/policy.rs Outdated Show resolved Hide resolved
mirrord/operator/src/crd/policy.rs Outdated Show resolved Hide resolved
mirrord/operator/src/crd/policy.rs Outdated Show resolved Hide resolved
Forbidden {
blocked_action: BlockedAction,
policy_name: Option<String>,
reason: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks compatibility, we can't add new fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be fine because it's at the end of all messages that contain this result and it looks working for old layer > new operator and new layer > old operator

Copy link
Member Author

Choose a reason for hiding this comment

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

Like we don't have any message where RemoteResult isn't the last field (well all the places it's the only field)

Copy link
Contributor

Choose a reason for hiding this comment

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

This test mocks new intproxy and old operator, decoding the first message fails :<

#[test]
fn compat_test() {
    #[derive(Encode, Decode, PartialEq, Eq, Debug)]
    enum ResponseErrorOld {
        IdsExhausted(String),
        NotFound(u64),
        NotDirectory(u64),
        NotFile(u64),
        RemoteIO(RemoteIOError),
        DnsLookup(DnsLookupError),
        Remote(RemoteError),
        PortAlreadyStolen(Port),
        NotImplemented,
        Forbidden {
            blocked_action: BlockedAction,
            policy_name: Option<String>,
        },
        StripPrefix(String),
        OpenLocal,
    }

    #[derive(Encode, Decode, PartialEq, Eq, Debug)]
    enum ResponseErrorNew {
        IdsExhausted(String),
        NotFound(u64),
        NotDirectory(u64),
        NotFile(u64),
        RemoteIO(RemoteIOError),
        DnsLookup(DnsLookupError),
        Remote(RemoteError),
        PortAlreadyStolen(Port),
        NotImplemented,
        Forbidden {
            blocked_action: BlockedAction,
            policy_name: Option<String>,
            reason: Option<String>,
        },
        StripPrefix(String),
        OpenLocal,
    }

    let mut codec_old_sender = ProtocolCodec::<(), ResponseErrorOld>::default();
    let mut codec_new_receiver = ProtocolCodec::<ResponseErrorNew, ()>::default();
    let mut buffer = BytesMut::default();

    codec_old_sender
        .encode(
            ResponseErrorOld::Forbidden {
                blocked_action: BlockedAction::Mirror(80),
                policy_name: Some("name_1".into()),
            },
            &mut buffer,
        )
        .unwrap();
    codec_old_sender
        .encode(
            ResponseErrorOld::Forbidden {
                blocked_action: BlockedAction::Steal(StealType::All(99)),
                policy_name: Some("name_2".into()),
            },
            &mut buffer,
        )
        .unwrap();

    let error_1 = codec_new_receiver.decode(&mut buffer).unwrap();
    assert_eq!(
        error_1,
        Some(ResponseErrorNew::Forbidden {
            blocked_action: BlockedAction::Mirror(80),
            policy_name: Some("name_1".into()),
            reason: None
        })
    );

    let error_2 = codec_new_receiver.decode(&mut buffer).unwrap();
    assert_eq!(
        error_2,
        Some(ResponseErrorNew::Forbidden {
            blocked_action: BlockedAction::Steal(StealType::All(99)),
            policy_name: Some("name_2".into()),
            reason: None
        })
    );
}

/// Require user's filter to match this regex if filter is provided. (this works in tandom with
/// `steal-without-filter` block to require the user to specify a header filter for network
/// steal feature)
pub header_filter: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if multiple filters are used? Right now the user can specify any_of/all_of filters.

I think the semantics should be that an HTTP filter is accepted only when it has a header filter matching this pattern, and this header filter is required to match for the request to be stolen (so any_of(some-path-filter, valid-header-filter) would not pass the policy check). If so, this should probably be named like requires_header_filter or required_header_filter, and the doc should mention the case of composed filters.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is also the possibility of multiple rules being applied on single target so making a correct regex can be tricky

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

Successfully merging this pull request may close these issues.

2 participants