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

Use Alpha NEG API when DualStackNEG feature is enabled #2092

Merged
merged 2 commits into from
May 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkg/neg/syncers/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ type transactionSyncer struct {

// enableDegradedMode indicates whether we do endpoint calculation using degraded mode procedures
enableDegradedMode bool
// Enables support for Dual-Stack NEGs within the NEG Controller.
enableDualStackNEG bool

// podLabelPropagationConfig configures the pod label to be propagated to NEG endpoints
podLabelPropagationConfig labels.PodLabelPropagationConfig
Expand Down Expand Up @@ -168,6 +170,7 @@ func NewTransactionSyncer(
errorState: false,
logger: logger,
enableDegradedMode: flags.F.EnableDegradedMode,
enableDualStackNEG: enableDualStackNEG,
podLabelPropagationConfig: lpConfig,
networkInfo: networkInfo,
}
Expand Down Expand Up @@ -243,7 +246,7 @@ func (s *transactionSyncer) syncInternalImpl() error {
}
s.logger.V(2).Info("Sync NEG", "negSyncerKey", s.NegSyncerKey.String(), "endpointsCalculatorMode", s.endpointsCalculator.Mode())

currentMap, currentPodLabelMap, err := retrieveExistingZoneNetworkEndpointMap(s.NegSyncerKey.NegName, s.zoneGetter, s.cloud, s.NegSyncerKey.GetAPIVersion(), s.endpointsCalculator.Mode())
currentMap, currentPodLabelMap, err := retrieveExistingZoneNetworkEndpointMap(s.NegSyncerKey.NegName, s.zoneGetter, s.cloud, s.NegSyncerKey.GetAPIVersion(), s.endpointsCalculator.Mode(), s.enableDualStackNEG)
if err != nil {
return fmt.Errorf("%w: %v", negtypes.ErrCurrentNegEPNotFound, err)
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/neg/syncers/transaction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1540,7 +1540,7 @@ func TestUnknownNodes(t *testing.T) {
}

// Check that unknown zone did not cause endpoints to be removed
out, _, err := retrieveExistingZoneNetworkEndpointMap(testNegName, zoneGetter, fakeCloud, meta.VersionGA, negtypes.L7Mode)
out, _, err := retrieveExistingZoneNetworkEndpointMap(testNegName, zoneGetter, fakeCloud, meta.VersionGA, negtypes.L7Mode, false)
if err != nil {
t.Errorf("errored retrieving existing network endpoints")
}
Expand Down Expand Up @@ -1819,7 +1819,7 @@ func TestEnableDegradedMode(t *testing.T) {
(s.syncer.(*syncer)).stopped = false
tc.modify(s)

out, _, err := retrieveExistingZoneNetworkEndpointMap(tc.negName, zoneGetter, fakeCloud, meta.VersionGA, negtypes.L7Mode)
out, _, err := retrieveExistingZoneNetworkEndpointMap(tc.negName, zoneGetter, fakeCloud, meta.VersionGA, negtypes.L7Mode, false)
if err != nil {
t.Errorf("errored retrieving existing network endpoints")
}
Expand All @@ -1832,7 +1832,7 @@ func TestEnableDegradedMode(t *testing.T) {
t.Errorf("syncInternal returned %v, expected %v", err, tc.expectErr)
}
err = wait.PollImmediate(time.Second, 3*time.Second, func() (bool, error) {
out, _, err = retrieveExistingZoneNetworkEndpointMap(tc.negName, zoneGetter, fakeCloud, meta.VersionGA, negtypes.L7Mode)
out, _, err = retrieveExistingZoneNetworkEndpointMap(tc.negName, zoneGetter, fakeCloud, meta.VersionGA, negtypes.L7Mode, false)
if err != nil {
return false, nil
}
Expand Down
12 changes: 8 additions & 4 deletions pkg/neg/syncers/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ func ipsForPod(eds []negtypes.EndpointsData) map[types.NamespacedName]negtypes.N
}

// retrieveExistingZoneNetworkEndpointMap lists existing network endpoints in the neg and return the zone and endpoints map
func retrieveExistingZoneNetworkEndpointMap(negName string, zoneGetter negtypes.ZoneGetter, cloud negtypes.NetworkEndpointGroupCloud, version meta.Version, mode negtypes.EndpointsCalculatorMode) (map[string]negtypes.NetworkEndpointSet, labels.EndpointPodLabelMap, error) {
func retrieveExistingZoneNetworkEndpointMap(negName string, zoneGetter negtypes.ZoneGetter, cloud negtypes.NetworkEndpointGroupCloud, version meta.Version, mode negtypes.EndpointsCalculatorMode, enableDualStackNEG bool) (map[string]negtypes.NetworkEndpointSet, labels.EndpointPodLabelMap, error) {
// Include zones that have non-candidate nodes currently. It is possible that NEGs were created in those zones previously and the endpoints now became non-candidates.
// Endpoints in those NEGs now need to be removed. This mostly applies to VM_IP_NEGs where the endpoints are nodes.
zones, err := zoneGetter.ListZones(utils.AllNodesPredicate)
Expand Down Expand Up @@ -520,6 +520,9 @@ func retrieveExistingZoneNetworkEndpointMap(negName string, zoneGetter negtypes.
if ne.NetworkEndpoint.Port != 0 {
newNE.Port = strconv.FormatInt(ne.NetworkEndpoint.Port, 10)
}
if enableDualStackNEG {
newNE.IPv6 = ne.NetworkEndpoint.Ipv6Address
}
zoneNetworkEndpointMap[zone].Insert(newNE)
endpointPodLabelMap[newNE] = ne.NetworkEndpoint.Annotations
}
Expand Down Expand Up @@ -548,9 +551,10 @@ func makeEndpointBatch(endpoints negtypes.NetworkEndpointSet, negType negtypes.N
return nil, fmt.Errorf("failed to decode endpoint port %v: %w", networkEndpoint, err)
}
cloudNetworkEndpoint := &composite.NetworkEndpoint{
Instance: networkEndpoint.Node,
IpAddress: networkEndpoint.IP,
Port: int64(portNum),
Instance: networkEndpoint.Node,
IpAddress: networkEndpoint.IP,
Ipv6Address: networkEndpoint.IPv6,
Port: int64(portNum),
}
if flags.F.EnableNEGLabelPropagation {
annotations, ok := endpointPodLabelMap[networkEndpoint]
Expand Down
2 changes: 1 addition & 1 deletion pkg/neg/syncers/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1054,7 +1054,7 @@ func TestRetrieveExistingZoneNetworkEndpointMap(t *testing.T) {
for _, tc := range testCases {
tc.mutate(negCloud)
// tc.mode of "" will result in the default node predicate being selected, which is ok for this test.
endpointSets, annotationMap, err := retrieveExistingZoneNetworkEndpointMap(negName, zoneGetter, negCloud, meta.VersionGA, tc.mode)
endpointSets, annotationMap, err := retrieveExistingZoneNetworkEndpointMap(negName, zoneGetter, negCloud, meta.VersionGA, tc.mode, false)

if tc.expectErr {
if err == nil {
Expand Down
16 changes: 16 additions & 0 deletions pkg/neg/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/ingress-gce/pkg/annotations"
"k8s.io/ingress-gce/pkg/flags"
"k8s.io/ingress-gce/pkg/network"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/namer"
Expand Down Expand Up @@ -287,6 +288,21 @@ func (key NegSyncerKey) String() string {
// GetAPIVersion returns the compute API version to be used in order
// to create the negType specified in the given NegSyncerKey.
func (key NegSyncerKey) GetAPIVersion() meta.Version {
if flags.F.EnableDualStackNEG {
// This condition can be removed when we're getting rid of the feature flag
// or when GCE meta.VersionGA API supports the IPv6 fields.
//
// As an exception, it's easier here to access the global flag value and not
// plumb it into the NegSyncerKey. Generally, code within the NEG controller
// tires to avoid accessing global flag values to aid with scenarios in unit
// testing -- in this case though, the actual differentiator between Alpha
// and other versions is NOT something that can (or rather should) be
// covered within unit tests.
//
// TODO(gauravkghildiyal): Start using Beta APIs once they have the
// necessary changes.
return meta.VersionAlpha
}
switch key.NegType {
case NonGCPPrivateEndpointType:
return meta.VersionAlpha
Expand Down