Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Add antrea-interworking package #4510

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

Conversation

wenqiq
Copy link

@wenqiq wenqiq commented Mar 22, 2023

Signed-off-by: Wenqi Qiu [email protected]

What this PR does / why we need it

This pr extends antrea package with interworking, when antreaNsx is enabled in antreaConfig, interworking will also be reconciled by kapp-controller

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

Release note


Additional information

Special notes for your reviewer

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230322065748/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #4510 (1e97f4f) into main (bb07e03) will decrease coverage by 1.32%.
The diff coverage is 23.25%.

@@            Coverage Diff             @@
##             main    #4510      +/-   ##
==========================================
- Coverage   49.77%   48.46%   -1.32%     
==========================================
  Files         453      485      +32     
  Lines       45424    47981    +2557     
==========================================
+ Hits        22612    23252     +640     
- Misses      20652    22518    +1866     
- Partials     2160     2211      +51     
Impacted Files Coverage Δ
...dons/controllers/antrea/antreaconfig_controller.go 0.00% <0.00%> (ø)
addons/controllers/antrea/antreaconfig_util.go 6.28% <27.77%> (ø)

... and 36 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230323063342/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from c4cabd5 to 9966d51 Compare March 23, 2023 08:41
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230323084931/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230324034849/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@hangyan
Copy link
Contributor

hangyan commented Mar 27, 2023

seems the build CI faild.

configSpec.AntreaNsx.BootstrapFrom.ProviderRef.ApiVersion = config.Spec.AntreaNsx.BootstrapFrom.ProviderRef.ApiGroup
configSpec.AntreaNsx.BootstrapFrom.ProviderRef.Kind = config.Spec.AntreaNsx.BootstrapFrom.ProviderRef.Kind
configSpec.AntreaNsx.BootstrapFrom.ProviderRef.Name = config.Spec.AntreaNsx.BootstrapFrom.ProviderRef.Name
}
Copy link
Contributor

@wjun wjun Mar 27, 2023

Choose a reason for hiding this comment

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

Is there any else branch required here?

Copy link
Contributor

Choose a reason for hiding this comment

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

if this pr #4219 can be merged into v0.28, this should also be included in v0.28

.gitignore Outdated Show resolved Hide resolved
@kmova kmova added the do-not-merge/hold Some fixes necessary, hold for merging label Mar 28, 2023
@wenqiq wenqiq requested a review from a team as a code owner March 29, 2023 00:49
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230329010445/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from 3be91aa to 06e90be Compare March 29, 2023 06:33
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230329064458/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

Copy link
Member

@joshuatcasey joshuatcasey left a comment

Choose a reason for hiding this comment

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

Hi @wenqiq I'm not actually sure why this PR requires an approval from @vmware-tanzu/tkg-iam-owners , but I'll drop an approve here as this does not appear to impact any TKG IAM code.

@wenqiq
Copy link
Author

wenqiq commented Mar 30, 2023

Hi @wenqiq I'm not actually sure why this PR requires an approval from @vmware-tanzu/tkg-iam-owners , but I'll drop an approve here as this does not appear to impact any TKG IAM code.

Thanks for reviewing. Not quite sure how it works, it seems to be triggered automatically.

@kmova kmova removed the do-not-merge/hold Some fixes necessary, hold for merging label Mar 30, 2023
@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from 2bfaeeb to 5c348fd Compare March 30, 2023 19:30
@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from 5c348fd to f163330 Compare March 30, 2023 19:34
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230330193546/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230330194635/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from f163330 to c990216 Compare March 31, 2023 05:10
@github-advanced-security
Copy link

You have successfully added a new Trivy configuration .github/workflows/cve_scan.yaml:trivy_scan. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@github-actions
Copy link

CVE Scan results for this PR can be viewed from
https://github.com/vmware-tanzu/tanzu-framework/security/code-scanning?query=pr%3A4510

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230331051919/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from c990216 to 27c0d76 Compare April 3, 2023 08:53
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230403090052/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from 27c0d76 to fd444fd Compare April 3, 2023 09:20
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230403092720/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230404074853/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from 31d8665 to 1b6ac9b Compare April 5, 2023 00:58
@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230405010820/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

// +kubebuilder:validation:Optional
Inline *AntreaNsxInline `json:"inline,omitempty"`
}
BootstrapSupervisorResourceName string `json:"bootstrapSupervisorResourceName,omitempty"`

Choose a reason for hiding this comment

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

I think we don't need to expose BootstrapSupervisorResourceName to CRD. BootstrapSupervisorResourceName is created by the antrea addon controller itself.

configSpec.AntreaNsx.AntreaNsxConfig.NSXCert = string(secret.Data["tls.crt"])
configSpec.AntreaNsx.AntreaNsxConfig.NSXKey = string(secret.Data["tls.key"])
configSpec.AntreaNsx.AntreaNsxConfig.VPCPath = config.Spec.AntreaNsx.AntreaNsxConfig.VPCPath
case bootstrapFromSupervisorCluster:

Choose a reason for hiding this comment

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

If config.Spec.AntreaNsx.AntreaNsxConfig.BootstrapFrom is empty or == bootstrapFromSupervisorCluster

configSpec.AntreaNsx.AntreaNsxConfig.NSXKey = string(secret.Data["tls.key"])
configSpec.AntreaNsx.AntreaNsxConfig.VPCPath = config.Spec.AntreaNsx.AntreaNsxConfig.VPCPath
case bootstrapFromSupervisorCluster:
configSpec.AntreaNsx.AntreaNsxConfig.BootstrapSupervisorResourceName = config.Spec.AntreaNsx.AntreaNsxConfig.BootstrapSupervisorResourceName

Choose a reason for hiding this comment

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

configSpec.AntreaNsx.AntreaNsxConfig.BootstrapSupervisorResourceName = NSXServiceAccount CR name. The NSXServiceAccount is created by Antrea addon controller, too. It's better find a way to derive the name automatically, rather than having user fill in the name manually.

@@ -288,51 +288,169 @@ type AntreaNsx struct {
// +kubebuilder:validation:Optional
// +kubebuilder:default:=false
Enable bool `json:"enable,omitempty"`
// BootstrapFrom either providerRef or inline configs
// +kubebuilder:validation:Optional
BootstrapFrom AntreaNsxBootstrapFrom `json:"bootstrapFrom,omitempty"`

Choose a reason for hiding this comment

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

We changed the CRD, so need to generate manifest and commit the manifest changes.

@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from 1b6ac9b to e1aa0ba Compare April 7, 2023 08:36
@github-actions
Copy link

github-actions bot commented Apr 7, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230407084433/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

configSpec.AntreaNsx.AntreaNsxConfig.BootstrapFrom = bootstrapFromSupervisorCluster
configSpec.AntreaNsx.AntreaNsxConfig.BootstrapSupervisorResourceName = getNSXServiceAccountName(cluster.Name)
}
configSpec.AntreaNsx.AntreaNsxConfig.ProxyEndpoints.NSXRpcFwdProxy = config.Spec.AntreaNsx.AntreaNsxConfig.ProxyEndpoints.NSXRpcFwdProxy

Choose a reason for hiding this comment

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

Line 346 and 347 should be moved to inside if config.Spec.AntreaNsx.AntreaNsxConfig.BootstrapFrom == bootstrapFromInline {. This is when bootstrap from SupervisorCluster, the Antrea-NSX adapters load proxy endpoints from NSXServiceAccount from Supervisor, it will ignore the proxy endpoints set initially in the config file.

Copy link
Author

Choose a reason for hiding this comment

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

done.

@@ -232,14 +225,14 @@ func (r *AntreaConfigReconciler) getProviderServiceAccountName(clusterName strin
return fmt.Sprintf("%s-antrea", clusterName)

Choose a reason for hiding this comment

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

There is also a method func (r *AntreaConfigReconciler) getProviderServiceAccountName(clusterName string) in this file. I think we can also remove (r *AntreaConfigReconciler) from that method to make it a pure function, and then we move both getProviderServiceAccountName and getNSXServiceAccountName functions to antreaconfig_util.go.

Copy link
Author

Choose a reason for hiding this comment

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

done.

// +kubebuilder:validation:Optional
Name string `json:"name,omitempty"`
}
NSXUser string `json:"nsxUser,omitempty"`

Choose a reason for hiding this comment

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

I think we don't need NSXUser and NSXPassword, maybe you forgot to remove them? They are useful for bootstrap Job, but we remove the bootstrap Job from the Package. Bootstrap Job is only used in interworking CI.

Copy link
Author

Choose a reason for hiding this comment

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

done.

// +kubebuilder:validation:Optional
ApiGroup string `json:"apigroup,omitempty"`
// Kind is the kind for crd, here its value is NsxServiceAccount
NSXCert string `json:"nsxCert,omitempty"`

Choose a reason for hiding this comment

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

I see that antreaconfig_util.go uses a getNSXCert function to get tls.crt and tls.key content from a Secret. I think that we can remove the NSXCert and NSXKey from the AntreaConfig CRD. Instead, we can introduce a NSXSecretName string field. It points to a Secret resource by name in the same Namespace. Then antreaconfig_util.go can refers to the NSXSecretName and pass it to getNSXCert function.

Copy link
Author

Choose a reason for hiding this comment

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

done.

bootstrapFromInline = "Inline"
bootstrapFromSupervisorCluster = "SupervisorCluster"
)

// AntreaConfigSpec defines the desired state of AntreaConfig
type AntreaConfigSpec struct {

Choose a reason for hiding this comment

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

AntreaConfigSpec should be aligned with Antrea package schema.yaml.

The AntreaNSX member in this struct needs to be renamed as antrea_nsx, and yaml:"antreaNsx,omitempty" needs to be changed to yaml:"antrea_nsx,omitempty".

Copy link
Author

Choose a reason for hiding this comment

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

done.

BootstrapFrom antreaNsxBootstrapFrom `yaml:"bootstrapFrom,omitempty"`
AntreaNsxConfig antreaNsxConfig `yaml:"config,omitempty"`
Enable bool `yaml:"enable,omitempty"`
AntreaNsxConfig antreaNsxConfig `yaml:"config,omitempty"`

Choose a reason for hiding this comment

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

Looks like AntreaNsxConfig should be moved to AntreaConfigSpec, and yaml:"config,omitempty" should be changed to yaml:"antrea_interworking,omitempty". This is to be aligned with Antrea package schema.yaml.

Copy link
Author

Choose a reason for hiding this comment

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

done.


return configSpec, nil
}

func copyStructAtoB(a interface{}, b interface{}) error {
va := reflect.ValueOf(a)
Copy link
Contributor

Choose a reason for hiding this comment

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

reflect can make it more feasible, but at same time it can introduce performance cost, maybe consider if the implementation is OK.

@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from e1aa0ba to a8d0063 Compare April 7, 2023 11:57
@github-actions
Copy link

github-actions bot commented Apr 7, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230407120432/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

github-actions bot commented Apr 7, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230407153157/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from 2317ac9 to dcfc598 Compare April 7, 2023 18:44
@github-actions
Copy link

github-actions bot commented Apr 7, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230407185312/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

Signed-off-by: Wenqi Qiu <[email protected]>

Fix unit-test

Signed-off-by: Wenqi Qiu <[email protected]>
Signed-off-by: Wenqi Qiu <[email protected]>
@wenqiq wenqiq force-pushed the topic/wenqi/antrea-interworking branch from dcfc598 to 1e97f4f Compare April 10, 2023 15:45
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4510/20230410155308/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

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

Successfully merging this pull request may close these issues.

9 participants