add-IPsecExternal #1667

JoshSalomon · 2023-11-16T09:58:25Z

Add api to enable/disable extern (NS) ipsec. The feature (NS ipsec) is in TR in 4.14, and in 4.15 we provide API for simple enablement (in 4.14 the enablement is awkward with no clear API

openshift-ci · 2023-11-16T09:58:39Z

Hello @JoshSalomon! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

yuvalk · 2023-11-16T15:20:03Z

/retest

yuvalk · 2023-11-16T15:22:18Z

The generated files for this change

can you reword the commit msg title, so it'll reference the other commit?
otherwise "this change" is ambiguous

operator/v1/types_network.go

yuvalk · 2023-11-16T15:34:45Z

operator/v1/types_network.go

+	// ipsecExtern enables external (NS) IPsec for the node, the configuration
+	// is done via nmstate operator.
+	// +optional
+	IPsecExtern *IPsecExtern `json:"ipsecExtern,omitempty"`


we might consider making it a flag not config like
IPsecHostEnable (with true/false possible values)

not 100% sure what's the best practice here though

JoelSpeed · 2023-11-17T10:38:11Z

operator/v1/types_network.go

+	// ipsecExternal enables external (NS) IPsec for the node, the configuration
+	// is done via nmstate operator.
+	// +optional
+	IPsecExternal *IPsecExternal `json:"ipsecExternal,omitempty"`


You're using the presence of the struct to enable the external ipsec? Would prefer to have an enum with the struct with an enum defining the state.

Does this field have any interactions with other fields within this configuration? Eg must I not specify some other fields when this field is specified?

Given you're adding a struct, do you expect there to be additional configuration in the future for this feature?

This is following the exact same pattern as the above IPsecConfig..

there is no interaction with other fields at the API level. the implementation will take care of all the relevant cases, mainly around the interaction between IPsecConfig and IPsecExternal (but they are both independent of each other on the API level)

and yes, it gives us the opportunity to add more config options in the future, without changing the API
see my comment - #1667 (comment)

@yuvalk - Does it make sense to put in this struct (in the future) reference to nmstate configuration to be used for the ipsec configuration. (name of the nmstate CRD?)

This is following the exact same pattern as the above IPsecConfig..

That API was merged 3 years ago, and API guidance has changed over time as we learn about prior mistakes. The Network resource is considered to be a configuration API. Where we prefer discoverability. To enable discoverability, we implement a few rules, eg, avoiding pointers and omitempty where possible.

Current guidance would suggest that this field should be

IPSecExternal IPSecExternal `json:"ipsecExternal"`

And the struct should contain a field

// +kubebuilder:validation:Enum="";Enabled;Disabled // +kubebuilder:validation:Required State IPSecExteranlState `json:"state"`

That said, looking at this, why is IPSecConfig and IPSecExternal not mutually exclusive.

there is no interaction with other fields at the API level. the implementation will take care of all the relevant cases, mainly around the interaction between IPsecConfig and IPsecExternal (but they are both independent of each other on the API level)

I'm interested in the behaviour that the operator will take care of, not just the interactions at the API level that we have defined so far. Given my question about not being mutually exclusive, I think understanding what the operator does might help here.

the names might be misleading
IPSecConfig - the old one. is used to enable/disable East-West IPsec
IPSecExternal - the new one, is used to enable/disable North-South IPsec

both will first deploy a MC to enable the host ipsec extension (ie - install libreswan and enable ipsec.service)
E-W is also running a DS that runs ovs ipsec monitor (used to configre ipsec policies between nodes)
N-S wont do anything else (beside the mentioned MC). ie - configuration is on the customer

there's also logic that handles upgrades etc.

openshift/enhancements#1453

Do you think we could make that clearer in the godoc, since I bet most people won't understand. How would you explain the north-south and east-west in laymans terms for the godoc? I think we probably want to update the documentation on both fields here to make it obvious where the traffic goes based on these two ipsec configurations

@JoelSpeed @yuvalk I am going to fix this, but I wonder if making this mandatory is not creating bw compatibility so that exiting yaml files will not work anymore. Shouldn't we make it optional with disabled value and remove the omitempty for discoverability?

IPSecExternal IPSecExternal json:"ipsecExternal"

@JoelSpeed does this means there is no default and it'll have to be specified at install time? (ie installer also need to change, and any place where even customer is injecting network config)

In situations such as this, we normally create an alias for the empty string, so that the empty string has the same meaning, in this case, as disabled.

@JoelSpeed - I made all the changes and tested them as much as I can, I hope it is OK now.

JoshSalomon · 2023-11-19T10:51:43Z

The generated files for this change

can you reword the commit msg title, so it'll reference the other commit? otherwise "this change" is ambiguous

Done

yuvalk · 2023-11-22T15:44:18Z

maybe something like this is better:

diff --git a/operator/v1/types_network.go b/operator/v1/types_network.go
index 52e9d53f..c98506f6 100644
--- a/operator/v1/types_network.go
+++ b/operator/v1/types_network.go
@@ -440,6 +440,11 @@ type OVNKubernetesConfig struct {
        // cluster.
        // +optional
        IPsecConfig *IPsecConfig `json:"ipsecConfig,omitempty"`
+       // ipsecExtern enables external (North-South). this will only install IPsec and
+       // enable it's service. the actuall NS IPsec policies should be specified by
+       // users. this can be done with k8s-nmstate operator.
+       // +optional
+       IPsecExternal IPsecExternal `json:"ipsecExternal"`
        // policyAuditConfig is the configuration for network policy audit events. If unset,
        // reported defaults are used.
        // +optional
@@ -480,6 +485,12 @@ type HybridOverlayConfig struct {
 type IPsecConfig struct {
 }
 
+type IPsecExternal struct {
+       // +kubebuilder:validation:Enum="";Enabled;Disabled
+       // +kubebuilder:validation:Required
+       State string `json:"state"`
+}
+
 type IPForwardingMode string
 
 const (

JoelSpeed · 2023-11-23T15:42:55Z

@yuvalk suggestion is what I was imagining. You could if you prefer, instead of the empty string use a default to default the value to Disabled

operator/v1/types_network.go

yuvalk · 2023-11-23T18:33:40Z

operator/v1/types_network.go

@@ -485,6 +487,11 @@ type IPsecConfig struct {
 }

 type IPsecExternal struct {
+	// State controls the node's external (aka NS) ipsec service state.
+	// +kubebuilder:validation:Enum="";Enabled;Disabled


deafault=Disabled, means we can remove the empty string "" option

This didn't work for me when the cluster starts. Somehow even with this default during the installation I got an error that value "" is not supported.

not sure which didnt work
the default setting?
or the ""

assuming default works, I'm saying you can remove the ""..

even with default=disabled, I got an error that "" is not supported. I am not sure how this works, but since the object is mandatory and is created by the system and not by the user, is is created with a value of "". My suggestion is that in the operator we will change "" to Disabled so for discoverability we will see only enable or disable but on the yaml file empty string is acceptable as valid input.

yuvalk · 2023-11-23T18:34:18Z

operator/v1/types_network.go

+
+const (
+	// IPSecExternalStateEmpty is used for defaulting, it is the same as disabled
+	IPSecExternalStateEmpty IPSecExternalState = ""


also from here

I am not sure this can be removed if the enum includes an empty string (I just don't know, I am not sure that giving a name to an empty string is a good idea, but hit is currently a valid value in the enum.

JoshSalomon · 2023-11-24T09:21:49Z

/retest

JoelSpeed · 2023-11-24T12:15:41Z

operator/v1/types_network.go

+	// State controls the node's external (aka NS) ipsec service state.
+	// +kubebuilder:validation:Enum="";Enabled;Disabled
+	// +kubebuilder:validation:Required
+	// +kubebuilder:default=Disabled


To use a default the field must be optional and have omitempty in the json tag, if you update this then you can remove the empty string case

You also need a second tag, since multiple tools rely on different tags for understanding the defaults

// + default:=Disabled

I had to play with the syntax but it seems that // +default="Disabled" does the trick and generates the correct openapi.json file. As far as I tested now everything works, and retrieving the CRD immediately after reboot before any change shows this:

defaultNetwork: ovnKubernetesConfig: egressIPConfig: {} gatewayConfig: ipv4: {} ipv6: {} routingViaHost: false genevePort: 6081 ipsecExternal: state: Disabled mtu: 1400

I hope everything is OK now ;-)

JoelSpeed · 2023-11-24T12:16:46Z

operator/v1/types_network.go

@@ -480,6 +486,14 @@ type HybridOverlayConfig struct {
 type IPsecConfig struct {
 }

+type IPsecExternal struct {
+	// State controls the node's external (aka NS) ipsec service state.


Comments should start with the JSON tag name.

I'd appreciate if you could expand the documentation on this field, please review the API conventions on writing good godocs and expand this with relevant information.

yuvalk · 2023-11-27T03:39:17Z

@JoshSalomon I think it' will aslo be nice if you add at least the following test:

diff --git a/operator/v1/stable.network.testsuite.yaml b/operator/v1/stable.network.testsuite.yaml
index 698e4bf4..bac7f67c 100644
--- a/operator/v1/stable.network.testsuite.yaml
+++ b/operator/v1/stable.network.testsuite.yaml
@@ -227,4 +227,22 @@ tests:
               ipv6:
                 internalMasqueradeSubnet: "abcd:ef01:2345:6789:abcd:ef01:2345::/125"
     expectedError: "Invalid value: \"string\": a valid IPv6 address must contain 8 segments unless elided (::), in which case it must contain at most 6 non-empty segments"
-    
\ No newline at end of file
+  - name: Should have state disabled by default when specifiying north-south IPsec
+    initial: |
+      apiVersion: operator.openshift.io/v1
+      kind: Network
+      spec:
+       defaultNetwork:
+          ovnKubernetesConfig:
+            ipsecExternal: {}
+    expected: |
+      apiVersion: operator.openshift.io/v1
+      kind: Network
+      spec:
+        defaultNetwork:
+          ovnKubernetesConfig:
+            ipsecExternal:
+              state: Disabled
+        disableNetworkDiagnostics: false
+        logLevel: Normal
+        operatorLogLevel: Normal

might also want to add a test for state: Enabled

and you can run just this test locally with: GINKGO_EXTRA_ARGS="--focus ipsec" make integration

This is a change based on the api PR (openshift/api#1667) before its approval, the api vendoring is from a private commit - this is needed only for testing until the api PR is merged. Signed-off-by: Josh Salomon <[email protected]>

JoelSpeed

Just need to drop this mention of omitted and we are good to merge

operator/v1/types_network.go

The mode field can have 3 values: - Disabled: Disable the ipsec.service on the node level - External: Enable ipsec.service on the node level, but additional configoration is required (usually by nmstate, usually for external communication) - Full: Enable ipsec.service on the node and configure secure communications between pods in the cluster. If required ipsec can be configured for external secure communications as in the case of mode External Signed-off-by: Josh Salomon <[email protected]>

Signed-off-by: Josh Salomon <[email protected]>

Testing that empty ipsecConfig translates into disabled mode, and that empty strings are not allowed for ipsecConfog.mode Added transition tests (added OnUpdate section to the file) for checking that ipsecConfog.mode can not be removed once set, that ipsec can be safely disabled, and that empty ispecConfig is not changed when changing other ovnKubernetesConfig fields. Additionally updated all successful path tests (without expecterError) since ipsecConfig setting appears now on all the out yaml file as a feature. Signed-off-by: Josh Salomon <[email protected]>

JoelSpeed · 2024-01-08T14:55:01Z

/lgtm

openshift-ci · 2024-01-08T14:55:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, JoshSalomon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yuvalk · 2024-01-08T15:18:43Z

/retest

This is a change based on the api PR (openshift/api#1667) before its approval, the api vendoring is from a private commit - this is needed only for testing until the api PR is merged. Signed-off-by: Josh Salomon <[email protected]>

openshift-ci-robot · 2024-01-08T16:15:00Z

/retest-required

Remaining retests: 0 against base HEAD c7a2d3b and 2 for PR HEAD 0e619cd in total

This is a change based on the api PR (openshift/api#1667) before its approval, the api vendoring is from a private commit - this is needed only for testing until the api PR is merged. Signed-off-by: Josh Salomon <[email protected]>

yuvalk · 2024-01-08T17:45:52Z

/cherry-pick release-4.15

openshift-cherrypick-robot · 2024-01-08T17:45:54Z

@yuvalk: once the present PR merges, I will cherry-pick it on top of release-4.15 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2024-01-08T19:55:52Z

/retest-required

Remaining retests: 0 against base HEAD 7b0f600 and 1 for PR HEAD 0e619cd in total

openshift-ci · 2024-01-08T22:01:19Z

@JoshSalomon: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-cherrypick-robot · 2024-01-08T22:07:12Z

@yuvalk: new pull request created: #1719

In response to this:

/cherry-pick release-4.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This is a change based on the api PR (openshift/api#1667) before its approval, the api vendoring is from a private commit - this is needed only for testing until the api PR is merged. Signed-off-by: Josh Salomon <[email protected]>

openshift-bot · 2024-01-09T09:04:28Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-config-api-container-v4.16.0-202401090832.p0.g355cd25.assembly.stream for distgit ose-cluster-config-api.
All builds following this will include this PR.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2023

openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 16, 2023

openshift-ci bot requested review from bparees and deads2k November 16, 2023 09:59

yuvalk reviewed Nov 16, 2023

View reviewed changes

operator/v1/types_network.go Outdated Show resolved Hide resolved

yuvalk reviewed Nov 16, 2023

View reviewed changes

JoshSalomon changed the title ~~wip-add-IPsecExtern~~ add-IPsecExternal Nov 16, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2023

JoelSpeed reviewed Nov 17, 2023

View reviewed changes

JoshSalomon force-pushed the ipsec-api branch from 3f944fc to 6057171 Compare November 19, 2023 09:49

openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 23, 2023

yuvalk reviewed Nov 23, 2023

View reviewed changes

operator/v1/types_network.go Outdated Show resolved Hide resolved

yuvalk reviewed Nov 23, 2023

View reviewed changes

JoshSalomon force-pushed the ipsec-api branch 2 times, most recently from e1d74f2 to 74a7268 Compare November 23, 2023 20:43

JoelSpeed reviewed Nov 24, 2023

View reviewed changes

JoshSalomon force-pushed the ipsec-api branch 2 times, most recently from 4216db7 to 8fc7980 Compare November 26, 2023 13:37

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 27, 2023

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 7, 2024

JoshSalomon force-pushed the ipsec-api branch from b7445a9 to 829952a Compare January 7, 2024 16:30

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2024

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 7, 2024

JoshSalomon force-pushed the ipsec-api branch from 829952a to f5b6cdd Compare January 7, 2024 17:09

JoelSpeed reviewed Jan 8, 2024

View reviewed changes

operator/v1/types_network.go Outdated Show resolved Hide resolved

JoshSalomon added 3 commits January 8, 2024 15:38

Generted files for the ipsec API commit (aa4df15)

48d8d61

Signed-off-by: Josh Salomon <[email protected]>

JoshSalomon force-pushed the ipsec-api branch from f5b6cdd to 0e619cd Compare January 8, 2024 13:40

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2024

openshift-merge-bot bot merged commit 355cd25 into openshift:master Jan 8, 2024
16 checks passed

openshift-cherrypick-robot mentioned this pull request Jan 8, 2024

OCPBUGS-27186: CNO IPsec API #1719

Merged

add-IPsecExternal #1667

add-IPsecExternal #1667

Conversation

JoshSalomon commented Nov 16, 2023

openshift-ci bot commented Nov 16, 2023

yuvalk commented Nov 16, 2023

yuvalk commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshSalomon Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvalk Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshSalomon commented Nov 19, 2023

yuvalk commented Nov 22, 2023

JoelSpeed commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshSalomon commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvalk commented Nov 27, 2023 • edited Loading

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed commented Jan 8, 2024

openshift-ci bot commented Jan 8, 2024

yuvalk commented Jan 8, 2024

openshift-ci-robot commented Jan 8, 2024

yuvalk commented Jan 8, 2024

openshift-cherrypick-robot commented Jan 8, 2024

openshift-ci-robot commented Jan 8, 2024

openshift-ci bot commented Jan 8, 2024

openshift-cherrypick-robot commented Jan 8, 2024

openshift-bot commented Jan 9, 2024

JoshSalomon Nov 19, 2023 •

edited

Loading

yuvalk Nov 20, 2023 •

edited

Loading

yuvalk commented Nov 27, 2023 •

edited

Loading