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

Add accessPolicy field to Server CRD #12845

Merged
merged 3 commits into from
Jul 22, 2024
Merged

Add accessPolicy field to Server CRD #12845

merged 3 commits into from
Jul 22, 2024

Conversation

alpeb
Copy link
Member

@alpeb alpeb commented Jul 15, 2024

Followup to #12844

This new field defines the default policy for Servers, i.e. if a request doesn't match the policy associated to a Server then this policy applies. The values are the same as for proxy.defaultInboundPolicy and the config.linkerd.io/default-inbound-policy annotation (all-unauthenticated, all-authenticated, cluster-authenticated, cluster-unauthenticated, deny), plus a new value "audit". The default is "deny", thus remaining backwards-compatible.

This field is also exposed as an additional printer column.

@alpeb alpeb requested a review from a team as a code owner July 15, 2024 21:41
alpeb added a commit that referenced this pull request Jul 15, 2024
Followup to #12845, branched off alpeb/policy-audit-crd

This expands the policy controller index in the following ways:

- Adds the new Audit variant to the DefaultPolicy enum
- Expands the function that synthesizes the authorizations for a given default policy (DefaultPolicy::default_authzs) so that it also creates an Unauthenticated client auth and a allow-all NetworkMatch for the new Audit default policy.
- Now that a Server can have a default policy different than Deny, when generating InboundServer authorizations (PolicyIndex::client_authzs) make sure to append the default authorizations when DefaultPolicy is Allow or Audit

Also, the admission controller ensures the new accessPolicy field contains a valid value.
@alpeb alpeb marked this pull request as draft July 15, 2024 22:54
@alpeb alpeb marked this pull request as ready for review July 16, 2024 13:46
@@ -148,6 +148,11 @@ spec:
- required: [podSelector]
- required: [externalWorkloadSelector]
properties:
accessPolicy:
Copy link
Member

Choose a reason for hiding this comment

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

Since we are adding a new field to the resource, should we also bump the resource version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was relying on this being BC to not bump the version, but good call, if someone wants to use this field the only way to guarantee it's supported is through the version.

Copy link
Member

@mateiidavid mateiidavid 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 to me! With the additional caveat that bumping the version means we once again get into some interesting questions around version skew for multicluster.

I think we should be fine here though, since a newer client should be able to send requests to an API Server that has an older version of the CRD and that all should work (except the returned resource won't have the new field). We're not reading the field anywhere in the destination service, or service mirror (which require connections to other API Servers). So all in all we shouldn't see any problems here, right?

Base automatically changed from alpeb/policy-audit-option to main July 17, 2024 20:54
Followup to #12844, branched off of alpeb/policy-audit-option

This new field defines the default policy for Servers, i.e. if a request doesn't match the policy associated to a Server then this policy applies.
The values are the same as for `proxy.defaultInboundPolicy` and the `config.linkerd.io/default-inbound-policy` annotation (all-unauthenticated, all-authenticated, cluster-authenticated, cluster-unauthenticated, deny), plus a new value "audit".
The default is "deny", thus remaining backwards-compatible. For this same reason no new version of the CRD is required.

This field is also exposed as an additional printer column.
alpeb added a commit that referenced this pull request Jul 18, 2024
Followup to #12845, branched off alpeb/policy-audit-crd

This expands the policy controller index in the following ways:

- Adds the new Audit variant to the DefaultPolicy enum
- Expands the function that synthesizes the authorizations for a given default policy (DefaultPolicy::default_authzs) so that it also creates an Unauthenticated client auth and a allow-all NetworkMatch for the new Audit default policy.
- Now that a Server can have a default policy different than Deny, when generating InboundServer authorizations (PolicyIndex::client_authzs) make sure to append the default authorizations when DefaultPolicy is Allow or Audit

Also, the admission controller ensures the new accessPolicy field contains a valid value.
@alpeb
Copy link
Member Author

alpeb commented Jul 19, 2024

Thanks Matei, let me summarize what we discussed off-github:

  • Source cluster with newer CRD, target cluster with old CRD: it breaks! This is the output from the destination controller:
level=error msg="Error adding cluster target to store: server CRD (policy.linkerd.io/v1beta3) not found" component=cluster-store

The target doesn't know that version exists, so it returns an error.

  • Source cluster with old CRD, target cluster with new CRD: it works as per my testing. We're still serving both versions and we're using the default "None" conversion strategy, which means that the Server is returned as its stored schema (v1beta3) with only the apiVersion field accommodating to the requested one (v1beta2).
    I did some client-go scavenging and confirmed the serializer built in server_client.go creates a CodecFactory leaving the default CodecFactoryOptions.Strict: false, so the decoder doesn't complain if there's an unrecognized field.

The first problem disappears as soon as we upgrade the CRDs on the target cluster and restart the destination controller. So something folks need to have clear when upgrading multicluster.

@alpeb alpeb merged commit 71291fe into main Jul 22, 2024
48 checks passed
@alpeb alpeb deleted the alpeb/policy-audit-crd branch July 22, 2024 14:01
alpeb added a commit that referenced this pull request Jul 22, 2024
Followup to #12845, branched off alpeb/policy-audit-crd

This expands the policy controller index in the following ways:

- Adds the new Audit variant to the DefaultPolicy enum
- Expands the function that synthesizes the authorizations for a given default policy (DefaultPolicy::default_authzs) so that it also creates an Unauthenticated client auth and a allow-all NetworkMatch for the new Audit default policy.
- Now that a Server can have a default policy different than Deny, when generating InboundServer authorizations (PolicyIndex::client_authzs) make sure to append the default authorizations when DefaultPolicy is Allow or Audit

Also, the admission controller ensures the new accessPolicy field contains a valid value.
alpeb added a commit that referenced this pull request Jul 23, 2024
Followup to #12845, branched off alpeb/policy-audit-crd

This expands the policy controller index in the following ways:

- Adds the new Audit variant to the DefaultPolicy enum
- Expands the function that synthesizes the authorizations for a given default policy (DefaultPolicy::default_authzs) so that it also creates an Unauthenticated client auth and a allow-all NetworkMatch for the new Audit default policy.
- Now that a Server can have a default policy different than Deny, when generating InboundServer authorizations (PolicyIndex::client_authzs) make sure to append the default authorizations when DefaultPolicy is Allow or Audit

Also, the admission controller ensures the new accessPolicy field contains a valid value.
alpeb added a commit that referenced this pull request Jul 23, 2024
Followup to #12845

This expands the policy controller index in the following ways:

- Adds the new Audit variant to the DefaultPolicy enum
- Expands the function that synthesizes the authorizations for a given default policy (DefaultPolicy::default_authzs) so that it also creates an Unauthenticated client auth and a allow-all NetworkMatch for the new Audit default policy.
- Now that a Server can have a default policy different than Deny, when generating InboundServer authorizations (PolicyIndex::client_authzs) make sure to append the default authorizations when DefaultPolicy is Allow or Audit

Also, the admission controller ensures the new accessPolicy field contains a valid value.

Required test changes are addressed in #12847. Also note you'll need the proxy changes at linkerd/linkerd2-proxy#3068 to make this work.

Please check linkerd/website#1805 for how this is supposed to work from the user's perspective.
alpeb added a commit that referenced this pull request Jul 25, 2024
Followup to #12845

This expands the policy controller index in the following ways:

- Adds the new Audit variant to the DefaultPolicy enum
- Expands the function that synthesizes the authorizations for a given default policy (DefaultPolicy::default_authzs) so that it also creates an Unauthenticated client auth and a allow-all NetworkMatch for the new Audit default policy.
- Now that a Server can have a default policy different than Deny, when generating InboundServer authorizations (PolicyIndex::client_authzs) make sure to append the default authorizations when DefaultPolicy is Allow or Audit

Also, the admission controller ensures the new accessPolicy field contains a valid value.

Required test changes are addressed in #12847. Also note you'll need the proxy changes at linkerd/linkerd2-proxy#3068 to make this work.

Please check linkerd/website#1805 for how this is supposed to work from the user's perspective.
alpeb added a commit that referenced this pull request Jul 26, 2024
Followup to #12845

This expands the policy controller index in the following ways:

- Adds the new Audit variant to the DefaultPolicy enum
- Expands the function that synthesizes the authorizations for a given default policy (DefaultPolicy::default_authzs) so that it also creates an Unauthenticated client auth and a allow-all NetworkMatch for the new Audit default policy.
- Now that a Server can have a default policy different than Deny, when generating InboundServer authorizations (PolicyIndex::client_authzs) make sure to append the default authorizations when DefaultPolicy is Allow or Audit

Also, the admission controller ensures the new accessPolicy field contains a valid value.

Required test changes are addressed in #12847. Also note you'll need the proxy changes at linkerd/linkerd2-proxy#3068 to make this work.

Please check linkerd/website#1805 for how this is supposed to work from the user's perspective.
alpeb added a commit that referenced this pull request Jul 26, 2024
Followup to #12845

This expands the policy controller index in the following ways:

- Adds the new Audit variant to the DefaultPolicy enum
- Expands the function that synthesizes the authorizations for a given default policy (DefaultPolicy::default_authzs) so that it also creates an Unauthenticated client auth and a allow-all NetworkMatch for the new Audit default policy.
- Now that a Server can have a default policy different than Deny, when generating InboundServer authorizations (PolicyIndex::client_authzs) make sure to append the default authorizations when DefaultPolicy is Allow or Audit

Also, the admission controller ensures the new accessPolicy field contains a valid value.

## Tests

New integration tests added:

- e2e_audit.rs exercising first the audit policy in Server, and then at the namespace level
- in admit_server.rs a new test checks invalid accessPolicy values are rejected.
- in inbound_api.rs server_with_audit_policy verifies the synthesized audit authorization is returned for a Server with accessPolicy=audit

> [!NOTE]
> Please check linkerd/website#1805 for how this is supposed to work from the user's perspective.
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