providers, podman: support podman 2.0 #1728

giuseppe · 2020-07-13T20:54:17Z

fix some issues that prevented using podman 2.0

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

pkg/cluster/internal/providers/podman/provider.go

pkg/cluster/internal/providers/podman/provision.go

aojea · 2020-07-13T21:17:28Z

sorry, but we don't have CI for podman, is this going to work for both 1.9 and 2.0?

giuseppe · 2020-07-14T06:40:12Z

sorry, but we don't have CI for podman, is this going to work for both 1.9 and 2.0?

yes, I've tested it both with 1.9 and 2.0

giuseppe · 2020-07-14T10:50:46Z

/test pull-kind-conformance-parallel-1-16

pkg/cluster/internal/providers/podman/provider.go

the podman inspect format changed, attempt to parse using both formats. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

aojea · 2020-07-15T07:21:17Z

pkg/cluster/internal/providers/podman/provision.go

@@ -180,7 +180,6 @@ func runArgsForNode(node *config.Node, clusterIPFamily config.ClusterIPFamily, n
 		// for now this is what we want. in the future we may revisit this.
 		"--privileged",
 		"--security-opt", "seccomp=unconfined", // also ignore seccomp
-		"--security-opt", "apparmor=unconfined", // also ignore apparmor


is there any side effect removing this?

ic

Command Output: Error: invalid config provided: AppArmorProfile and privileged are mutually exclusive options

I'm not very much into apparmor, so my question is about the user experience, does this means the users need to disable apparmor or use specific profiles, or just will work?

Let's just specify --privileged and drop all --security-opt *.

Let's just specify --privileged and drop all --security-opt *.

that is better, thanks for the suggestion. I've updated the PR

how does this apply to older versions of podman?

were these flags just ignored? docker does not throw this error.

I think it is just a regression in Podman that raises an error when both --privileged and --security-opt apparmor=* are used. There is already an issue to track the problem in podman 2.0 so that --security-opt apparmor= overrides --privileged. For our use case though it doesn't matter since --privileged implies no apparmor and selinux

SGTM, I just want to make sure we're not not-reporting these 😅

aojea · 2020-07-15T08:06:42Z

@mheon talking about compatibility , if I use the same command we are using in the docker provider, podman 2.0 fails :/

kind/pkg/cluster/internal/providers/docker/provider.go

Lines 160 to 166 in c9469c6

    
           // retrieve the specific port mapping using docker inspect 
        
           cmd := exec.Command( 
        
           	"docker", "inspect", 
        
           	"--format", fmt.Sprintf( 
        
           		"{{ with (index (index .NetworkSettings.Ports \"%d/tcp\") 0) }}{{ printf \"%%s\t%%s\" .HostIp .HostPort }}{{ end }}", common.APIServerInternalPort, 
        
           	), 
        
           	n.String(),

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

aojea · 2020-07-15T13:09:14Z

/retest
/lgtm
hope we don't have more differences between 1.9 and 2.x
Thanks
/assign @BenTheElder

giuseppe · 2020-07-15T16:23:36Z

/test pull-kind-conformance-parallel-1-16

giuseppe · 2020-07-15T18:58:48Z

/test pull-kind-conformance-parallel-1-16

BenTheElder · 2020-07-15T19:53:56Z

1.16 has a regression somewhere in the stack, I'll bypass that job when we're ready to merge.

BenTheElder · 2020-07-15T19:54:30Z

pkg/cluster/internal/providers/podman/provider.go

@@ -182,20 +182,45 @@ func (p *Provider) GetAPIServerEndpoint(cluster string) (string, error) {
 		return "", errors.Errorf("network details should only be one line, got %d lines", len(lines))
 	}

-	// portMapping maps to the standard CNI portmapping capability
+	// portMapping19 maps to the standard CNI portmapping capability used in podman 1.9


this also applies to other < 2.0, right?

yes, this is the struct used by podman <= 1.9

maybe we should note that /shrug

BenTheElder · 2020-07-15T19:55:20Z

pkg/cluster/internal/providers/podman/provision.go

@@ -355,7 +353,7 @@ func generatePortMappings(clusterIPFamily config.ClusterIPFamily, portMappings .
 		if strings.HasSuffix(hostPortBinding, ":0") {
 			hostPortBinding = strings.TrimSuffix(hostPortBinding, "0")
 		}
-		args = append(args, fmt.Sprintf("--publish=%s:%d/%s", hostPortBinding, pm.ContainerPort, protocol))
+		args = append(args, fmt.Sprintf("--publish=%s:%d/%s", hostPortBinding, pm.ContainerPort, strings.ToLower(protocol)))


aside: this really seems like a bug in podman.

Already fixed on master, but it slipped into a few published releases

docker accepts uppercase AND this is the canonical value in CRI, which is why we're using uppercase https://github.com/kubernetes/cri-api/blob/34366a3c19379c566a82895ce1bcb75d9a502fac/pkg/apis/runtime/v1alpha2/api.pb.go#L58

ack, thanks

BenTheElder · 2020-07-15T20:54:05Z

/lgtm
/approve
/hold

k8s-ci-robot · 2020-07-15T20:54:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, giuseppe

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~~ [BenTheElder]

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

BenTheElder · 2020-07-16T03:47:55Z

We need to file a bug about 1.16
/override pull-kind-conformance-parallel-1-16

k8s-ci-robot · 2020-07-16T03:48:01Z

@BenTheElder: Overrode contexts on behalf of BenTheElder: pull-kind-conformance-parallel-1-16

In response to this:

We need to file a bug about 1.16
/override pull-kind-conformance-parallel-1-16

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.

BenTheElder · 2020-07-16T03:48:06Z

/hold cancel

k8s-ci-robot requested review from aojea and krzyzacy July 13, 2020 20:54

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 13, 2020

BenTheElder requested review from amwat and removed request for krzyzacy July 13, 2020 21:02

aojea reviewed Jul 13, 2020

View reviewed changes

pkg/cluster/internal/providers/podman/provider.go Show resolved Hide resolved

aojea reviewed Jul 13, 2020

View reviewed changes

pkg/cluster/internal/providers/podman/provision.go Outdated Show resolved Hide resolved

BenTheElder added this to the v0.9.0 milestone Jul 14, 2020

giuseppe force-pushed the podman-fixes branch from e8888aa to 5bf1bb9 Compare July 14, 2020 06:39

aojea reviewed Jul 14, 2020

View reviewed changes

pkg/cluster/internal/providers/podman/provider.go Show resolved Hide resolved

AkihiroSuda mentioned this pull request Jul 15, 2020

Podman: json: cannot unmarshal object into Go value of type []podman.portMapping #1734

Closed

providers, podman: support podman 2.0

6bcbd70

the podman inspect format changed, attempt to parse using both formats. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the podman-fixes branch from 5bf1bb9 to be19abf Compare July 15, 2020 06:57

aojea reviewed Jul 15, 2020

View reviewed changes

giuseppe added 2 commits July 15, 2020 12:55

podman: apparmor cannot be specified with privileged

b3032fb

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

podman: specify protocol lowercase

36d7135

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the podman-fixes branch from be19abf to 36d7135 Compare July 15, 2020 10:56

k8s-ci-robot assigned BenTheElder and aojea Jul 15, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2020

BenTheElder reviewed Jul 15, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2020

k8s-ci-robot merged commit 0cd238e into kubernetes-sigs:master Jul 16, 2020

BenTheElder mentioned this pull request Jul 29, 2020

Cannot start with podman backend due to AppArmor errors #1757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

providers, podman: support podman 2.0 #1728

providers, podman: support podman 2.0 #1728

giuseppe commented Jul 13, 2020

aojea commented Jul 13, 2020

giuseppe commented Jul 14, 2020

giuseppe commented Jul 14, 2020

aojea Jul 15, 2020

aojea Jul 15, 2020

AkihiroSuda Jul 15, 2020

giuseppe Jul 15, 2020

BenTheElder Jul 15, 2020

BenTheElder Jul 15, 2020

giuseppe Jul 15, 2020

BenTheElder Jul 15, 2020

aojea commented Jul 15, 2020

aojea commented Jul 15, 2020

giuseppe commented Jul 15, 2020

giuseppe commented Jul 15, 2020

BenTheElder commented Jul 15, 2020

BenTheElder Jul 15, 2020

giuseppe Jul 15, 2020

BenTheElder Jul 15, 2020

BenTheElder Jul 15, 2020

mheon Jul 15, 2020

BenTheElder Jul 15, 2020

BenTheElder Jul 15, 2020

BenTheElder commented Jul 15, 2020

k8s-ci-robot commented Jul 15, 2020

BenTheElder commented Jul 16, 2020

k8s-ci-robot commented Jul 16, 2020

BenTheElder commented Jul 16, 2020

providers, podman: support podman 2.0 #1728

providers, podman: support podman 2.0 #1728

Conversation

giuseppe commented Jul 13, 2020

aojea commented Jul 13, 2020

giuseppe commented Jul 14, 2020

giuseppe commented Jul 14, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jul 15, 2020

aojea commented Jul 15, 2020

giuseppe commented Jul 15, 2020

giuseppe commented Jul 15, 2020

BenTheElder commented Jul 15, 2020

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

Choose a reason for hiding this comment

BenTheElder commented Jul 15, 2020

k8s-ci-robot commented Jul 15, 2020

BenTheElder commented Jul 16, 2020

k8s-ci-robot commented Jul 16, 2020

BenTheElder commented Jul 16, 2020