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

v1.14 Backports 2023-12-13 #29863

Merged
merged 11 commits into from
Dec 14, 2023
Merged

v1.14 Backports 2023-12-13 #29863

merged 11 commits into from
Dec 14, 2023

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented Dec 13, 2023

Once this PR is merged, a GitHub action will update the labels of these PRs:

 27894 29455 29694 29201 29448 29675 29670 29752 29502

@giorio94 giorio94 added kind/backports This PR provides functionality previously merged into master. backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. labels Dec 13, 2023
leblowl and others added 3 commits December 13, 2023 15:58
[ upstream commit 28975e3 ]

[ backporter's notes: introduced a simplified version of the
  'identity.IsWorld()' method, which was not present in v1.14, as
  added in a94fa56 ("Fix CIDR to World Entity Conversion Bug"). ]

In Hubble, ignore certain cases where the datapath security ID does
not match the userspace security ID.

Signed-off-by: Lucas Leblow <lucasleblow@mailbox.org>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
[ upstream commit 1c10df2 ]

With the removal of setting a nameserver via the little-vm-helper
GitHub action, conformance-runtime tests are failing quite often
with the following error.

```
dial tcp: lookup quay.io on 127.0.0.53:53: read udp 127.0.0.1:40553->127.0.0.53:53: read: connection refused
```

The assumption is that the local DNS resolver isn't ready at that time when pulling the image.

Therefore, this way temporarily re-adds the nameserver `1.1.1.1` manually.

See: cilium/little-vm-helper#118

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
[ upstream commit a2694fc ]

[ backporter's notes: skipped changes in tests-e2e-upgrade.yaml, as not
  present in v1.14. ]

The property dns-resolver has been removed from the little-vm-helper
GitHub action.

Therefore, this commit removes the usage of it in the cilium repository.

See: cilium/little-vm-helper#118

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
Copy link
Member Author

/test-backport-1.14

@giorio94 giorio94 marked this pull request as ready for review December 13, 2023 15:01
@giorio94 giorio94 requested review from a team as code owners December 13, 2023 15:01
pkg/fqdn/name_manager.go Outdated Show resolved Hide resolved
Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

I realized that I forgot to label #29502 with needs-backport/v1.14. This fix is needed in addition to #29455 . Would be great if we could include this. Even though it's again kind of depending on the LVH images that are used in v1.14.

@giorio94
Copy link
Member Author

I realized that I forgot to label #29502 with needs-backport/v1.14. This fix is needed in addition to #29455 . Would be great if we could include this. Even though it's again kind of depending on the LVH images that are used in v1.14.

Added, PTAL

@tklauser
Copy link
Member

tklauser commented Dec 13, 2023

fqdn: avoid converting from netip.Addr to net.IP and back #29625 (@tklauser)

  • ⚠️ MapSelectorsToIPsLocked is still returning a map value is a net.IP slice; hence, let's convert the netip.IPAddr on the fly before assigning it. @tklauser please let me know if you prefer this approach or drop the backport of this PR.

Ah, I see MapSelectorsToIPsLocked only got changed in #29036 which isn't backported (and probably won't be). Without this change backporting my PR is kind of pointless. Let's drop it for now. Sorry for the trouble caused.

Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

My commits look good. Thanks @giorio94

Let's 🙏 that the tests succeed :)

mhofstetter and others added 4 commits December 13, 2023 16:17
[ upstream commit 1f1c384 ]

Cloud provider related workflows use the full configuration as matrix
when being executed on a scheduled basis on stable branches, whereas
only the default configuration is used on PR workflows.

Currently, this decision checks whether the workflow is triggered
via `event_name == schedule`.

This is working fine on `main`, but not on all other stable branches
where the workflows are triggered via workflow_dispatch event (called
by a scheduled workflow (ariane-scheduled.yaml) on main).

Therefore, this commit extends the decision to check for the input `PR-number`
starting with a "v". This is the case for Ariane triggered runs - as they pass
the branch name as PR-number (PR runs pass the actual numeric PR number).

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
[ upstream commit 7e13f1a ]

[ backporter's notes: dropped the Registry parameter from the new invoke
  function in pkg/metrics/cell.go, as it is not provided. ]

Because legacy metrics are now initialized in Hive, the logging hook was
being set to the NoOpCounterVec instance.

This moves initializing the errors/warnings metric out of the
NewLegacyMetrics function and provides a manual way to init metrics that
must be initialized prior to Hive.

Fixes: #29525

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
[ upstream commit 83b87c4 ]

This comment is over 5 years old and no longer seems relevant.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
[ upstream commit 854ea3d ]

The poststart-eni.sh script got recently modified to delete both AWS-SNAT
and AWS-CONNMARK related chains. Yet, the filter is now a regular expression,
and does not return any match with plain grep. Let's fix this by setting the
`-E` (--extended-regexp) flag.

Fixes: b836cb1 ("helm: Add missing type to poststart iptables regex")
Fixes: 8c86d07 ("install: Remove AWS-CONNMARK-CHAIN iptables")
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
giorio94 and others added 4 commits December 13, 2023 16:17
[ upstream commit dbe56dd ]

Now that known issues causing connection disruption (which appeared to
mostly affect dual stack clusters) have been fixed, let's enable IPv6
again in the clustermesh upgrade/downgrade workflow.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
[ upstream commit 5107391 ]

Recently, a regression of the poststart-eni.sh script got unnoticed,
as we are not explicitly testing it as part of the E2E pipelines.

Let's add a step to the Conformance EKS and Conformance AWS-CNI
workflows to assert that, in the former case, the stale AWS iptables
chains are removed, and in the latter they are not modified.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
[ upstream commit c947484 ]

It turns out that the following find command is subtle:

    find . -name foo -o -name bar -exec ...

This means: execute the command for bar. It doesn't execute the command for foo.
For this reason verifier logs are currently not being copied correctly into
GH artifacts.

Fixes: 735807f ("test/verifier: fix complexity tests not being recompiled")
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
[ upstream commit 83425f2 ]

It seems that even though we're setting the nameserver at the top
of `/etc/resolv.conf`, in some cases docker still uses 127.0.0.53
to resolve names while pulling the cilium docker plugin in the
ci-runtime test.

It seems as docker tries to use nameserver information from
`systemd-resolved`.

Therefore, this commit tries to force docker to use the nameserver
1.1.1.1, by removing the resolv.conf symlink, deleting the resolv.conf
from systemd-resolved and restarting the docker service after applying
the changes.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@tklauser tklauser removed their request for review December 13, 2023 15:18
@giorio94
Copy link
Member Author

Ah, I see MapSelectorsToIPsLocked only got changed in #29036 which isn't backported (and probably won't be). Without this change backporting my PR is kind of pointless. Let's drop it for now. Sorry for the trouble caused.

Dropped, thanks for confirming.

@giorio94
Copy link
Member Author

/test-backport-1.14

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

I tried removing myself from the reviewer list after my PR was dropped but that seems to cause my requested changes to block the PR again. Approving to unblock this PR.

@giorio94
Copy link
Member Author

Both Conformance E2E and Conformance Cluster Mesh failures appear to be due to slow image retrieval from quay.
One of the Conformance Ginkgo matrix entries failed during LVH VM setup.

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thank you, approving #27894

@giorio94
Copy link
Member Author

Both Conformance E2E and Conformance Cluster Mesh failures appear to be due to slow image retrieval from quay.

The same happened for one of the Conformance IPsec E2E matrix entries. Currently rerunning

@giorio94
Copy link
Member Author

CI is green. Marking ready to merge as it only misses the final approval from @cilium/tophat, who will be merging the PR.

@giorio94 giorio94 added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 14, 2023
@aanm aanm merged commit afe175b into v1.14 Dec 14, 2023
213 checks passed
@aanm aanm deleted the pr/v1.14-backport-2023-12-13 branch December 14, 2023 12:47
@maintainer-s-little-helper maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants