salt: Support `127.0.0.1` hostPort #3396

TeddyAndrieux · 2021-05-26T08:28:34Z

Component:

'salt'

Context:

Support 127.0.0.1 as hostPort

Summary:

Before this commit we only support hostPort on workloadPlane IP, this
commit also add 127.0.0.1 as supported hostPort

NOTE: It's needed for Kubernetes conformance tests for Kubernetes 1.20+

bert-e · 2021-05-26T08:28:36Z

Hello teddyandrieux,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2021-05-26T08:28:42Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/allow-hostport-on-localhost into
development/2.9
w/2.10/improvement/allow-hostport-on-localhost will be merged into development/2.10

The following branches will NOT be impacted:

development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6
development/2.7
development/2.8

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

bert-e · 2021-05-26T08:28:44Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

Before this commit we only support hostPort on workloadPlane IP, this commit also add `127.0.0.1` as supported hostPort NOTE: It's needed for Kubernetes conformance tests for Kubernetes 1.20+

bert-e · 2021-05-26T08:29:18Z

History mismatch

Merge commit #ee5ee2a3b98f668eeb3574e214ae383e638dada1 on the integration branch
w/2.10/improvement/allow-hostport-on-localhost is merging a branch which is neither the current
branch improvement/allow-hostport-on-localhost nor the development branch
development/2.10.

It is likely due to a rebase of the branch improvement/allow-hostport-on-localhost and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

TeddyAndrieux · 2021-05-26T08:29:23Z

/reset

bert-e · 2021-05-26T08:29:29Z

Reset complete

I have successfully deleted this pull request's integration branches.

bert-e · 2021-05-26T08:29:36Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/allow-hostport-on-localhost into
development/2.9
w/2.10/improvement/allow-hostport-on-localhost will be merged into development/2.10

The following branches will NOT be impacted:

development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6
development/2.7
development/2.8

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

bert-e · 2021-05-26T08:29:37Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

alexandre-allard · 2021-05-26T08:32:07Z

salt/metalk8s/kubernetes/cni/calico/configured.sls

@@ -63,7 +63,7 @@ Create CNI calico configuration file:
            snat: true
            capabilities:
              portMappings: true
-            conditionsV4: ["-d", "{{ grains.metalk8s.workload_plane_ip }}/32"]
+            conditionsV4: ["-d", "{{ grains.metalk8s.workload_plane_ip }}/32,127.0.0.1/32"]


Shouldn't it be on the whole 127.0.0.0/8 network ?

Hmmm I'm not sure, to me here we only want localhost and not any other "loopback" addresses used for .... whatelse
I think but not sure

IIUC, what we want is "all host-local addresses except control plane", no?

To me, we want workload plane IP and that's it (just we add 127.0.0.1 as it's needed for conformance tests and does not hurt anyway)

I don't really know what we should do either and I'm not even sure that's really important.
We can merge your changes as that's only what's requested by the K8s conformance tests.
But I don't see any valid reason to restrict to only 127.0.0.1/32 instead of 127.0.0.0/8

Agree 👍,
Maybe we will want to make it configurable at some point also, but for the moment let's not bother about it 😃

Likely, yes.

Though, maybe the current change is sufficient. It passes conformance, so it's supposed to be conformant. I'm not sure where it's said a K8s cluster must expose NodePorts on 127.0.0.0/8. If this is the case, maybe we should widen things up further.

TeddyAndrieux · 2021-05-26T09:54:30Z

/approve

bert-e · 2021-05-26T09:54:40Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/2.9
✔️ development/2.10

The following branches will NOT be impacted:

development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6
development/2.7
development/2.8

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

NicolasT · 2021-05-26T09:55:13Z

Others ran into this as well:

SchedulerPredicates: pods with same hostPort but different hostIP and protocol cilium/cilium#14287
Walk back on a sig-network conformance test kubernetes/kubernetes#100522

bert-e · 2021-05-26T11:13:41Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/2.9
✔️ development/2.10

The following branches have NOT changed:

development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6
development/2.7
development/2.8

Please check the status of the associated issue None.

Goodbye teddyandrieux.

NicolasT · 2021-05-26T13:26:47Z

For future reference, cfr. https://kubernetes.slack.com/archives/C09QYUH5W/p1622023243096200

TeddyAndrieux added topic:networking Networking-related issues topic:deployment Bugs in or enhancements to deployment stages complexity:easy Something that requires less than a day to fix labels May 26, 2021

TeddyAndrieux requested a review from a team May 26, 2021 08:28

salt: Support 127.0.0.1 hostPort

91b4dc1

Before this commit we only support hostPort on workloadPlane IP, this commit also add `127.0.0.1` as supported hostPort NOTE: It's needed for Kubernetes conformance tests for Kubernetes 1.20+

TeddyAndrieux force-pushed the improvement/allow-hostport-on-localhost branch from ee5ee2a to 91b4dc1 Compare May 26, 2021 08:29

alexandre-allard approved these changes May 26, 2021

View reviewed changes

alexandre-allard reviewed May 26, 2021

View reviewed changes

bert-e merged commit 6f71984 into development/2.9 May 26, 2021

bert-e deleted the improvement/allow-hostport-on-localhost branch May 26, 2021 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

salt: Support `127.0.0.1` hostPort #3396

salt: Support `127.0.0.1` hostPort #3396

TeddyAndrieux commented May 26, 2021

bert-e commented May 26, 2021

bert-e commented May 26, 2021

bert-e commented May 26, 2021

bert-e commented May 26, 2021

TeddyAndrieux commented May 26, 2021

bert-e commented May 26, 2021

bert-e commented May 26, 2021

bert-e commented May 26, 2021

alexandre-allard May 26, 2021

TeddyAndrieux May 26, 2021

gdemonet May 26, 2021

TeddyAndrieux May 26, 2021

alexandre-allard May 26, 2021 •

edited

Loading

TeddyAndrieux May 26, 2021

NicolasT May 26, 2021

NicolasT May 26, 2021

TeddyAndrieux commented May 26, 2021

bert-e commented May 26, 2021

NicolasT commented May 26, 2021

bert-e commented May 26, 2021

NicolasT commented May 26, 2021

salt: Support 127.0.0.1 hostPort #3396

salt: Support 127.0.0.1 hostPort #3396

Conversation

TeddyAndrieux commented May 26, 2021

bert-e commented May 26, 2021

Hello teddyandrieux,

bert-e commented May 26, 2021

Integration data created

bert-e commented May 26, 2021

Waiting for approval

bert-e commented May 26, 2021

History mismatch

TeddyAndrieux commented May 26, 2021

bert-e commented May 26, 2021

Reset complete

bert-e commented May 26, 2021

Integration data created

bert-e commented May 26, 2021

Waiting for approval

alexandre-allard May 26, 2021

Choose a reason for hiding this comment

TeddyAndrieux May 26, 2021

Choose a reason for hiding this comment

gdemonet May 26, 2021

Choose a reason for hiding this comment

TeddyAndrieux May 26, 2021

Choose a reason for hiding this comment

alexandre-allard May 26, 2021 • edited Loading

Choose a reason for hiding this comment

TeddyAndrieux May 26, 2021

Choose a reason for hiding this comment

NicolasT May 26, 2021

Choose a reason for hiding this comment

NicolasT May 26, 2021

Choose a reason for hiding this comment

TeddyAndrieux commented May 26, 2021

bert-e commented May 26, 2021

In the queue

NicolasT commented May 26, 2021

bert-e commented May 26, 2021

NicolasT commented May 26, 2021

salt: Support `127.0.0.1` hostPort #3396

salt: Support `127.0.0.1` hostPort #3396

alexandre-allard May 26, 2021 •

edited

Loading