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

Bump network-policy-api dependency to v0.1.5 #6353

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Dyanngg
Copy link
Contributor

@Dyanngg Dyanngg commented May 21, 2024

Previously released version mistakenly contains go.mod dependency to
k8s.io/kubernetes, which should be removed.

In addition, there are following changes made in network-policy-api:

  • sameLabels and notSameLabels are deprecated to make way for the
    future tenancy based API
  • Peers are split into AdminNetworkPolicyIngress/EgressPeer since
    there will be a fqdn field added specifically for the egress peer
  • Minor changes in terms of nesting of pod/ns selectors
  • Conformance profiles are added

Antrea implementation has been updated to reflect those changes.

@Dyanngg Dyanngg requested a review from antoninbas May 21, 2024 20:40
@Dyanngg Dyanngg force-pushed the update-network-policy-api-dep branch 8 times, most recently from f049c78 to e13d0e6 Compare May 22, 2024 18:22
@Dyanngg Dyanngg removed the request for review from antoninbas May 23, 2024 01:48
@Dyanngg Dyanngg force-pushed the update-network-policy-api-dep branch 3 times, most recently from 6c7d3eb to e53325f Compare June 7, 2024 20:58
Previously released version still contain go.mod dependency to
k8s.io/kubernetes, which should be removed.

In addition, there are following changes made in network-policy-api:
- sameLabels and notSameLabels are deprecated to make way for the
  future tenancy based API
- Because of the removal of these fields, admission control of the
  ANP and BANP resources are no longer required
- Peers are split into AdminNetworkPolicyIngress/EgressPeer since
  there will be a fqdn field added specifically for the egress peer
- Minor changes in terms of nesting of pod/ns selectors

Antrea implementation has been updated to reflect those changes.

Signed-off-by: Dyanngg <dingyang@vmware.com>
@Dyanngg Dyanngg force-pushed the update-network-policy-api-dep branch from e53325f to ae629ff Compare June 10, 2024 17:43
@Dyanngg Dyanngg changed the title [WIP] Bump network-policy-api dependency to v0.1.5 Bump network-policy-api dependency to v0.1.5 Jun 10, 2024
@Dyanngg Dyanngg requested a review from antoninbas June 10, 2024 18:07
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Just to clarify: you are intentionally keeping the /validate/anp and /validate/banp validation endpoints in the Antrea APIServer?

s.Handler.NonGoRestfulMux.HandleFunc("/validate/anp", webhook.HandlerForValidateFunc(v.Validate))
s.Handler.NonGoRestfulMux.HandleFunc("/validate/banp", webhook.HandlerForValidateFunc(v.Validate))

I think this is the right thing to do, as some users may be using an old Antrea YAML manifest with a newer Antrea container image. But maybe we should add a comment to apiserver.go to indicate why we have these paths registered, but we are not actually performing any validation?

BTW, the upstream API changes break backwards-compatibility, but I assume it was intentional (Alpha API)? Even then, it would have been nice to have an API version bump upstream (v1alpha2). I wonder if we should add a note in our own release notes for v2.1 about this breaking change. Not bumping up the API version upstream makes it difficult to track which "version" is implemented by a given version of a network plugin such as Antrea.

Comment on lines +151 to +152
--organization=antrea-io -project=antrea -url=https://github.com/antrea-io/antrea -version=v2.0 \
--additional-info=https://github.com/antrea-io/antrea/actions/workflows/kind.yml \
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious to know what these 4 flags are for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you want to handle the version though? Because technically, the version being tested here is unreleased when this is run as part of CI.

What you could do if you want is use an environment variable, that can be set when calling this script, and would otherwise default to the contents of the VERSION file (which is at the root of the repository). Then you would add a new Github workflow that can be run on demand (workflow_dispatch trigger) and would run the tests for a specific released Antrea version (version string to be provided as a workflow input). The workflow would upload the generated compliance report as an artifact for convenient download, and could even be run automatically for new Antrea releases.
I think that's the right approach. but it could be done in a later PR. For now I would just recommend implementing part of this proposal:

  • add VERSION environment variable or command-line flag
    • if provided, install the specified Antrea version and use the provided version value as -version
    • if not provided, keep the current behavior, but use the contents of the VERSION file ($(head -n1 $ROOT_DIR/VERSION)) for -version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. I'll update this PR once the network-policy-api cuts a v2.0 release, which is going to happen very soon

@@ -265,7 +225,8 @@ func banpActionToCRDAction(action v1alpha1.BaselineAdminNetworkPolicyRuleAction)
}

func (n *NetworkPolicyController) processAdminNetworkPolicy(anp *v1alpha1.AdminNetworkPolicy) (*antreatypes.NetworkPolicy, map[string]*antreatypes.AppliedToGroup, map[string]*antreatypes.AddressGroup) {
appliedToPerRule := anpHasNamespaceLabelRule(anp)
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we have this function call before, given that this case was not supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah they shouldn't really be there, admission webhook would reject any policies that have these fields

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Jun 11, 2024

Just to clarify: you are intentionally keeping the /validate/anp and /validate/banp validation endpoints in the Antrea APIServer? I think this is the right thing to do, as some users may be using an old Antrea YAML manifest with a newer Antrea container image. But maybe we should add a comment to apiserver.go to indicate why we have these paths registered, but we are not actually performing any validation?

Yes I'll also add a TODO to have these removed at some point.

BTW, the upstream API changes break backwards-compatibility, but I assume it was intentional (Alpha API)? Even then, it would have been nice to have an API version bump upstream (v1alpha2). I wonder if we should add a note in our own release notes for v2.1 about this breaking change. Not bumping up the API version upstream makes it difficult to track which "version" is implemented by a given version of a network plugin such as Antrea.

Agreed

Copy link
Contributor

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 19, 2024
@Dyanngg Dyanngg removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 23, 2024
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